Refactor RenderMathMLOperator and RenderMathMLToken to avoid using anonymous renderers

• Component: MathML
• Status: RESOLVED FIXED

[Frédéric Wang (:fredw)]

This is another commit from our MathMLLayout branch:

1) Thanks to bug 152244, we can use StretchyOperator for RenderMathMLOperator to avoid (re)creating anonymous text nodes. 
2) As suggested in bug 108778 comment 10.

[Frédéric Wang (:fredw)]

Created attachment 273290 [details]
Patch

[Frédéric Wang (:fredw)]

Created attachment 273418 [details]
Patch

A slightly better version where I implement layoutBlock and remove computeLogicalHeight so that bug 153991 will be easier. I also had forgotten to layout children when we do "special" painting ; fixing that changes some text expectations.

[Frédéric Wang (:fredw)]

Comment on attachment 273418 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=273418&action=review

> Source/WebCore/css/mathml.css:43
>  

Note: it also makes sense to remove the following rule in this patch. It does not seem to have any effect (see bug 151592).

mtext {
    line-height: 1.0;
}

[Frédéric Wang (:fredw)]

Created attachment 273706 [details]
Patch

I'm uploading updated version of the MathML refactoring patches to solve conflicts after bug 154388 and bug 155005 ; and to do other minor changes.

[Frédéric Wang (:fredw)]

Created attachment 275289 [details]
Patch

[Joanmarie Diggs]

In terms of the accessibility-related changes (tests only), the one-pixel changes reported for size and position are fine. And it looks like we're now exposing the operator text in the same way we expose the other element text. Arguably, that's a bug fix. :) So a11y-wize, lgtm.

[Alejandro G. Castro]

Comment on attachment 275289 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=275289&action=review

The cleaning looks great, I'll let other reviewer to check it.

> LayoutTests/ChangeLog:10
> +        * platform/mac/TestExpectations: Disable some tests for now. Some just fail because of small rendering differences. The accessibility test for mfenced must be updated to take into account changes in the render tree.

Why don't we update the ones that fail because of small rendering differences?

> LayoutTests/platform/mac/TestExpectations:818
> +# FIXME: Small differences.
> +mathml/presentation/attributes-mathvariant.html [ ImageOnlyFailure ]
> +mathml/presentation/fenced-mi.html [ ImageOnlyFailure ]
> +mathml/presentation/fenced.html [ ImageOnlyFailure ]
> +mathml/presentation/tokenElements-mathvariant.html [ ImageOnlyFailure ]

Shall we update the images? Or you think we are adding a bug? In that case we should add a bug for it and add it here with the explanation.

> Source/WebCore/ChangeLog:11
> +        For anonymous RenderMathMLOperator renderers created by RenderMathMLFenced, we have to paint the character by ourselves ;

Nit, there is a space before the ;

> Source/WebCore/ChangeLog:29
> +        (WebCore::RenderMathMLOperator::specialPainting): New function to determine what will be painted using StretchyOperator.

specialPainting means stretched? Not sure if the name is very clear.

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:279
> +        for (RenderBox* child = firstChildBox(); child; child = child->nextSiblingBox())
> +            child->layoutIfNeeded();

What children an Operator can have when the anonymous are there, the text? Is it just one child?

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:287
> +        // We first do the normal layout without spacing.
> +        recomputeLogicalWidth();
> +        setLogicalWidth(width() - m_leadingSpace - m_trailingSpace);
> +        RenderMathMLToken::layoutBlock(relayoutChildren, pageLogicalHeight);
> +        recomputeLogicalWidth();

This does not look correct, what are you trying to do calling recomputeLogicalWidth twice, it actually returns a bool to check if the width was actually recomputed. Also setting the logicalwidth and later calling the layoutBlock of the parent looks weird to me.

> Source/WebCore/rendering/mathml/RenderMathMLOperator.h:64
> -    void updateTokenContent() final;
> +    void updateTokenContent() override;

Do you need to override in this patch?

> Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:159
> +    ASSERT(needsLayout());

Why we do not have this in the first line of the function?

> Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:165
> +    for (RenderBox* child = firstChildBox(); child; child = child->nextSiblingBox())
> +        child->layoutIfNeeded();

Is this too general, in what situations a token could have more than one child?

> Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:175
> +    RenderMathMLBlock::paint(info, paintOffset);

Is this required in case isValid and we paint the glyph?

[Frédéric Wang (:fredw)]

Created attachment 277376 [details]
Patch

[Frédéric Wang (:fredw)]

