Bug 99618

Some test cases here: https://eyeasme.com/Joe/MathML/MathML_browser_test

<rdar://problem/14369170>

This is typically the example where the flexboxes+anonymous boxes+padding approach is very inconvenient. In Gecko, the DOM structure is generally exactly the same as the rendering tree structure (so Javascript and CSS apply normally). When one layouts a MathML frame, the layout is first called on its children (so you get their sizes) and then the child coordinates inside the MathML frame are computed. So here, that's just positioning frames from left to right by browsing the list of prescript pairs, then the base and finally the postscript pairs. With WebKit's current approach, one should for example do (there are other possibilities): - create an anonymous boxes for the prescripts and postscripts that are horizontally oriented flexbox. You get something like [PRE] BASE [POST] - for each script pairs in [PRE], create a vertically oriented anonymous flexbox where you put the subscript/sup. Same for [POST]. - tweak the padding of boxes so that the scripts are positioned correctly (ideally, according to TeX heuristics or supscriptshift/subscriptshift) - each time the child list is changed (e.g. children removed/added via Javascript) you have to remove/add the child at the right place and ensure that the rendering tree structure with all these anonymous boxes is preserved - when some CSS style is applied to the children, you must propagate it to the correct anonymous boxes. Without necessarily a complete rewrite as suggested elsewhere, at least having something like in Gecko would be much better.

Created attachment 206205 [details] testcase Here is a basic testcase.

Created attachment 207658 [details] Patch V1 This is a WIP patch.

Created attachment 207760 [details] Patch V2 Here is a better patch that should implement a correct rendering for mmultiscripts and preserve the one for former scripts. I'm still not quite sure how to remove children when the document is being destroyed so I'll have to check that. I also need to do more testing and to write tests.

Comment on attachment 207760 [details] Patch V2 View in context: https://bugs.webkit.org/attachment.cgi?id=207760&action=review > Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:156 > + if (!beforeChild) { this function should probably be broken up into smaller functions so it's a bit easier to digest > Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:357 > + for (RenderObject* child2 = child->nextSibling(); child2 && !child2->node()->hasTagName(MathMLNames::mprescriptsTag); child2 = child2->nextSibling()) { i'd call this sibling rather than child2

Created attachment 207872 [details] Patch V3 Here is a patch that improves insertion/removal of children. However, this is still not correct, the removeChild/addChild calls must be transmitted from the anonymous block to the RenderMathMLScripts object to make that work. I'll need to rework the code to do that cleanly. The old code didn't do that and just had a FIXME comment, so that does not make the situation worse anyway.

Created attachment 208202 [details] Patch V4 New iteration of the patch: i) I've simplified the render tree structure and this fixes an alignment issue with nested mmultiscripts. ii) I've rewritten the addChild/removeChild functions but this is not finished yet. iii) I think I'm done with the reftests for mmultiscripts. I need to write more test for DOM manipulations and ensure that they pass (this is of course related to point ii) Here are a few manual tests: - https://developer.mozilla.org/en-US/docs/Mozilla_MathML_Project/MathML_Torture_Test (test 2) - https://developer.mozilla.org/fr/docs/Mozilla/MathML_Project/mmultiscripts - W3C MathML test suite: http://www.w3.org/Math/testsuite/build/main/Presentation/CSS/mprescripts/mprescripts-01-full.xhtml, http://www.w3.org/Math/testsuite/build/main/Presentation/ScriptsAndLimits/mmultiscripts/mmultiscripts1-full.xhtml. Also check the tests after these two pages (link "next" at the top right) - https://eyeasme.com/Joe/MathML/MathML_browser_test (see multiscripts & greek alphabet)

Created attachment 208488 [details] testcase (dynamic)

Created attachment 208489 [details] Patch V5 First "ready for review" patch i.e. all the problems I was aware with the previous patches are fixed. Testing & Feedback are welcome.

Comment on attachment 208489 [details] Patch V5 Attachment 208489 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/1409844

Comment on attachment 208489 [details] Patch V5 Attachment 208489 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1399763

