Bug 153991

Summary: Refactor layout functions to avoid using flexbox in MathML
Product: WebKit Reporter: Alejandro G. Castro <alex>
Component: MathMLAssignee: Frédéric Wang (:fredw) <fred.wang>
Status: RESOLVED FIXED    
Severity: Normal CC: alex, bfulgham, buildbot, commit-queue, darin, fred.wang, hyatt, jeffrey+webkit, jfernandez, mrobinson, rego, rhodovan.u-szeged, rniwa, sabouhallawa, simon.fraser, svillar, zalan
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 153208, 153742, 153917, 153918, 153987, 155018, 155019, 155168, 159090, 159091, 159114, 159339    
Bug Blocks: 123348, 139480, 151592, 44210, 107613, 121054, 127775, 133845, 138095, 139403, 159336, 159348    
Attachments:
Description Flags
Patch
none
Patch
none
Patch (move paintChildren functions)
none
Patch (flexbox to block)
none
Patch (move paintChildren functions)
none
Patch (initial attempt, duplicating RenderFlexibleBox logic)
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
buildbot: commit-queue-
Archive of layout-test-results from ews100 for mac-yosemite
none
Archive of layout-test-results from ews121 for ios-simulator-wk2
none
Archive of layout-test-results from ews107 for mac-yosemite-wk2
none
Archive of layout-test-results from ews112 for mac-yosemite
none
Patch
none
Patch
alex: review-
Patch
none
Patch
none
Patch
none
Patch
none
Patch bfulgham: review+

Description Alejandro G. Castro 2016-02-08 10:39:25 PST
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.
Comment 1 Alejandro G. Castro 2016-02-08 11:29:47 PST
Created attachment 270869 [details]
Patch
Comment 2 Frédéric Wang (:fredw) 2016-02-10 03:17:33 PST
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
Comment 3 Frédéric Wang (:fredw) 2016-02-11 05:31:05 PST
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.
Comment 4 Alejandro G. Castro 2016-02-12 04:30:53 PST
(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.
Comment 5 Alejandro G. Castro 2016-02-12 04:32:59 PST
Created attachment 271166 [details]
Patch
Comment 6 Frédéric Wang (:fredw) 2016-03-09 04:43:06 PST
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).
Comment 7 Frédéric Wang (:fredw) 2016-03-09 05:21:48 PST
Created attachment 273424 [details]
Patch (move paintChildren functions)

This is a simple patch to move duplicate paintChildren code into RenderMathMLBlock.
Comment 8 Frédéric Wang (:fredw) 2016-03-09 05:24:20 PST
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.
Comment 9 Frédéric Wang (:fredw) 2016-03-15 05:33:19 PDT
Created attachment 274089 [details]
Patch (move paintChildren functions)
Comment 10 Frédéric Wang (:fredw) 2016-03-15 06:40:26 PDT
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...
Comment 11 Frédéric Wang (:fredw) 2016-03-15 10:49:36 PDT
Created attachment 274103 [details]
Patch
Comment 12 Frédéric Wang (:fredw) 2016-03-31 08:40:11 PDT
Created attachment 275294 [details]
Patch
Comment 13 Frédéric Wang (:fredw) 2016-04-26 09:00:21 PDT
Created attachment 277377 [details]
Patch
Comment 14 Frédéric Wang (:fredw) 2016-05-12 03:15:55 PDT
Created attachment 278726 [details]
Patch

Updating test expectations...
Comment 15 Frédéric Wang (:fredw) 2016-05-20 00:14:16 PDT
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()
Comment 16 Frédéric Wang (:fredw) 2016-06-24 06:29:05 PDT
Created attachment 281973 [details]
Patch
Comment 17 Frédéric Wang (:fredw) 2016-06-24 09:48:54 PDT
Created attachment 281989 [details]
Patch
Comment 18 Build Bot 2016-06-24 10:44:03 PDT
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
Comment 19 Build Bot 2016-06-24 10:44:08 PDT
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
Comment 20 Build Bot 2016-06-24 10:44:11 PDT
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
Comment 21 Build Bot 2016-06-24 10:44:17 PDT
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
Comment 22 Build Bot 2016-06-24 10:46:17 PDT
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
Comment 23 Build Bot 2016-06-24 10:46:22 PDT
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
Comment 24 Build Bot 2016-06-24 11:05:50 PDT
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
Comment 25 Build Bot 2016-06-24 11:05:55 PDT
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
Comment 26 Frédéric Wang (:fredw) 2016-06-24 23:52:00 PDT
Created attachment 282048 [details]
Patch

Updating two tests to take into account removal of line breaks that were due to flexboxes.
Comment 27 WebKit Commit Bot 2016-06-24 23:53:25 PDT
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.
Comment 28 Frédéric Wang (:fredw) 2016-06-24 23:55:40 PDT
Created attachment 282050 [details]
Patch
Comment 29 Alejandro G. Castro 2016-06-25 13:07:40 PDT
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.
Comment 30 Frédéric Wang (:fredw) 2016-06-26 23:51:37 PDT
> 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.
Comment 31 Frédéric Wang (:fredw) 2016-06-27 01:25:26 PDT
Created attachment 282111 [details]
Patch
Comment 32 Frédéric Wang (:fredw) 2016-06-27 02:33:32 PDT
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.
Comment 33 Frédéric Wang (:fredw) 2016-07-01 01:03:03 PDT
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.
Comment 34 Frédéric Wang (:fredw) 2016-07-01 14:39:17 PDT
Created attachment 282584 [details]
Patch

Rebasing ios & mac tests after bug 159339.
Comment 35 Frédéric Wang (:fredw) 2016-07-07 08:49:10 PDT
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.
Comment 36 Brent Fulgham 2016-07-07 14:17:47 PDT
Comment on attachment 283015 [details]
Patch

A great cleanup! r=me.
Comment 37 Frédéric Wang (:fredw) 2016-07-07 14:48:59 PDT
Committed r202934: <http://trac.webkit.org/changeset/202934>
Comment 38 Frédéric Wang (:fredw) 2016-07-07 22:31:34 PDT
Rebasing the same tests on other ports too:

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