Bug 99618

Summary: [MathML] Implement the mmultiscripts tag
Product: WebKit Reporter: Dave Barton <dbarton>
Component: MathMLAssignee: Frédéric Wang (:fredw) <fred.wang>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, apinheiro, bfulgham, buildbot, cfleizach, commit-queue, ddkilzer, dmazzoni, donggwan.kim, eflews.bot, esprehn+autocc, fred.wang, glenn, gur.trio, gyuyoung.kim, jdiggs, kondapallykalyan, macpherson, mario, menard, mrobinson, rakuco, rniwa
Priority: P2 Keywords: InRadar, WebExposed
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 84019, 99623, 120069    
Attachments:
Description Flags
testcase
none
Patch V1
none
Patch V2
none
Patch V3
none
Patch V4
none
testcase (dynamic)
none
Patch V5
eflews.bot: commit-queue-
Patch V5 - bis
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion
none
Patch V6
none
test-case that shows red boxes
none
Patch V7
cfleizach: review-, buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2
none
Patch V8
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion
none
Patch V9
cfleizach: review+
Patch Final Version none

Description Dave Barton 2012-10-17 11:11:41 PDT
See <http://www.w3.org/TR/MathML3/chapter3.html#presm.mmultiscripts>. This will use techniques from RenderMathMLSubSup.h/cpp. If someone else would like to work on this, I'd be happy to discuss ideas.
Comment 1 David Kilzer (:ddkilzer) 2013-07-06 15:23:54 PDT
Some test cases here:  https://eyeasme.com/Joe/MathML/MathML_browser_test
Comment 2 David Kilzer (:ddkilzer) 2013-07-06 15:24:14 PDT
<rdar://problem/14369170>
Comment 3 Frédéric Wang (:fredw) 2013-07-07 01:00:40 PDT
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.
Comment 4 Frédéric Wang (:fredw) 2013-07-07 01:01:43 PDT
Created attachment 206205 [details]
testcase

Here is a basic testcase.
Comment 5 Frédéric Wang (:fredw) 2013-07-29 08:01:17 PDT
Created attachment 207658 [details]
Patch V1

This is a WIP patch.
Comment 6 Frédéric Wang (:fredw) 2013-07-30 13:11:44 PDT
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 7 chris fleizach 2013-07-30 13:35:00 PDT
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
Comment 8 Frédéric Wang (:fredw) 2013-07-31 12:30:05 PDT
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.
Comment 9 Frédéric Wang (:fredw) 2013-08-06 10:53:24 PDT
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)
Comment 10 Frédéric Wang (:fredw) 2013-08-11 05:40:28 PDT
Created attachment 208488 [details]
testcase (dynamic)
Comment 11 Frédéric Wang (:fredw) 2013-08-11 05:44:29 PDT
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 12 EFL EWS Bot 2013-08-11 05:50:51 PDT
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 13 Build Bot 2013-08-11 05:51:05 PDT
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 14 EFL EWS Bot 2013-08-11 05:51:44 PDT
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 15 Build Bot 2013-08-11 05:56:51 PDT
Comment on attachment 208489 [details]
Patch V5

Attachment 208489 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1350970
Comment 16 Frédéric Wang (:fredw) 2013-08-11 05:58:32 PDT
Created attachment 208490 [details]
Patch V5 - bis

Same with explicit parenthesis to avoid a build failure on Mac/EFL.
Comment 17 Build Bot 2013-08-12 11:57:48 PDT
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
Comment 18 Build Bot 2013-08-12 11:57:52 PDT
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
Comment 19 Frédéric Wang (:fredw) 2013-08-13 08:31:34 PDT
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 20 Frédéric Wang (:fredw) 2013-08-20 03:15:18 PDT
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 21 chris fleizach 2013-08-21 16:40:45 PDT
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
Comment 22 Frédéric Wang (:fredw) 2013-08-22 00:14:29 PDT
(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.
Comment 23 Frédéric Wang (:fredw) 2013-08-22 02:15:43 PDT
> > 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 24 chris fleizach 2013-08-22 12:08:04 PDT
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
Comment 25 Frédéric Wang (:fredw) 2013-08-22 13:24:09 PDT
(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.
Comment 26 chris fleizach 2013-08-27 15:23:05 PDT
Created attachment 209808 [details]
test-case that shows red boxes

Attaching a test case that shows red boxes around items. 
Is this expected?
Comment 27 Frédéric Wang (:fredw) 2013-08-28 00:17:57 PDT
(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.
Comment 28 Frédéric Wang (:fredw) 2013-08-31 02:27:25 PDT
Created attachment 210184 [details]
Patch V7

Refreshed patch with review comments addressed.
Comment 29 Build Bot 2013-08-31 03:40:47 PDT
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
Comment 30 Build Bot 2013-08-31 03:40:51 PDT
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 31 Build Bot 2013-08-31 07:20:51 PDT
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
Comment 32 Build Bot 2013-08-31 07:20:56 PDT
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 33 chris fleizach 2013-09-13 10:47:14 PDT
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 34 chris fleizach 2013-09-13 10:47:39 PDT
Comment on attachment 210184 [details]
Patch V7

r- for failing layout test and other errata
Comment 35 Frédéric Wang (:fredw) 2013-09-14 02:56:12 PDT
Created attachment 211638 [details]
Patch V8

Updated patch. The test failure was just a typo in the test when I rename a function.
Comment 36 Frédéric Wang (:fredw) 2013-09-14 03:07:02 PDT
(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 37 Build Bot 2013-09-14 03:48:14 PDT
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
Comment 38 Build Bot 2013-09-14 03:48:17 PDT
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
Comment 39 Frédéric Wang (:fredw) 2013-09-14 04:38:16 PDT
Created attachment 211643 [details]
Patch V9
Comment 40 chris fleizach 2013-09-14 23:49:12 PDT
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
Comment 41 Frédéric Wang (:fredw) 2013-09-15 00:33:21 PDT
Created attachment 211688 [details]
Patch Final Version

Thanks for the review.
Comment 42 WebKit Commit Bot 2013-09-15 01:08:22 PDT
Comment on attachment 211688 [details]
Patch Final Version

Clearing flags on attachment: 211688

Committed r155797: <http://trac.webkit.org/changeset/155797>
Comment 43 WebKit Commit Bot 2013-09-15 01:08:40 PDT
All reviewed patches have been landed.  Closing bug.
Comment 44 Brent Fulgham 2013-10-02 15:23:35 PDT
<rdar://problem/14361918>
Comment 45 Frédéric Wang (:fredw) 2014-03-10 12:25:44 PDT
Mass change: add WebExposed keyword to help MDN documentation.