Bug 153991

Created attachment 270869 [details] Patch

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 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.

(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.

Created attachment 271166 [details] Patch

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).

Created attachment 273424 [details] Patch (move paintChildren functions) This is a simple patch to move duplicate paintChildren code into RenderMathMLBlock.

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.

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

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...

Created attachment 274103 [details] Patch

Created attachment 275294 [details] Patch

Created attachment 277377 [details] Patch

Created attachment 278726 [details] Patch Updating test expectations...

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()

Created attachment 281973 [details] Patch

Created attachment 281989 [details] Patch

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

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 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

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 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

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 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

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

Created attachment 282048 [details] Patch Updating two tests to take into account removal of line breaks that were due to flexboxes.

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.

Created attachment 282050 [details] Patch

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.

> 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.

Created attachment 282111 [details] Patch

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.

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.

Created attachment 282584 [details] Patch Rebasing ios & mac tests after bug 159339.

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 on attachment 283015 [details] Patch A great cleanup! r=me.

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

Rebasing the same tests on other ports too: win: https://trac.webkit.org/changeset/202959 efl: https://trac.webkit.org/changeset/202958