Created attachment 278626 [details]
Testing a small subset in EWS

[Frédéric Wang (:fredw)]

Comment on attachment 275289 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=275289&action=review

>> LayoutTests/ChangeLog:10
>> +        * platform/mac/TestExpectations: Disable some tests for now. Some just fail because of small rendering differences. The accessibility test for mfenced must be updated to take into account changes in the render tree.
> 
> Why don't we update the ones that fail because of small rendering differences?

See below.

>> LayoutTests/platform/mac/TestExpectations:818
>> +mathml/presentation/tokenElements-mathvariant.html [ ImageOnlyFailure ]
> 
> Shall we update the images? Or you think we are adding a bug? In that case we should add a bug for it and add it here with the explanation.

These are reftests not pixel tests, so that's not easy to just update. The problem is the following: for some math text (like anonymous operator created by mfenced or italic mi and more generally mathvariant char) we will for use our own layout instead of the standard text layout because we need special painting. This works well on GTK but on other platforms you have small shifts. The problem is probably the mix between int and LayoutUnit (see bug 155879). I've been able to fixed the case of mfenced operators by correct int rounding of firstBaseLine. I'll see if I can do something for the non-mo token elements tomorrow.

>> Source/WebCore/ChangeLog:29
>> +        (WebCore::RenderMathMLOperator::specialPainting): New function to determine what will be painted using StretchyOperator.
> 
> specialPainting means stretched? Not sure if the name is very clear.

I renamed it to useMathOperator and moved that to bug 157519.

>> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:279
>> +            child->layoutIfNeeded();
> 
> What children an Operator can have when the anonymous are there, the text? Is it just one child?

I think only one anonymous block created by the CSS rules (maybe 0 too), but it does not hurt to have a general loop I guess.

>> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:287
>> +        recomputeLogicalWidth();
> 
> This does not look correct, what are you trying to do calling recomputeLogicalWidth twice, it actually returns a bool to check if the width was actually recomputed. Also setting the logicalwidth and later calling the layoutBlock of the parent looks weird to me.

I need to check that and see if I can make this less hacky.

>> Source/WebCore/rendering/mathml/RenderMathMLOperator.h:64
>> +    void updateTokenContent() override;
> 
> Do you need to override in this patch?

I removed that. Probably I just mixed things up when trying to update all the MathML patches to move to the new override/final/virtual conventions.

>> Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:159
>> +    ASSERT(needsLayout());
> 
> Why we do not have this in the first line of the function?

I moved that to the top for consistency with other functions. (This assert is probably in RenderMathMLBlock::layoutBlock too.)

>> Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:165
>> +        child->layoutIfNeeded();
> 
> Is this too general, in what situations a token could have more than one child?

Same here, I think it's fine to have a loop even it there is probably only one anonymous block container.

>> Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:175
>> +    RenderMathMLBlock::paint(info, paintOffset);
> 
> Is this required in case isValid and we paint the glyph?

