Bug 153991 – Refactor layout functions to avoid using flexbox in MathML

• Status: RESOLVED
• Resolution: FIXED
• Component: MathML

[Alejandro G. Castro]

This is the last patch in the series, it removes the children traversing of every renderer and adds the code to the RenderMathMLBlock to handle the render tree like it was doing the flex in these cases.

[Alejandro G. Castro]

Created attachment 270869 [details]
Patch

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

Comment on attachment 270869 [details]
Patch

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

> Source/WebCore/css/mathml.css:35
> +ms, mspace, mtext, mi, mn, mo, mrow, mfenced, mfrac, msub, msup, msubsup, mmultiscripts, mprescripts, none, munder, mover, munderover, msqrt, mroot, merror, mphantom, mstyle, menclose, semantics mpadded {

A comma is missing semantics and mpadded

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

Comment on attachment 270869 [details]
Patch

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

> LayoutTests/TestExpectations:860
> +mathml/presentation/foreign-mi-dynamic.html [ Skip ]

So I verified that one. In the test, you have some <mi>x</mi> and try to append some <span> inside it. MathMLTextElement::childrenChanged is called, but the renderer for the <span> does not seem to be attached yet when RenderMathMLToken::updateStyle is called and so WebKit still thinks it has to use italic. Moreover, MathMLTextElement::didAttachRenderers is not called and so italic style is not removed either ; maybe this is because the children of the token element are wrapped inside an anonymous RenderBlock. 

I've fixed that on the MathMLLayout-bis branch by delaying the determination of when mathvariant=italic applies as well as the selection of the italic glyph to use. I'm not sure that can easily be done without the refactoring of RenderMathMLToken to remove anonymous node/style, so indeed it seems sensible to skip it at this point.

[Alejandro G. Castro]

(In reply to comment #2)
> Comment on attachment 270869 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270869&action=review
> 
> > Source/WebCore/css/mathml.css:35
> > +ms, mspace, mtext, mi, mn, mo, mrow, mfenced, mfrac, msub, msup, msubsup, mmultiscripts, mprescripts, none, munder, mover, munderover, msqrt, mroot, merror, mphantom, mstyle, menclose, semantics mpadded {
> 
> A comma is missing semantics and mpadded

Thanks, fixed, I'm going to upload again.

[Alejandro G. Castro]

Created attachment 271166 [details]
Patch

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

Comment on attachment 271166 [details]
Patch

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

> Source/WebCore/css/mathml.css:35
> +ms, mspace, mtext, mi, mn, mo, mrow, mfenced, mfrac, msub, msup, msubsup, mmultiscripts, mprescripts, none, munder, mover, munderover, msqrt, mroot, merror, mphantom, mstyle, menclose, semantics, mpadded {

For the record: maction is missing here (in our MathMLLayout branch we did it in our RenderMathMLRow refactoring, but it's more relevant to integrate it in this patch).

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

Created attachment 273424 [details]
Patch (move paintChildren functions)

This is a simple patch to move duplicate paintChildren code into RenderMathMLBlock.

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

Created attachment 273425 [details]
Patch (flexbox to block)

A work-in-progress patch to replace flexbox with block. I have not extracted all the changes from the initial patch, so this does not work at the moment. However, I suspect we won't need all the flexbox-specific rules.

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

Created attachment 274089 [details]
Patch (move paintChildren functions)

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

Created attachment 274092 [details]
Patch (initial attempt, duplicating RenderFlexibleBox logic)

This is an update of Alex's patch (attachment 271166 [details]) to make it apply at the end of the refactoring ; using some parts of attachment 271162 [details]. This also adds new mac expectations.

At the moment, it still adds a lot of flexbox-like code, but we can probably simplify this...

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

Created attachment 274103 [details]
Patch

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

Created attachment 275294 [details]
Patch

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

Created attachment 277377 [details]
Patch

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

Created attachment 278726 [details]
Patch

Updating test expectations...

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

Comment on attachment 278726 [details]
Patch

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

> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:347
> +        LayoutUnit childMainExtent = child->width();

Here and in other places, this should probably be repaced with logicalWidth() / logicalHeight()

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

Created attachment 281973 [details]
Patch

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

Created attachment 281989 [details]
Patch

[Build Bot]

Comment on attachment 281989 [details]
Patch

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

New failing tests:
accessibility/mac/mathml-elements.html
imported/blink/fast/text/output-isolate-at-end-of-line-crash.html

[Build Bot]

Created attachment 281997 [details]
Archive of layout-test-results from ews100 for mac-yosemite

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

[Build Bot]

Comment on attachment 281989 [details]
Patch

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

New failing tests:
imported/blink/fast/text/output-isolate-at-end-of-line-crash.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 281989 [details]
Patch

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

New failing tests:
accessibility/mac/mathml-elements.html
imported/blink/fast/text/output-isolate-at-end-of-line-crash.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 281989 [details]
Patch

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

New failing tests:
accessibility/mac/mathml-elements.html
imported/blink/fast/text/output-isolate-at-end-of-line-crash.html

[Build Bot]

Created attachment 282000 [details]
Archive of layout-test-results from ews112 for mac-yosemite

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

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

Created attachment 282048 [details]
Patch

Updating two tests to take into account removal of line breaks that were due to flexboxes.

[WebKit Commit Bot]

Attachment 282048 [details] did not pass style-queue:


ERROR: LayoutTests/ChangeLog:25:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

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

Created attachment 282050 [details]
Patch

[Alejandro G. Castro]

Comment on attachment 282050 [details]
Patch

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

Awesome, we are so close :), some comments inlined.

> Source/WebCore/ChangeLog:12
> +        (math): Change display of inline expressions from inline-flex to inline.
> +        Change display of inline expressions from inline-flex to inline

This comment is repeated.

> Source/WebCore/ChangeLog:13
> +        (math[display="block"]): Change display of display expression from flex to block

Maybe: Change display expression from flex to block ...

> Source/WebCore/ChangeLog:15
> +        (ms, mspace, mtext, mi, mn, mo, mrow, mfenced, mfrac, msub, msup, msubsup, mmultiscripts, mprescripts, none, munder, mover, munderover, msqrt, mroot, merror, mphantom, mstyle, menclose, semantics, mpadded, maction):  Add more MathML elements and change display from inline-flex to block.

Try wrapping the line. The other changes are more direct but in this case it is a good idea to explain why we go from inline-flex to block, now everything is a block IIRC.

> Source/WebCore/ChangeLog:18
> +        (ms, mi, mo, mrow, mfenced, mfrac, msub, msup, msubsup, mmultiscripts, mprescripts, none, munder, mover, munderover, msqrt, mroot, merror, mphantom, mstyle, menclose): Deleted.

This is not correct, this is the line modified before.

> Source/WebCore/ChangeLog:32
> +        (WebCore::RenderMathMLBlock::layoutItems): Implement a simplified version of
> +        RenderFlexibleBox::layoutItems where we assume horizontal layout for all children.

This is not a simplification anymore, we have removed the anonymous, try to explain what options we have for children here and how we solve it.

> Source/WebCore/ChangeLog:34
> +        (WebCore::RenderMathMLBlock::layoutBlock): Add a basic implementation based on
> +        RenderFlexibleBox::layoutBlock.

Ditto.

> Source/WebCore/rendering/RenderBox.cpp:2444
> +        && !isFloating() && !isInline() && !cb.isFlexibleBoxIncludingDeprecated() && !cb.isRenderMathMLBlock()

Probably someone with good understanding of RendeBox should give a thumbs up to this change and the next one. We also should use the compilation define: ENABLE(MATHML). And are we still using the overrides? These changes were don because we needed the override with the anonymous nodes, now that we have remove them where are we using the overrides and could we remove them?

> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:314
> +    LayoutUnit crossAxisOffset = borderBefore() + paddingBefore();
> +    LayoutUnit mainAxisOffset = borderStart() + paddingStart();

I would avoid cross and main names, we have now just horizontal and vertical. Those are flexbox concepts.

> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:317
> +    for (auto* child = firstChildBox(); child; child = child->nextSiblingBox()) {

Now that we do not have anonymous in which situation we have multiple children? We have to check the children we can have and review the loops.

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

> This is not a simplification anymore, we have removed the anonymous, try to explain what options we have for children here and how we solve it.

> Now that we do not have anonymous in which situation we have multiple children? We have to check the children we can have and review the loops.

Strictly speaking, this is still a simplification of the generic flexbox implementation, even if I've been able to removed much more code than the original patch.

From a quick look to the MathML element classes, it seems that we are only using RenderMathMLBlock::layoutBlock for token elements (for which there are probably 0 or 1 child) and other empty elements like <none/>, <mprescripts/>. So we could certainly do a simplified implementation in RenderTokenElement::layoutBlock for the former and in RenderMathMLBlock::layoutBlock for the latter, adding appropriate ASSERT on the number of children. This will have to be checked carefully tough and actually I wonder if we are not going to simplify/reorganize the element class...

However we noticed while working on this refactoring that each small change is very likely to break the existing tests on mac, ios or gtk... Actually, I already tried implementing a specific implementation for token elements in bug 155018 but temporarily gave up. So I believe it's safer to keep a generic implementation for now and I would prefer to do any layout change in separate follow-up bugs.

I think the main goal here (and of phase 1) was to move from RenderFlexible inheritance to RenderBlock inheritance in order to really make MathML independent from flexbox, so let's focus on this in this bug. I'll upload a new patch addressing the other review comments.

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

Created attachment 282111 [details]
Patch

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

Created attachment 282113 [details]
Patch

(In reply to comment #29)
> And are we still using the overrides? These changes were don because we needed the override with the anonymous nodes, now that we have remove them where are we using the overrides and could we remove them?

After discussion with jfernandez, I'm adding some comments explaining why these two changes are still needed. But basically, the width of MathML element is computed from its content not its container.

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

Created attachment 282521 [details]
Patch

New version to take into account whitespace change of http://trac.webkit.org/changeset/202727. Minor changes in RenderMathMLBlock.h too.

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

Created attachment 282584 [details]
Patch

Rebasing ios & mac tests after bug 159339.

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

Created attachment 283015 [details]
Patch

Since many MathML tests have been enabled again on iOS & Mac, I'm updating the patch again to verify that it does not break these tests.

[Brent Fulgham]

Comment on attachment 283015 [details]
Patch

A great cleanup! r=me.

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

Committed r202934: <http://trac.webkit.org/changeset/202934>

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

Rebasing the same tests on other ports too:

win: https://trac.webkit.org/changeset/202959
efl: https://trac.webkit.org/changeset/202958