Fixing format syntax after macro and verbatim syntax

Development
1

Hi everyone,

with the help of Claude Code, I’ve had another look at XRENDERING-766 and I have a possible fix that correctly preserves the empty parameters after macro and verbatim syntax like {{icon name="add"/}}(% id="test" %), avoiding that it applies to either the macro or the next paragraph.

However, I noticed a deeper problem here that is independent of the concrete fix:

The standard model of WikiModel is that empty parameter syntax is discarded. If the content was icon (% id="test" %), we would also discard that format syntax, even if a closing (%%) was added.

I see several options now:

  1. We implement the expected behavior expressed in XRENDERING-766 and keep the empty format, but only in that exact situation, i.e., a macro/verbatim syntax at the start of the line followed by an empty format. This seems inconsistent, but the empty format here matters as it is what makes the verbatim or macro inline and not standalone. In the case of a line with further text content, it doesn’t matter as the content of the line is inline, anyway.
  2. We ignore the empty format as we do in a regular paragraph (or list item), but keep the macro/verbatim inline. This doesn’t survive rendering roundtrips as after rendering back to XWiki syntax, the format is gone and the macro or verbatim is standalone now.
  3. We ignore the empty format as we do in a regular paragraph (or list item) and the macro or verbatim syntax is treated as standalone verbatim/macro.
  4. We keep all empty format syntax that has parameters (so an empty bold text would be discarded, an empty format with an id attribute would be kept).
  5. We keep all empty format syntax.

Note that the first and the last two options might have unintended consequences for WYSIWYG editing as empty format syntax is most likely invisible in WYSIWYG editing.

To me, option 3 seems to be the most consistent one given the existing behavior of WikiModel, but it also doesn’t really fix the original bug. For that, I guess option 1 would be best. But then the question is if we shouldn’t rather implement option 4, but I fear a bit the possible breakages, so this seems a bit better suited for a new syntax version not based on WikiModel.

@mleduc After looking a bit at XWIKI-21973 from which you created the rendering issue, I have the feeling that the example given in XRENDERING-766 is wrong as it misses the key detail that the format is non-empty. Could you confirm that empty formats aren’t interesting here? If yes, I would prefer implementing option 3 as it seems the cleanest (while ensuring that a non-empty format actually keeps the macro as inline).

2

It is unclear to me what “empty format” is exactly.

3

An empty format is a format syntax whose content is empty, like (% id="test" %)(%%), or just (% id="test" %) at the end of a paragraph/list item/…, so in XDOM it would be represented as a FormatBlock without children.

4

Note that <!--startmacro:info|-||-|\{\{mention reference="XWiki.Admin" style="FULL_NAME" anchor="XWiki-Admin-7qcxlk"/}}(% id="xwiki-macro-inline-enforcer" %) --> from XWIKI-21973 is not an example of an empty format, there is a space character after the format syntax which means that there is a space character inside the format. I believe XRENDERING-766 incorrectly characterizes this as “empty block” while the format block is actually not empty.

5

4. seems to be the cleanest to me. I doubt there really are use cases which rely on this auto cleanup enough for 4. to really break them.

6

I’d intuitively say 4 as well.

I’m quite sure I’ve been running into, had to fight against and work around wikimodel “helpfully” dropping empty formats with parameters while handling Confluence syntax.

It occurred in particular when handling inline comments, which was originally handled by Thomas using formats and which I had to replace with fake, hackish macros and I more or less lost confidence in beginFormat and endFormat because of this.

Not dropping empty formats with parameters would surely make things easier and cleaner.

7

Thank you very much for your input @tmortagne and @rjakse. I’ll see if I can develop such a fix. However, I’m wondering if we should also have a safer fix (like option 2 or 3) that we can backport on LTS branches as I don’t think we can backport option 4. What do you think?

8

I don’t have an opinion on this. Improving things on LTS without risking introducing too big a change, and then having a more comprehensive fix for newer versions seems reasonable to me.

9

I would not be against 4 for LTS too, but yes it’s maybe too risky.

But I think it would IMO be a very bad idea to do 3 and then drop that behavior for 4 on more recent versions, as it would be quite a breakage for something you expect to be standalone to suddenly turn inline when you upgrade.