I think RenderMathMLBlock has some compile flag to paint the MathML frame (not sure if that's really needed though).

[Frédéric Wang (:fredw)]

Created attachment 278630 [details]
Patch

Here is an updated version. Some parts have been moved to bug 157519, bug 157521 and bug 157568, so the code change will be smaller here and hopefully easier to review. I still need to check again Alex's comments and the tests on iOS/Mac.

[Frédéric Wang (:fredw)]

Created attachment 278706 [details]
Patch for EWS testing

[Build Bot]

Comment on attachment 278706 [details]
Patch for EWS testing

Attachment 278706 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1308126

New failing tests:
accessibility/math-text.html
imported/w3c/web-platform-tests/html/dom/documents/dom-tree-accessors/document.getElementsByName/document.getElementsByName-namespace.html
imported/w3c/web-platform-tests/html/dom/documents/dom-tree-accessors/document.getElementsByName/document.getElementsByName-namespace-xhtml.xhtml
fast/css/readwrite-pseudoclass-editable.html
fast/css/readonly-pseudoclass-common-element.html
accessibility/mac/mathml-elements.html

[Build Bot]

Created attachment 278709 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 278706 [details]
Patch for EWS testing

Attachment 278706 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1308134

New failing tests:
accessibility/ios-simulator/math.html
fast/css/readwrite-pseudoclass-editable.html
imported/w3c/web-platform-tests/html/dom/documents/dom-tree-accessors/document.getElementsByName/document.getElementsByName-namespace.html
imported/w3c/web-platform-tests/html/dom/documents/dom-tree-accessors/document.getElementsByName/document.getElementsByName-namespace-xhtml.xhtml
fast/css/readonly-pseudoclass-common-element.html

[Build Bot]

Created attachment 278710 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

[Frédéric Wang (:fredw)]

Created attachment 278711 [details]
Patch for EWS testing

[Frédéric Wang (:fredw)]

Created attachment 278715 [details]
Patch

[Build Bot]

Comment on attachment 278711 [details]
Patch for EWS testing

Attachment 278711 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1308390

New failing tests:
fast/css/readwrite-pseudoclass-editable.html
imported/w3c/web-platform-tests/html/dom/documents/dom-tree-accessors/document.getElementsByName/document.getElementsByName-namespace.html
imported/w3c/web-platform-tests/html/dom/documents/dom-tree-accessors/document.getElementsByName/document.getElementsByName-namespace-xhtml.xhtml
fast/css/readonly-pseudoclass-common-element.html

[Build Bot]

Created attachment 278717 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 278711 [details]
Patch for EWS testing

Attachment 278711 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1308370

New failing tests:
fast/css/readwrite-pseudoclass-editable.html
imported/w3c/web-platform-tests/html/dom/documents/dom-tree-accessors/document.getElementsByName/document.getElementsByName-namespace.html
imported/w3c/web-platform-tests/html/dom/documents/dom-tree-accessors/document.getElementsByName/document.getElementsByName-namespace-xhtml.xhtml
fast/css/readonly-pseudoclass-common-element.html

[Build Bot]

Created attachment 278718 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 278711 [details]
Patch for EWS testing

Attachment 278711 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1308418

New failing tests:
fast/css/readwrite-pseudoclass-editable.html
imported/w3c/web-platform-tests/html/dom/documents/dom-tree-accessors/document.getElementsByName/document.getElementsByName-namespace.html
imported/w3c/web-platform-tests/html/dom/documents/dom-tree-accessors/document.getElementsByName/document.getElementsByName-namespace-xhtml.xhtml
fast/css/readonly-pseudoclass-common-element.html

[Build Bot]

Created attachment 278720 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

[Frédéric Wang (:fredw)]

Created attachment 278721 [details]
Patch

[Frédéric Wang (:fredw)]

Comment on attachment 278721 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=278721&action=review

> Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:187
> +    IntPoint adjustedPaintOffset = roundedIntPoint(paintOffset + location() + LayoutPoint(0, glyphAscent));

So after more testing in bug 108778 on Mac/iOS, it seems that we should only round the vertical coordinate (glyphAscent) not the horizontal one or otherwise one mathvariant reftest does not work.

[Frédéric Wang (:fredw)]

Created attachment 279116 [details]
Patch

[Frédéric Wang (:fredw)]

Created attachment 281732 [details]
Patch

[Frédéric Wang (:fredw)]

Created attachment 281743 [details]
Patch

[Frédéric Wang (:fredw)]

Created attachment 281821 [details]
Patch

Here is an updated patch to run EWS, but I'll try and move some fenced accessibility code into a separate patch.

[Frédéric Wang (:fredw)]

Created attachment 281828 [details]
Patch

This version applies on top of the patch for bug 139582.

[Frédéric Wang (:fredw)]

Created attachment 281847 [details]
Patch

After discussion with joanie, it probably makes more sense to make the accessibility changes independent from this layout refactoring. So I'm uploading a new patch that goes back to the idea of temporarily disabling the fenced accessibility test on mac. The code to make that test work again and to fix a similar bug on ATK/Linux is essentially ready on bug 139582, but the way to organize the patches and review has to be sorted out with accessibility people.

[Frédéric Wang (:fredw)]

Created attachment 281972 [details]
Patch

[Martin Robinson]

Comment on attachment 281972 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=281972&action=review

> LayoutTests/accessibility/mac/mathml-elements.html:114
> +   /*
> +   FIXME: AccessibilityRenderObject should be updated to properly expose the text of mfenced operators.
> +   See https://bugs.webkit.org/show_bug.cgi?id=139582.
>     var fenceValues = new Array("{", "2", ",", "a", ",", "e", "}");

Instead of commenting out this code you should remove it.

> Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:115
> +    // Early return if the token element contains RendererElement's.

Do you mean RenderElements here?

[Frédéric Wang (:fredw)]

Committed r202420: <http://trac.webkit.org/changeset/202420>

[Chris Dumez]

Comment on attachment 281972 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=281972&action=review

> Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:167
> +    setLogicalWidth(m_mathVariantGlyph.font->widthForGlyph(m_mathVariantGlyph.glyph));

What guarantees m_mathVariantGlyph.font is non-null and alive? I suspect it could be because we see crashes like these in the wild:
Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Subtype: KERN_INVALID_ADDRESS at 0x000000000000074f
Triggered by Thread:  0

Filtered syslog:
None found
Thread 0 name:  Dispatch queue: com.apple.main-thread
Thread 0 Crashed ↩:
0   WebCore                       	0x0000000190d67970 WTF::HashTableAddResult<WTF::HashTableIterator<int, WTF::KeyValuePair<int, std::__1::unique_ptr<WebCore::GlyphMetricsMap<float>::GlyphMetricsPage, std::__1::default_delete<WebCore::GlyphMetricsMap<float>::GlyphMetricsPage> > >, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<int, std::__1::unique_ptr<WebCore::GlyphMetricsMap<float>::GlyphMetricsPage, std::__1::default_delete<WebCore::GlyphMetricsMap<float>::GlyphMetricsPage> > > >, WTF::IntHash<unsigned int>, WTF::HashMap<int, std::__1::unique_ptr<WebCore::GlyphMetricsMap<float>::GlyphMetricsPage, std::__1::default_delete<WebCore::GlyphMetricsMap<float>::GlyphMetricsPage> >, WTF::IntHash<unsigned int>, WTF::HashTraits<int>, WTF::HashTraits<std::__1::unique_ptr<WebCore::GlyphMetricsMap<float>::GlyphMetricsPage, std::__1::default_delete<WebCore::GlyphMetricsMap<float>::GlyphMetricsPage> > > >::KeyValuePairTraits, WTF::HashTraits<int> > > WTF::HashMap<int, std::__1::unique_ptr<WebCore::GlyphMetricsMap<float>::GlyphMetricsPage, std::__1::default_delete<WebCore::GlyphMetricsMap<float>::GlyphMetricsPage> >, WTF::IntHash<unsigned int>, WTF::HashTraits<int>, WTF::HashTraits<std::__1::unique_ptr<WebCore::GlyphMetricsMap<float>::GlyphMetricsPage, std::__1::default_delete<WebCore::GlyphMetricsMap<float>::GlyphMetricsPage> > > >::add<std::nullptr_t>(int&&, std::nullptr_t&&) + 28 (HashTable.h:869)
1   WebCore                       	0x0000000190970634 WebCore::GlyphMetricsMap<float>::locatePageSlowCase(unsigned int) + 176 (GlyphMetricsMap.h:120)
2   WebCore                       	0x0000000190970634 WebCore::GlyphMetricsMap<float>::locatePageSlowCase(unsigned int) + 176 (GlyphMetricsMap.h:120)
3   WebCore                       	0x00000001914cfb3c WebCore::RenderMathMLToken::layoutBlock(bool, WebCore::LayoutUnit) + 240 (GlyphMetricsMap.h:87)
4   WebCore                       	0x00000001908de104 WebCore::RenderBlock::layout() + 72 (RenderBlock.cpp:1075)
5   WebCore                       	0x00000001914cc320 WebCore::RenderMathMLRow::computeLineVerticalStretch(WebCore::LayoutUnit&, WebCore::LayoutUnit&) + 152 (RenderElement.h:133)
6   WebCore                       	0x00000001914ccca0 WebCore::RenderMathMLRow::layoutBlock(bool, WebCore::LayoutUnit) + 64 (RenderMathMLRow.cpp:190)
7   WebCore                       	0x00000001908de104 WebCore::RenderBlock::layout() + 72 (RenderBlock.cpp:1075)

[Frédéric Wang (:fredw)]

(In reply to comment #36)
> Comment on attachment 281972 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=281972&action=review
> 
> > Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:167
> > +    setLogicalWidth(m_mathVariantGlyph.font->widthForGlyph(m_mathVariantGlyph.glyph));
> 
> What guarantees m_mathVariantGlyph.font is non-null and alive? I suspect it

There are isValid() calls everywhere in the MathML code. However, they do "glyph || font" rather than "glyph && font" as I expected...

[Frédéric Wang (:fredw)]

I see in bug 150171 comment 5 that the initial intent was GlyphData::isEmpty(). For me GlyphData::isValid() means having a glyph id and a font.

@Myles, Carlos: is there any reason to call a GlyphData "valid"  without a font?

[Frédéric Wang (:fredw)]

I opened bug 159783 and will upload a patch tomorrow.