Commit c1a4522e authored by Michael Hamann's avatar Michael Hamann
Browse files

XRENDERING-631: The annotated HTML5 renderer should not render figcaption inside macros

* Change the annotated HTML5 renderer to only render figure captions when inside a figure.
* Extend BlockStateChainingListener with an event stack to allow getting
  the parent event.
* Extend BlockStateChainingListenerTest to test for parent events.
parent 38c9b46e
......@@ -29,6 +29,7 @@
import org.xwiki.rendering.listener.MetaData;
import org.xwiki.rendering.listener.reference.ResourceReference;
import org.xwiki.rendering.syntax.Syntax;
import org.xwiki.stability.Unstable;
/**
* Indicates block element for which we are inside and previous blocks.
......@@ -79,6 +80,8 @@ public class BlockStateChainingListener extends AbstractChainingListener impleme
private Event previousEvent = Event.NONE;
private final Deque<Event> eventStack = new ArrayDeque<>();
private int inlineDepth;
private boolean isInParagraph;
......@@ -248,8 +251,31 @@ public boolean isInMacro()
return getMacroDepth() > 0;
}
/**
* @return The event that encloses the current event.
* @since 14.0RC1
*/
@Unstable
public Event getParentEvent()
{
return this.eventStack.peek();
}
// Events
/**
* {@inheritDoc}
*
* @since 14.0RC1
*/
@Override
public void beginDocument(MetaData metadata)
{
super.beginDocument(metadata);
this.eventStack.push(Event.DOCUMENT);
}
@Override
public void beginDefinitionDescription()
{
......@@ -257,6 +283,8 @@ public void beginDefinitionDescription()
++this.definitionListDepth.peek().definitionListItemIndex;
super.beginDefinitionDescription();
this.eventStack.push(Event.DEFINITION_DESCRIPTION);
}
/**
......@@ -270,6 +298,8 @@ public void beginDefinitionList(Map<String, String> parameters)
this.definitionListDepth.push(new DefinitionListState());
super.beginDefinitionList(parameters);
this.eventStack.push(Event.DEFINITION_LIST);
}
@Override
......@@ -279,6 +309,8 @@ public void beginDefinitionTerm()
++this.definitionListDepth.peek().definitionListItemIndex;
super.beginDefinitionTerm();
this.eventStack.push(Event.DEFINITION_TERM);
}
/**
......@@ -292,6 +324,8 @@ public void beginLink(ResourceReference reference, boolean freestanding, Map<Str
++this.linkDepth;
super.beginLink(reference, freestanding, parameters);
this.eventStack.push(Event.LINK);
}
@Override
......@@ -300,6 +334,8 @@ public void beginList(ListType type, Map<String, String> parameters)
this.listDepth.push(new ListState());
super.beginList(type, parameters);
this.eventStack.push(Event.LIST);
}
@Override
......@@ -309,6 +345,8 @@ public void beginListItem()
++this.listDepth.peek().listItemIndex;
super.beginListItem();
this.eventStack.push(Event.LIST_ITEM);
}
@Override
......@@ -318,6 +356,8 @@ public void beginListItem(Map<String, String> parameters)
++this.listDepth.peek().listItemIndex;
super.beginListItem(parameters);
this.eventStack.push(Event.LIST_ITEM);
}
@Override
......@@ -326,6 +366,34 @@ public void beginMacroMarker(String name, Map<String, String> parameters, String
++this.macroDepth;
super.beginMacroMarker(name, parameters, content, isInline);
this.eventStack.push(Event.MACRO_MARKER);
}
/**
* {@inheritDoc}
*
* @since 14.0RC1
*/
@Override
public void beginMetaData(MetaData metadata)
{
super.beginMetaData(metadata);
this.eventStack.push(Event.META_DATA);
}
/**
* {@inheritDoc}
*
* @since 14.0RC1
*/
@Override
public void beginGroup(Map<String, String> parameters)
{
super.beginGroup(parameters);
this.eventStack.push(Event.GROUP);
}
@Override
......@@ -335,6 +403,8 @@ public void beginParagraph(Map<String, String> parameters)
++this.inlineDepth;
super.beginParagraph(parameters);
this.eventStack.push(Event.PARAGRAPH);
}
@Override
......@@ -343,6 +413,8 @@ public void beginQuotation(Map<String, String> parameters)
++this.quotationDepth;
super.beginQuotation(parameters);
this.eventStack.push(Event.QUOTATION);
}
@Override
......@@ -353,6 +425,8 @@ public void beginQuotationLine()
++this.quotationLineIndex;
super.beginQuotationLine();
this.eventStack.push(Event.QUOTATION_LINE);
}
@Override
......@@ -362,6 +436,8 @@ public void beginHeader(HeaderLevel level, String id, Map<String, String> parame
++this.inlineDepth;
super.beginHeader(level, id, parameters);
this.eventStack.push(Event.HEADER);
}
@Override
......@@ -370,6 +446,8 @@ public void beginTable(Map<String, String> parameters)
this.isInTable = true;
super.beginTable(parameters);
this.eventStack.push(Event.TABLE);
}
@Override
......@@ -378,6 +456,8 @@ public void beginTableRow(Map<String, String> parameters)
++this.cellRow;
super.beginTableRow(parameters);
this.eventStack.push(Event.TABLE_ROW);
}
@Override
......@@ -388,6 +468,8 @@ public void beginTableCell(Map<String, String> parameters)
++this.cellCol;
super.beginTableCell(parameters);
this.eventStack.push(Event.TABLE_CELL);
}
@Override
......@@ -398,11 +480,28 @@ public void beginTableHeadCell(Map<String, String> parameters)
++this.cellCol;
super.beginTableHeadCell(parameters);
this.eventStack.push(Event.TABLE_HEAD_CELL);
}
/**
* Removes an event from the stack if it matches the passed event.
*
* @param event The event to remove.
* @since 14.0RC1
*/
private void removeEventFromStack(Event event)
{
if (this.eventStack.peek() == event) {
this.eventStack.pop();
}
}
@Override
public void endDefinitionDescription()
{
removeEventFromStack(Event.DEFINITION_DESCRIPTION);
super.endDefinitionDescription();
--this.inlineDepth;
......@@ -417,6 +516,8 @@ public void endDefinitionDescription()
@Override
public void endDefinitionList(Map<String, String> parameters)
{
removeEventFromStack(Event.DEFINITION_LIST);
super.endDefinitionList(parameters);
this.definitionListDepth.pop();
......@@ -427,6 +528,8 @@ public void endDefinitionList(Map<String, String> parameters)
@Override
public void endDefinitionTerm()
{
removeEventFromStack(Event.DEFINITION_TERM);
super.endDefinitionTerm();
--this.inlineDepth;
......@@ -441,14 +544,31 @@ public void endDefinitionTerm()
@Override
public void endDocument(MetaData metadata)
{
removeEventFromStack(Event.DOCUMENT);
super.endDocument(metadata);
this.previousEvent = Event.DOCUMENT;
}
/**
* {@inheritDoc}
*
* @since 14.0RC1
*/
@Override
public void beginFormat(Format format, Map<String, String> parameters)
{
super.beginFormat(format, parameters);
this.eventStack.push(Event.FORMAT);
}
@Override
public void endFormat(Format format, Map<String, String> parameters)
{
removeEventFromStack(Event.FORMAT);
super.endFormat(format, parameters);
this.previousEvent = Event.FORMAT;
......@@ -462,6 +582,8 @@ public void endFormat(Format format, Map<String, String> parameters)
@Override
public void endLink(ResourceReference reference, boolean freestanding, Map<String, String> parameters)
{
removeEventFromStack(Event.LINK);
super.endLink(reference, freestanding, parameters);
--this.linkDepth;
......@@ -471,6 +593,8 @@ public void endLink(ResourceReference reference, boolean freestanding, Map<Strin
@Override
public void endList(ListType type, Map<String, String> parameters)
{
removeEventFromStack(Event.LIST);
super.endList(type, parameters);
this.listDepth.pop();
......@@ -481,6 +605,8 @@ public void endList(ListType type, Map<String, String> parameters)
@Override
public void endListItem()
{
removeEventFromStack(Event.LIST_ITEM);
super.endListItem();
--this.inlineDepth;
......@@ -490,6 +616,8 @@ public void endListItem()
@Override
public void endListItem(Map<String, String> parameters)
{
removeEventFromStack(Event.LIST_ITEM);
super.endListItem(parameters);
--this.inlineDepth;
......@@ -499,6 +627,8 @@ public void endListItem(Map<String, String> parameters)
@Override
public void endMacroMarker(String name, Map<String, String> parameters, String content, boolean isInline)
{
removeEventFromStack(Event.MACRO_MARKER);
super.endMacroMarker(name, parameters, content, isInline);
this.previousEvent = Event.MACRO_MARKER;
......@@ -513,6 +643,8 @@ public void endMacroMarker(String name, Map<String, String> parameters, String c
@Override
public void endMetaData(MetaData metadata)
{
removeEventFromStack(Event.META_DATA);
super.endMetaData(metadata);
this.previousEvent = Event.META_DATA;
......@@ -526,6 +658,8 @@ public void endMetaData(MetaData metadata)
@Override
public void endGroup(Map<String, String> parameters)
{
removeEventFromStack(Event.GROUP);
super.endGroup(parameters);
this.previousEvent = Event.GROUP;
......@@ -534,6 +668,8 @@ public void endGroup(Map<String, String> parameters)
@Override
public void endParagraph(Map<String, String> parameters)
{
removeEventFromStack(Event.PARAGRAPH);
super.endParagraph(parameters);
this.isInParagraph = false;
......@@ -544,6 +680,8 @@ public void endParagraph(Map<String, String> parameters)
@Override
public void endQuotation(Map<String, String> parameters)
{
removeEventFromStack(Event.QUOTATION);
super.endQuotation(parameters);
--this.quotationDepth;
......@@ -556,6 +694,8 @@ public void endQuotation(Map<String, String> parameters)
@Override
public void endQuotationLine()
{
removeEventFromStack(Event.QUOTATION_LINE);
super.endQuotationLine();
--this.quotationLineDepth;
......@@ -563,9 +703,24 @@ public void endQuotationLine()
this.previousEvent = Event.QUOTATION_LINE;
}
/**
* {@inheritDoc}
*
* @since 14.0RC1
*/
@Override
public void beginSection(Map<String, String> parameters)
{
super.beginSection(parameters);
this.eventStack.push(Event.SECTION);
}
@Override
public void endSection(Map<String, String> parameters)
{
removeEventFromStack(Event.SECTION);
super.endSection(parameters);
this.previousEvent = Event.SECTION;
......@@ -574,6 +729,8 @@ public void endSection(Map<String, String> parameters)
@Override
public void endHeader(HeaderLevel level, String id, Map<String, String> parameters)
{
removeEventFromStack(Event.HEADER);
super.endHeader(level, id, parameters);
this.isInHeader = false;
......@@ -584,6 +741,8 @@ public void endHeader(HeaderLevel level, String id, Map<String, String> paramete
@Override
public void endTable(Map<String, String> parameters)
{
removeEventFromStack(Event.TABLE);
super.endTable(parameters);
this.isInTable = false;
......@@ -594,6 +753,8 @@ public void endTable(Map<String, String> parameters)
@Override
public void endTableCell(Map<String, String> parameters)
{
removeEventFromStack(Event.TABLE_CELL);
super.endTableCell(parameters);
this.isInTableCell = false;
......@@ -604,6 +765,8 @@ public void endTableCell(Map<String, String> parameters)
@Override
public void endTableHeadCell(Map<String, String> parameters)
{
removeEventFromStack(Event.TABLE_HEAD_CELL);
super.endTableHeadCell(parameters);
this.isInTableCell = false;
......@@ -614,15 +777,32 @@ public void endTableHeadCell(Map<String, String> parameters)
@Override
public void endTableRow(Map<String, String> parameters)
{
removeEventFromStack(Event.TABLE_ROW);
super.endTableRow(parameters);
this.previousEvent = Event.TABLE_ROW;
this.cellCol = -1;
}
/**
* {@inheritDoc}
*
* @since 14.0RC1
*/
@Override
public void beginFigure(Map<String, String> parameters)
{
super.beginFigure(parameters);
this.eventStack.push(Event.FIGURE);
}
@Override
public void endFigure(Map<String, String> parameters)
{
removeEventFromStack(Event.FIGURE);
super.endFigure(parameters);
this.previousEvent = Event.FIGURE;
......@@ -634,11 +814,15 @@ public void beginFigureCaption(Map<String, String> parameters)
++this.inlineDepth;
super.beginFigureCaption(parameters);
this.eventStack.push(Event.FIGURE_CAPTION);
}
@Override
public void endFigureCaption(Map<String, String> parameters)
{
removeEventFromStack(Event.FIGURE_CAPTION);
super.endFigureCaption(parameters);
--this.inlineDepth;
......
......@@ -37,6 +37,8 @@
import org.xwiki.rendering.listener.MetaData;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.fail;
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.mock;
......@@ -65,8 +67,9 @@ void setUpChain()
}
/**
* Tests for all "begin/end"-methods if the previous event is correctly set, but only after the end event has been
* forwarded in the chain.
* Tests for all "begin/end"-methods if they do not modify the parent event (for the next in the chain) in the
* begin-method, correctly set it afterwards and if the previous event is correctly set, but only after the end
* event has been forwarded in the chain.
*/
@TestFactory
Stream<DynamicTest> beginEndMethods()
......@@ -79,8 +82,8 @@ Stream<DynamicTest> beginEndMethods()
}
/**
* Tests for all "on..." methods if the previous event is correctly set, but only after the event has been forwarded
* in the chain.
* Tests for all "on..." methods if they do not modify the parent event (for the next in the chain) and if the
* previous event is correctly set, but only after the event has been forwarded in the chain.
*/
@TestFactory
Stream<DynamicTest> onMethods()
......@@ -111,10 +114,16 @@ private void testBeginEndMethod(Method beginMethod)
beginMethod.getName().equals("beginDefinitionTerm") || beginMethod.getName().equals(
"beginDefinitionDescription");
BlockStateChainingListener.Event expectedParentEvent;
if (isListItem) {
this.listener.beginList(ListType.NUMBERED, Listener.EMPTY_PARAMETERS);
expectedParentEvent = BlockStateChainingListener.Event.LIST;
} else if (isDefinitionItem) {
this.listener.beginDefinitionList(Listener.EMPTY_PARAMETERS);
expectedParentEvent = BlockStateChainingListener.Event.DEFINITION_LIST;
} else {
expectedParentEvent = null;
}
Object[] parameters = Arrays.stream(parameterClasses).map(this::mockParameter).toArray();
......@@ -123,10 +132,11 @@ private void testBeginEndMethod(Method beginMethod)
Stubber verifyPreviousAndParentEventStubber = doAnswer(invocation -> {
assertEquals(BlockStateChainingListener.Event.ID, this.listener.getPreviousEvent());
assertEquals(expectedParentEvent, this.listener.getParentEvent());
return null;
}).doNothing();
// Assert that in the begin method, the previous event are unchanged
// Assert that in the begin method, the parent and the previous event are unchanged.
beginMethod.invoke(verifyPreviousAndParentEventStubber.when(this.mockListener), parameters);
// Actually call the begin method.
......@@ -135,7 +145,14 @@ private void testBeginEndMethod(Method beginMethod)
// Verify the mock listener in the chain has been called.
beginMethod.invoke(verify(this.mockListener), parameters);
// Assert that in the end method, the previous event is unchanged
BlockStateChainingListener.Event parentEvent = this.listener.getParentEvent();
assertNotNull(parentEvent, "No parent set after calling " + beginMethod.getName());
String parentEventName = parentEvent.name();
String eventNameCamelCase = CaseUtils.toCamelCase(parentEventName, true, '_');
assertEquals(beginMethod.getName(), "begin" + eventNameCamelCase,
"Wrong event " + parentEventName + " generated for " + beginMethod.getName());
// Assert that in the end method, the parent has been restored and the previous event is unchanged.
endMethod.invoke(verifyPreviousAndParentEventStubber.when(this.mockListener), parameters);
// Actually call the end method.
......@@ -145,16 +162,15 @@ private void testBeginEndMethod(Method beginMethod)
endMethod.invoke(verify(this.mockListener), parameters);
// Verify that the previous event has been set to the event corresponding to the current methods.
String previousEventName = this.listener.getPreviousEvent().name();
String previousEventCamelCase = CaseUtils.toCamelCase(previousEventName, true, '_');
assertEquals(beginMethod.getName(), "begin" + previousEventCamelCase,
"Wrong event " + previousEventName + " generated for " + beginMethod.getName());
assertEquals(parentEvent, this.listener.getPreviousEvent());
if (isDefinitionItem) {
this.listener.endDefinitionList(Listener.EMPTY_PARAMETERS);
} else if (isListItem) {
this.listener.endList(ListType.NUMBERED, Listener.EMPTY_PARAMETERS);
}
assertNull(this.listener.getParentEvent());
} catch (NoSuchMethodException e) {
fail("Expected end method " + endMethodName + " for " + beginMethod.getName() + " not found: "
+ e.getMessage());
......@@ -176,9 +192,11 @@ private void testOnMethod(Method method)
Object[] parameters = Arrays.stream(method.getParameterTypes()).map(this::mockParameter).toArray();
try {
// Verify that the next in the chain still gets the old previous event.
// Verify that the next in the chain still gets the old previous event and that the parent is not
// changed.
method.invoke(
doAnswer(invocationOnMock -> {
assertEquals(BlockStateChainingListener.Event.DOCUMENT, this.listener.getParentEvent());
assertEquals(BlockStateChainingListener.Event.PARAGRAPH, this.listener.getPreviousEvent());
return null;
})
......
......@@ -24,6 +24,7 @@
import org.xwiki.rendering.internal.renderer.xhtml.XHTMLMacroRenderer;
import org.xwiki.rendering.internal.renderer.xhtml.image.XHTMLImageRenderer;
import org.xwiki.rendering.internal.renderer.xhtml.link.XHTMLLinkRenderer;
import org.xwiki.rendering.listener.chaining.BlockStateChainingListener;