Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

XRENDERING-705: Footnote macro produces inline content in standalone mode #299

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+36 −8
Open

XRENDERING-705: Footnote macro produces inline content in standalone mode #299

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions ...ootnotes/src/main/java/org/xwiki/rendering/internal/macro/footnote/PutFootnotesMacro.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
import org.xwiki.rendering.block.ListItemBlock;
import org.xwiki.rendering.block.MacroMarkerBlock;
import org.xwiki.rendering.block.NumberedListBlock;
import org.xwiki.rendering.block.ParagraphBlock;
import org.xwiki.rendering.block.SpaceBlock;
import org.xwiki.rendering.block.WordBlock;
import org.xwiki.rendering.block.match.MacroMarkerBlockMatcher;
Expand Down Expand Up @@ -192,6 +193,7 @@ public List<Block> execute(FootnoteMacroParameters parameters, String content, M
// Get the list of footnotes in the document
macroMarkerBlocks.stream()
.filter(macro -> FootnoteMacro.MACRO_NAME.equals(macro.getId()))
.map(PutFootnotesMacro::repairStandaloneFootnote)
.map(Footnote::new)
.forEach(footnote -> footnotes.put(
Objects.requireNonNullElseGet(footnote.id, () -> String.valueOf(temporaryId.getAndIncrement())),
Expand Down Expand Up @@ -228,6 +230,25 @@ public List<Block> execute(FootnoteMacroParameters parameters, String content, M
return result;
}

/**
* Repair a standalone footnote macro marker block by wrapping it in a paragraph if it is not already inline.
*
* @param macroMarkerBlock the macro marker block to repair
* @return the repaired macro marker block
*/
private static MacroMarkerBlock repairStandaloneFootnote(MacroMarkerBlock macroMarkerBlock)
{
if (macroMarkerBlock.isInline()) {
return macroMarkerBlock;
} else {
// Wrap the macro marker block in a paragraph and make it inline.
MacroMarkerBlock result = new MacroMarkerBlock(macroMarkerBlock.getId(), macroMarkerBlock.getParameters(),
macroMarkerBlock.getContent(), macroMarkerBlock.getChildren(), true);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tmortagne I'm wondering if I should move the attributes of the block, too, any opinions on that?

Copy link
Member

@tmortagne tmortagne May 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have any attribute on MacroMarkerBlock right now AFAIK, but I feel it would be more future-proof to keep them on the new MacroMarkerBlock. We might have the same problem in other places where we do this kind of block refactoring.

That being said, shouldn't the content of a standalone footnote be standalone from the start ? It's not clear to me why it needs to be repaired.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two things that make the footnote macro special:

  1. The initially produced content doesn't stay in the footnote but is moved to the putFootnotes macro by the putFootnotes macro and new content is set by the putFootnotes macro. So initially, the footnote macro should always produce inline content as that's what fits better in putFootnotes (it puts it inside a list item).
  2. While the new content that is added by the putFootnotes macro could be made standalone by wrapping it inside a paragraph, I felt that while technically correct, this won't provide a good user experience in the WYSIWYG editor. It doesn't make any sense for a footnote to be standalone and the WYSIWYG editor provides no way to change the footnote back to inline.

Phrased differently, what I would actually want is a way to indicate that the footnote macro doesn't support standalone mode. As this is not exactly common (I think), I did the quick solution to just wrap the footnote macro in a paragraph.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a way to indicate that the footnote macro doesn't support standalone mode

Indeed, we never thought about that need, thinking it's always easy to display something standalone if you can display it inline, but it would make sense for the footnote. I guess we either need a new Macro#supportsStandaloneMode, or deprecate #supportsInlineMode and introduce something more generic like an enum based #supportedModes (and maybe take that opportinuty to put it in the MacroDescriptor instead).

macroMarkerBlock.getParent().replaceChild(new ParagraphBlock(List.of(result)), macroMarkerBlock);
return result;
}
}

/**
* Collect and remove footnote contents from the given putFootnotes macro marker block.
*
Expand Down
19 changes: 13 additions & 6 deletions ...i-rendering-macros/xwiki-rendering-macro-footnotes/src/test/resources/macrofootnote6.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
.#-----------------------------------------------------
.input|xwiki/2.0
.# Verify that several footnotes and putFootnotes markers work.
.# Also verify that standalone footnote macros are converted to inline macros in a paragraph.
.#-----------------------------------------------------
Footnote{{footnote}}content 1{{/footnote}} {{footnote}}content 2{{/footnote}}

Expand Down Expand Up @@ -46,25 +47,30 @@ endFormat [SUPERSCRIPT] [[class]=[footnoteRef][id]=[x_footnote_ref_2]]
endMacroMarkerInline [footnote] [] [content 2]
endParagraph
beginMacroMarkerStandalone [executedcontent] [] [{{footnote}}Inner footnote{{/footnote}}]
beginMacroMarkerStandalone [footnote] [] [Inner footnote]
beginParagraph
beginMacroMarkerInline [footnote] [] [Inner footnote]
beginFormat [SUPERSCRIPT] [[class]=[footnoteRef][id]=[x_footnote_ref_1]]
beginLink [Typed = [true] Type = [doc] Parameters = [[anchor] = [x_footnote_1]]] [false]
onWord [3]
endLink [Typed = [true] Type = [doc] Parameters = [[anchor] = [x_footnote_1]]] [false]
endFormat [SUPERSCRIPT] [[class]=[footnoteRef][id]=[x_footnote_ref_1]]
endMacroMarkerStandalone [footnote] [] [Inner footnote]
endMacroMarkerInline [footnote] [] [Inner footnote]
endParagraph
endMacroMarkerStandalone [executedcontent] [] [{{footnote}}Inner footnote{{/footnote}}]
beginMacroMarkerStandalone [executedcontent] [] [{{footnote}}Second inner footnote{{/footnote}}]
beginMacroMarkerStandalone [footnote] [] [Second inner footnote]
beginParagraph
beginMacroMarkerInline [footnote] [] [Second inner footnote]
beginFormat [SUPERSCRIPT] [[class]=[footnoteRef][id]=[x_footnote_ref_1-1]]
beginLink [Typed = [true] Type = [doc] Parameters = [[anchor] = [x_footnote_1-1]]] [false]
onWord [4]
endLink [Typed = [true] Type = [doc] Parameters = [[anchor] = [x_footnote_1-1]]] [false]
endFormat [SUPERSCRIPT] [[class]=[footnoteRef][id]=[x_footnote_ref_1-1]]
endMacroMarkerStandalone [footnote] [] [Second inner footnote]
endMacroMarkerInline [footnote] [] [Second inner footnote]
endParagraph
endMacroMarkerStandalone [executedcontent] [] [{{footnote}}Second inner footnote{{/footnote}}]
onEmptyLines [1]
beginMacroMarkerStandalone [footnote] [] [These are the footnotes:
beginParagraph
beginMacroMarkerInline [footnote] [] [These are the footnotes:

{{putFootnotes/}}
]
Expand All @@ -73,10 +79,11 @@ beginLink [Typed = [true] Type = [doc] Parameters = [[anchor] = [x_footnote_5]]]
onWord [5]
endLink [Typed = [true] Type = [doc] Parameters = [[anchor] = [x_footnote_5]]] [false]
endFormat [SUPERSCRIPT] [[class]=[footnoteRef][id]=[x_footnote_ref_5]]
endMacroMarkerStandalone [footnote] [] [These are the footnotes:
endMacroMarkerInline [footnote] [] [These are the footnotes:

{{putFootnotes/}}
]
endParagraph
beginMacroMarkerStandalone [putFootnotes] []
beginList [NUMBERED] [[class]=[footnotes]]
beginListItem
Expand Down
4 changes: 2 additions & 2 deletions xwiki-rendering-standalone/src/test/java/org/xwiki/rendering/example/ExampleTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,8 +164,8 @@ public void executeAllBundledMacros() throws Exception
+ "<div class=\"box warningmessage\"><p>warning</p></div>"
+ "<div class=\"box errormessage\"><p>error</p></div>"
+ "<p><strong>bold</strong></p>"
+ "<sup><span id=\"x_footnote_ref_1\" class=\"footnoteRef\"><span class=\"wikilink\">"
+ "<a href=\"#x_footnote_1\">1</a></span></span></sup>"
+ "<p><sup><span id=\"x_footnote_ref_1\" class=\"footnoteRef\"><span class=\"wikilink\">"
+ "<a href=\"#x_footnote_1\">1</a></span></span></sup></p>"
+ "<ol class=\"footnotes\"><li><span class=\"wikilink\">"
+ "<a id=\"x_footnote_1\" class=\"footnoteBackRef\" href=\"#x_footnote_ref_1\">^</a>"
+ "</span> footnote</li></ol>";
Expand Down