Comment on attachment 208489 [details] Patch V5 Attachment 208489 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/1413752

Comment on attachment 208489 [details] Patch V5 Attachment 208489 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1350970

Created attachment 208490 [details] Patch V5 - bis Same with explicit parenthesis to avoid a build failure on Mac/EFL.

Comment on attachment 208490 [details] Patch V5 - bis Attachment 208490 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1453182 New failing tests: mathml/presentation/scripts-base-alignment.html platform/mac/accessibility/mathml-elements.html platform/mac/accessibility/mathml-multiscript.html

Created attachment 208552 [details] Archive of layout-test-results from webkit-ews-02 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-02 Port: mac-mountainlion Platform: Mac OS X 10.8.4

Created attachment 208636 [details] Patch V6 Quick update to fix the test failures on Mac. I forgot to say that I disabled two MathML pixel tests. They should probably be converted to reftests.

Comment on attachment 208636 [details] Patch V6 Asking review for the patch. I noticed that there is a "SubSup" that should be replaced by "Scripts" in a comment of the Xcode file. I'm wondering how to handle the failures of the mo-stretch.html and roots.xhtml pixel tests. I'm not really willing to regenerate all the references again and I'd prefer to wait Martin Robinson's refactoring for stretchy operators before converting them to reftests. So for now, I just disable them.

Comment on attachment 208636 [details] Patch V6 View in context: https://bugs.webkit.org/attachment.cgi?id=208636&action=review starting to look at this. it will take me a bit of time to make it through everything. thanks > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3460 > + return (toRenderMathMLBlock(m_renderer)->isRenderMathMLScripts() && !isMathMultiscript()); parentheses not needed here > Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:48 > +// BaseWrapper SubSupPairWrapper* (mprescripts SubSupPairWrapper*)* i'm not sure which render tree structure you're saying you're going to use here. Are you using the above version for valid elements, or the below version for valid and invalid elements? > Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:54 > +bool isMPrescripts(RenderObject *renderObject) isMathPrescript is a better name, or just isPrescript. static should be on the same line as bool ... * is in wrong place > Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:96 > + scriptsStyle->setJustifyContent(m_kind == Sub ? JustifyFlexStart : m_kind == Super ? JustifyFlexEnd : JustifySpaceBetween); i don't see anything explicit about non-wrapped comments but these will be easer to read if the comments wrap after a reasonable number of characters > Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:122 > + for (subSupPair = subSupPair->nextSibling(); subSupPair && !isMPrescripts(subSupPair); subSupPair = subSupPair->nextSibling()) didn't you already hit the prescript tag in the first loop? is there a reason to check for it again? > Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:140 > +void RenderMathMLScripts::addChildInternal(bool normalInsertion, RenderObject* child, RenderObject* beforeChild) i'm not clear what "normal" means in this context. it sounds a bit subjective. can you clarify when that flag is to be used > Source/WebCore/rendering/mathml/RenderMathMLScripts.h:58 > + virtual bool isRenderMathMLScriptsWrapper() const { return true; } OVERRIDE FINAL > Source/WebCore/rendering/mathml/RenderMathMLScripts.h:110 > + virtual bool isRenderMathMLScripts() const { return true; } OVERRIDE

(In reply to comment #21) > i'm not sure which render tree structure you're saying you're going to use here. Are you using the above version for valid elements, or the below version for valid and invalid elements? I'm using the below version for valid and invalid elements. In particular that allows to avoid rebuilding everything when children are added/removed and the mmultiscripts becomes temporarily invalid. > didn't you already hit the prescript tag in the first loop? is there a reason to check for it again? Per the previous remark, invalid mmultiscripts may have more than one mprescripts, so I'll have to take care of that too. > > Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:96 > > + scriptsStyle->setJustifyContent(m_kind == Sub ? JustifyFlexStart : m_kind == Super ? JustifyFlexEnd : JustifySpaceBetween); > > i don't see anything explicit about non-wrapped comments but these will be easer to read if the comments wrap after a reasonable number of characters I used 80 columns at some point but you mentioned I should not (that was in a reftest file IIRC). I also seem to remember that the check-webkit-style complained when a comment wraps in a C++ file. I'll check into that.

> > Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:140 > > +void RenderMathMLScripts::addChildInternal(bool normalInsertion, RenderObject* child, RenderObject* beforeChild) > > i'm not clear what "normal" means in this context. it sounds a bit subjective. can you clarify when that flag is to be used I forgot to reply to this. When a child renderer is inserted/removed, the other child/anonymous renderers must be reorganized to preserve the structure described at the top of the file. normalInsertion/normalRemoval means that the child is just inserted/removed by calling the RenderMathMLBlock member function, without doing any post-processing.

Comment on attachment 208636 [details] Patch V6 View in context: https://bugs.webkit.org/attachment.cgi?id=208636&action=review > LayoutTests/mathml/invalid-scripts-crash.html:5 > + if (window.testRunner) is dumpAsText necessary since this is a reftest? > LayoutTests/mathml/presentation/multiscripts-positions.html:9 > + <!-- This page contains several <mmultiscripts> elements. We place black squares at the position where we expect some red scripts to be, so that these scripts are hidden by the black squares. --> can you explain why you want to add black squares in this description and how you'll use the expected test to test this. > LayoutTests/mathml/scripts-addChild.html:17 > + var t = document.createTextNode(n.toString()); indentation needed > LayoutTests/mathml/scripts-removeChild.html:8 > + <meta charset="utf-8"/> seems unnecessary > LayoutTests/mathml/scripts-removeChild.html:15 > + function removeMPrescript(n) this should be renamed to testRemovePrescript() otherwise its confusing that Test 1-4 are captured using this function > LayoutTests/mathml/scripts-removeChild.html:18 > + var mmultiscripts = document.getElementById("test"+n).getElementsByTagNameNS(mathmlNS, "mmultiscripts"); space around "test" + n needed > LayoutTests/mathml/scripts-removeChild.html:64 > + removeMPrescript(1); indendation

(In reply to comment #24) > > LayoutTests/mathml/invalid-scripts-crash.html:5 > > + if (window.testRunner) > > is dumpAsText necessary since this is a reftest? > This one is not a reftest.

Created attachment 209808 [details] test-case that shows red boxes Attaching a test case that shows red boxes around items. Is this expected?

(In reply to comment #26) > Created an attachment (id=209808) [details] > test-case that shows red boxes > > Attaching a test case that shows red boxes around items. > Is this expected? Yes, this is invalid markup. The MathML spec indicates how invalid markup should be handled: http://www.w3.org/TR/MathML/chapter2.html#interf.error As a comparison, Gecko displays and "invalid-markup" message instead of the mmultiscript element and sends an error to the console. MathJax generally prints an error message in place of invalid element (when it does not crash) but for your example it just shows nothing. Here I chose to handle valid and invalid markup the same way, to avoid having to remove all the renderers when the mmultiscript element becomes temporarily invalid and creating them again when it becomes valid (for example if one adds/removes a child via javascript). However, I added a rule in css/mathml.css so that extra children in msub/msup/msubsup/mmultiscript are painted as merror. I thought that could help people to visualize invalid markup while still showing the mmultiscript content.

Created attachment 210184 [details] Patch V7 Refreshed patch with review comments addressed.

Comment on attachment 210184 [details] Patch V7 Attachment 210184 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1675129 New failing tests: mathml/scripts-removeChild.html

Created attachment 210185 [details] Archive of layout-test-results from webkit-ews-01 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-01 Port: mac-mountainlion Platform: Mac OS X 10.8.4

Comment on attachment 210184 [details] Patch V7 Attachment 210184 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1661334 New failing tests: mathml/scripts-removeChild.html

Created attachment 210192 [details] Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-15 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.4

Comment on attachment 210184 [details] Patch V7 View in context: https://bugs.webkit.org/attachment.cgi?id=210184&action=review a few comments inline. once you fix the failing layout test i think we should move forward > Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:298 > + for (bool isPostScript = true; subSupPair; subSupPair = toRenderMathMLBlock(subSupPair->nextSibling())) { we should probably initialize isPostScript outside of the for loop. It's a bit crowded to stick that initialization in there too > Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:333 > + newPadding = Length(topPadding, Fixed); seems like you can define and declare at the same time Length newPadding(topPadding, Fixed) > Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:364 > +RenderMathMLScriptsWrapper* RenderMathMLScriptsWrapper::createAnonymousWrapper(RenderMathMLScripts* renderObject, WrapperType type) can we make this a PassRefPtr? > Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:423 > + destroy(); is this going to destroy() this object? > Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:440 > + for (RenderObject* previousSibling = subSupPair->previousSibling(); subSupPair != this; subSupPair = toRenderMathMLScriptsWrapper(previousSibling), previousSibling = previousSibling->previousSibling()) { i would move this to the end of the loop subSupPair = toRenderMathMLScriptsWrapper(previousSibling) because it's not directly related to iterated the for loop, and this for () is a bit crowded > Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:486 > + for (RenderObject* nextSibling = subSupPair->nextSibling(); nextSibling && !isPrescript(nextSibling); subSupPair = toRenderMathMLScriptsWrapper(nextSibling), nextSibling = nextSibling->nextSibling()) { ditto about for loop > Source/WebCore/rendering/mathml/RenderMathMLScripts.h:45 > + RenderMathMLScriptsWrapper(Element* element, WrapperType kind) : RenderMathMLBlock(element), m_kind(kind) { }; i don't know about style guidelines for this, but i would bet we should put the code on different lines > Source/WebCore/rendering/mathml/RenderMathMLScripts.h:48 > + virtual void removeChild(RenderObject* child) OVERRIDE; child parameter name not necessary > Source/WebCore/rendering/mathml/RenderMathMLScripts.h:98 > + virtual void removeChild(RenderObject* child) OVERRIDE; child param name not necessary

Comment on attachment 210184 [details] Patch V7 r- for failing layout test and other errata

Created attachment 211638 [details] Patch V8 Updated patch. The test failure was just a typo in the test when I rename a function.

(In reply to comment #33) > > Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:364 > > +RenderMathMLScriptsWrapper* RenderMathMLScriptsWrapper::createAnonymousWrapper(RenderMathMLScripts* renderObject, WrapperType type) > > can we make this a PassRefPtr? I tried to make the argument a PassRefPtr but got an error that RenderMathMLScripts does not implement deref. As I can see all the createAnonymous functions in rendering/ use raw pointers. However, there is a comment in RenderMathMLRow to make the return pointer a ref. Perhaps we can migrate the createAnonymous MathML functions to ref counted pointers in a separate bug? > > > Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:423 > > + destroy(); > > is this going to destroy() this object? Not the C++ destructor, but that will clean up all its data, I think. In particular that's why I get the parent node in RenderMathMLScriptsWrapper::addChild before calling addChildInternal. Without that, if I create/delete many nodes with attachment 208488 [details], then I get memory leaks.

Comment on attachment 211638 [details] Patch V8 Attachment 211638 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1862467 New failing tests: platform/mac/accessibility/mathml-multiscript.html

Created attachment 211640 [details] Archive of layout-test-results from webkit-ews-07 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-07 Port: mac-mountainlion Platform: Mac OS X 10.8.5

Created attachment 211643 [details] Patch V9

Comment on attachment 211643 [details] Patch V9 View in context: https://bugs.webkit.org/attachment.cgi?id=211643&action=review > Source/WebCore/rendering/mathml/RenderMathMLScripts.h:57 > + virtual const char* renderName() const { return (m_kind == Base ? "Base Wrapper" : "SubSupPair Wrapper"); } these parens are not necessary

Created attachment 211688 [details] Patch Final Version Thanks for the review.

Comment on attachment 211688 [details] Patch Final Version Clearing flags on attachment: 211688 Committed r155797: <http://trac.webkit.org/changeset/155797>

All reviewed patches have been landed. Closing bug.

<rdar://problem/14361918>

Mass change: add WebExposed keyword to help MDN documentation.