RenderMathMLUnderOver adds spacing to the child operator indefinitely when resizing the window

• Component: MathML
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

1. Launch webkit with the following 

<div>
	<math xmlns='http://www.w3.org/1998/Math/MathML' display='block'>
		<mi>S</mi>
		<mover>
			<mo>&#x02192;</mo>
			<mrow>
				<mi>K</mi>
				<mn>1</mn>
			</mrow>
		</mover>
		<mi>R</mi>
	</math>
</div>

2. Resize the window from the left side multiple times increasing and decreasing the width of the window

Result: the arrow operator of the mover construct keeps moving to the left.  At the beginning the arrow operator overlaps the text before the mover construct.  At the end the arrow might disappear at the end until then page is reloaded.

<rdar://problem/18432214>

[Said Abou-Hallawa]

Created attachment 239078 [details]
Before resizing

[Said Abou-Hallawa]

Created attachment 239079 [details]
After resizing

[Said Abou-Hallawa]

The bug happens because in RenderMathMLUnderOver::layout() we get the maximum width of all the children.  This step includes running the layout for any dirty child.  After this step we stretch the operator of the math object to the maximum width.  The last thing we do is to run the layout of the base class which adds spacing to the object and its children.  When calling RenderMathMLUnderOver::layout() because of resizing, we do not run the layout for the children because they are not dirty.  But when we get the width of the children, we get the width after adding the spacing.  And when running the layout of the base class, a new spacing will be added to the math object.  Because the alignment of the display math is centered, increasing the width of the math operator causes the starting position to be moved to the left.

[Said Abou-Hallawa]

Created attachment 239216 [details]
Patch

[Said Abou-Hallawa]

Created attachment 239254 [details]
Patch

[Darin Adler]

Comment on attachment 239254 [details]
Patch

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

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:77
> +    Vector<RenderMathMLOperator*> renderOperators;

Would be more efficient to put an inline capacity here so we don't do memory allocation unless we have an unusually large number of operators.

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:97
> +    for (auto& renderOperator : renderOperators) {
> +        renderOperator->stretchTo(stretchWidth);
>      }

No braces on a 1-line for statement body in WebKit coding style.

[Daniel Bates]

Comment on attachment 239254 [details]
Patch

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

> LayoutTests/mathml/presentation/munderover-window-resize.html:5
> +  <math xmlns='http://www.w3.org/1998/Math/MathML' display='block'>

Nit: The attribute xmlns is unnecessary given that this is an HTML5 document (as specified by the DOCTYPE directive).

> LayoutTests/mathml/presentation/munderover-window-resize.html:28
> +      </msub>         

Please remove the trailing whitespace on this line.

> LayoutTests/mathml/presentation/munderover-window-resize.html:87
> +window.addEventListener("load", function(){

Nit: Is it necessary to register a Load event listener as opposed to inlining the contents of the function body into the <script>?

> LayoutTests/mathml/presentation/munderover-window-resize.html:90
> +    window.resizeTo(250, window.outerHeight); 
> +    window.resizeTo(width, window.outerHeight);

I take it we are guaranteed to perform a layout between these two calls?

[Said Abou-Hallawa]

Created attachment 239448 [details]
Patch

[Said Abou-Hallawa]

In reply to comment #6)
> (From update of attachment 239254 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239254&action=review
> 
> > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:77
> > +    Vector<RenderMathMLOperator*> renderOperators;
> 
> Would be more efficient to put an inline capacity here so we don't do memory allocation unless we have an unusually large number of operators.
> 
> > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:97
> > +    for (auto& renderOperator : renderOperators) {
> > +        renderOperator->stretchTo(stretchWidth);
> >      }
> 
> No braces on a 1-line for statement body in WebKit coding style.

I was testing the fix when I realized we still have a similar problem when zooming the view in or out.  The width of the math operator child of the <munderover> object keeps increasing every time the view is zoomed.  Also I noticed that the math operator does not display its glyph centered under the text (please see the right aroow in the 'Before resizing' image).

So I realized that this fix is not the right one.  I debugged the code more and I found out the real problem. It happens because of using a stall value for the stretch size.  Here is the explanation:

-- The function RenderMathMLUnderOver::layout() does the layout for the <munderover> object in three steps:
  a) It runs the layout for the two children and then gets the maximum size of them.
  b) Then it sets the stretch size for the math operator.
  c) And finally it calls the base class to run the layout for the children again.

** In step a), the layout() of the math operator calls RenderMathMLOperator::computePreferredLogicalWidths() which sets the logical_width = max(stretch_size, glyph_size) + leading_sapce + trailing_sapce. But since stretch_size is not set yet, we end up having: logical_width = glyph_size + leading_sapce + trailing_sapce.

** In step b) we set stretch_size = glyph_size + leading_sapce + trailing_sapce

** In step c), the layout() of the math operator calls RenderMathMLOperator::computePreferredLogicalWidths() a second time we end having:
logical_width = glyph_size + 2 * ( leading_sapce + trailing_sapce ) which is wrong but not very obvious.

-- When the window is resized or the view is zoomed in or out.  The rendering object is marked as 'needsLayout' but the stretch size is not changed.  And here is what happens in RenderMathMLUnderOver::layout() this time:

** In step a) the RenderMathMLOperator needs to run layout, so it calls its layout() function.  When RenderMathMLOperator::computePreferredLogicalWidths() gets called the third time, it uses the old stretch size, so get:
logical_width = max(stretch_size, glyph_size) + leading_sapce + trailing_sapce; which is translated to
logical_width = glyph_size + 2 * (leading_sapce + trailing_sapce)

** In step b) we set stretch_size = glyph_size + 2 * (leading_sapce + trailing_sapce )

** In step c) we end up having logical_width = glyph_size + 3 * ( leading_sapce + trailing_sapce ).

So the more we do the zoom in or out, the more we add spacing to the logical with of the math operator.

The fix is to ensure the stretch size of the math operator render object is reset before running its layout.  Running the layout means either the page is just loaded or it's invalidated which means, no layout stall data should be used.

[Said Abou-Hallawa]

(In reply to comment #7)
> (From update of attachment 239254 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239254&action=review
> 
> > LayoutTests/mathml/presentation/munderover-window-resize.html:5
> > +  <math xmlns='http://www.w3.org/1998/Math/MathML' display='block'>

Done.

> Nit: The attribute xmlns is unnecessary given that this is an HTML5 document (as specified by the DOCTYPE directive).
> 
> > LayoutTests/mathml/presentation/munderover-window-resize.html:28
> > +      </msub>         
> 
> Please remove the trailing whitespace on this line.

Done
 
> > LayoutTests/mathml/presentation/munderover-window-resize.html:87
> > +window.addEventListener("load", function(){
> 
> Nit: Is it necessary to register a Load event listener as opposed to inlining the contents of the function body into the <script>?

Done.I removed the event listener and now the javascript should run after loading the page.

> > LayoutTests/mathml/presentation/munderover-window-resize.html:90
> > +    window.resizeTo(250, window.outerHeight); 
> > +    window.resizeTo(width, window.outerHeight);
> 
> I take it we are guaranteed to perform a layout between these two calls?

Yes The layout runs because of the resizing or zooming.  I stepped in the x-code debugger and I made sure the layout runs for every time the view is zoomed or the window is resized.

[Build Bot]

Comment on attachment 239448 [details]
Patch

Attachment 239448 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5032493468090368

New failing tests:
mathml/opentype/opentype-stretchy-horizontal.html
mathml/presentation/inferred-mrow-stretchy.html
mathml/presentation/stretchy-depth-height-symmetric.html
mathml/opentype/opentype-stretchy.html
mathml/presentation/stretchy-depth-height.html
mathml/presentation/mo-stretchy-vertical-bar.html
mathml/presentation/munderover-window-resize.html

[Build Bot]

Created attachment 239449 [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.5

[Build Bot]

Comment on attachment 239448 [details]
Patch

Attachment 239448 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6128910571929600

New failing tests:
mathml/opentype/opentype-stretchy-horizontal.html
mathml/presentation/inferred-mrow-stretchy.html
mathml/presentation/stretchy-depth-height-symmetric.html
mathml/opentype/opentype-stretchy.html
mathml/presentation/stretchy-depth-height.html
mathml/presentation/mo-stretchy-vertical-bar.html
mathml/presentation/munderover-window-resize.html

[Build Bot]

Created attachment 239451 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

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

[Darin Adler]

Comment on attachment 239448 [details]
Patch

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

review- because of the test failures, otherwise looks fine

> Source/WebCore/rendering/mathml/RenderMathMLOperator.h:69
> +    void resetStretchSize() { m_isVertical ? m_stretchHeightAboveBaseline = m_stretchDepthBelowBaseline = 0 : m_stretchWidth = 0; }

I think we should start this as private. Seems like the body could be inside the RenderMathMLOperator.cpp file, too. We could still mark it inline, but don’t need it in the header.

Also, WebKit coding style explicitly frowns upon setting two variables with a single chained assignment statement and implicitly frowns upon using the ?: operator on an expression with side effects. I think we should just use a plain old if statement for this.

> Source/WebCore/rendering/mathml/RenderMathMLOperator.h:77
> +    virtual void layout() override;

I suggest making this protected, or perhaps even private.

[Said Abou-Hallawa]

Created attachment 239508 [details]
Patch

[Said Abou-Hallawa]

(In reply to comment #15)
> (From update of attachment 239448 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239448&action=review
> 
> review- because of the test failures, otherwise looks fine
> 
> > Source/WebCore/rendering/mathml/RenderMathMLOperator.h:69
> > +    void resetStretchSize() { m_isVertical ? m_stretchHeightAboveBaseline = m_stretchDepthBelowBaseline = 0 : m_stretchWidth = 0; }
>
> I think we should start this as private. Seems like the body could be inside the RenderMathMLOperator.cpp file, too. We could still mark it inline, but don’t need it in the header.
> 
> Also, WebKit coding style explicitly frowns upon setting two variables with a single chained assignment statement and implicitly frowns upon using the ?: operator on an expression with side effects. I think we should just use a plain old if statement for this.
> 

Done expect it is still public.  The reason is it will be called from RenderMathMLUnderOver::layout(). More about the need for this is explained below.

> > Source/WebCore/rendering/mathml/RenderMathMLOperator.h:77
> > +    virtual void layout() override;
> 
> I suggest making this protected, or perhaps even private.

I had to go back to my original solution: resetting the stretch size before running the layout for the math operator in the RenderMathMLUnderOver::layout().  The reason is forcing stretching the math operator should happen only in RenderMathMLUnderOver.  But the math operator itself can have stretch size set in the html page and in this case we should not change the stretch size.  We need to limit resetting the stretch size of the math operator to the case where it is a child of a  RenderMathMLUnderOver object and not always as I was doing in the previous patch.  This was the reason for the test cases failures.

[Said Abou-Hallawa]

Created attachment 239509 [details]
Patch

[Said Abou-Hallawa]

Created attachment 239512 [details]
Patch

[Said Abou-Hallawa]

I fixed a merge issue because my build was not up to date.

[Darin Adler]

Comment on attachment 239512 [details]
Patch

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

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1316
> +    if (m_isVertical)
> +        m_stretchHeightAboveBaseline = m_stretchDepthBelowBaseline = 0;
> +    else
> +        m_stretchWidth = 0;

Without judging the readability of the different styles, in standard WebKit coding style this would be:

    if (m_isVertical) {
        m_stretchHeightAboveBaseline = 0;
        m_stretchDepthBelowBaseline = 0;
    } else
        m_stretchWidth = 0;

[Said Abou-Hallawa]

Created attachment 239545 [details]
Patch

[Said Abou-Hallawa]

(In reply to comment #21)
> (From update of attachment 239512 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239512&action=review
> 
> > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1316
> > +    if (m_isVertical)
> > +        m_stretchHeightAboveBaseline = m_stretchDepthBelowBaseline = 0;
> > +    else
> > +        m_stretchWidth = 0;
> 
> Without judging the readability of the different styles, in standard WebKit coding style this would be:
> 
>     if (m_isVertical) {
>         m_stretchHeightAboveBaseline = 0;
>         m_stretchDepthBelowBaseline = 0;
>     } else
>         m_stretchWidth = 0;

Done

[Chris Dumez]

Comment on attachment 239545 [details]
Patch

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

> Source/WebCore/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

You already got a review so you're supposed to update this line before reuploaded. Should be "Reviewed by Darin Adler.".

Then, don't set the r? flag, only the cq? one.

[Said Abou-Hallawa]

Created attachment 239546 [details]
Patch

[Chris Dumez]

Comment on attachment 239546 [details]
Patch

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

> LayoutTests/ChangeLog:6
> +        Reviewed by Darin Adler, Daniel Bates.

I've never seen more than 1 reviewer on that line (Technically, only 1 person can do the formal review with Bugzilla) but this may very well be OK so I'll let someone else take a look and cq+.

[Said Abou-Hallawa]

Created attachment 239547 [details]
Patch

[Daniel Bates]

(In reply to comment #26)
> (From update of attachment 239546 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239546&action=review
> 
> > LayoutTests/ChangeLog:6
> > +        Reviewed by Darin Adler, Daniel Bates.
> 
> I've never seen more than 1 reviewer on that line (Technically, only 1 person can do the formal review with Bugzilla) but this may very well be OK so I'll let someone else take a look and cq+.

As far as I know we do not have a policy against listing more than one reviewer in a change log entry. One example of a change set whose change log entry references two reviewers is <http://trac.webkit.org/changeset/142924>.

[WebKit Commit Bot]

Comment on attachment 239547 [details]
Patch

Rejecting attachment 239547 [details] from commit-queue.

New failing tests:
mathml/presentation/munderover-window-resize.html
Full output: http://webkit-queues.appspot.com/results/6040983330357248

[WebKit Commit Bot]

Created attachment 239552 [details]
Archive of layout-test-results from webkit-cq-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: webkit-cq-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 239547 [details]
Patch

Attachment 239547 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5870690024554496

New failing tests:
mathml/presentation/munderover-window-resize.html

[Build Bot]

Created attachment 239553 [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.5

[Build Bot]

Comment on attachment 239547 [details]
Patch

Attachment 239547 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6571544532746240

New failing tests:
mathml/presentation/munderover-window-resize.html

[Build Bot]

Created attachment 239557 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

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

[Said Abou-Hallawa]

Created attachment 239558 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 239558 [details]
Patch

The test I submitted was failing on mac.  Since it is a rendering tree dump test, I think I need to specify explicit font name and size.  This is going to avoid any font font fallback change on the system level.

[Build Bot]

Comment on attachment 239558 [details]
Patch

Attachment 239558 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6352033451868160

New failing tests:
mathml/opentype/munderover-window-resize.html

[Build Bot]

Created attachment 239566 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

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

[Build Bot]

Comment on attachment 239558 [details]
Patch

Attachment 239558 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6487152720347136

New failing tests:
mathml/opentype/munderover-window-resize.html

[Build Bot]

Created attachment 239569 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

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

[Said Abou-Hallawa]

Created attachment 239572 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 239572 [details]
Patch

The test munderover-window-resize.html was failing again because of one pixel difference everywhere between the expected and the actual rendering trees.  This should have happened because I generated the expected results on mac-yosemite while the Bot system runs on mac-mountainlion.

[Said Abou-Hallawa]

Created attachment 239574 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 239574 [details]
Patch

Clearing flags on attachment: 239574

Committed r174540: <http://trac.webkit.org/changeset/174540>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Said Abou-Hallawa]

Reopening to attach new patch.

[Said Abou-Hallawa]

Created attachment 239593 [details]
Patch

[Said Abou-Hallawa]

I have to change the test to a reference test instead of rendering tree test since it was failing on WK1 but was succeeding on WK2.

[Simon Fraser (smfr)]

Comment on attachment 239593 [details]
Patch

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

> LayoutTests/mathml/opentype/munderover-window-resize-expected.html:1
> +<!DOCTYPE html>

The filename should be changed; this isn't about resize any more, it's about layout being unstable for some mathml operators.

> LayoutTests/mathml/opentype/munderover-window-resize-expected.html:4
> +  <title>Open Type MATH - under-over operator</title>

We usually don't bother with titles in tests.

> LayoutTests/mathml/opentype/munderover-window-resize-expected.html:5
> +  <meta charset="utf-8">

You don't need this.

> LayoutTests/mathml/opentype/munderover-window-resize-expected.html:7
> +    /* This font is taken from Mozilla's test suite. */

Is the comment necessary?

> LayoutTests/mathml/opentype/munderover-window-resize.html:41
> +      setTimeout(function(){testRunner.notifyDone();},7000);

We can't have a test take 7s. Imagine if every test took multiple seconds; we have tens of thousands of them!

You should change this test to do a single layout (foo.style.width = "400px") which presumably would also show the bug. You can probably do this synchronously:

<div id="container" style="width: 300px">....

<script>
document.body.offsetWidth; // force initial layout
document.getElementById('container').style.width = "400px";
</script>

[Said Abou-Hallawa]

Created attachment 239598 [details]
Patch

[Said Abou-Hallawa]

(In reply to comment #49)
> (From update of attachment 239593 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239593&action=review
> 
> > LayoutTests/mathml/opentype/munderover-window-resize-expected.html:1
> > +<!DOCTYPE html>
> 
> The filename should be changed; this isn't about resize any more, it's about layout being unstable for some mathml operators.
> 

Done, I renamed it to be munderover-layout-resize since it is still about resizing the container of the math and making sure the layout of the munderover math object is still valid.

> > LayoutTests/mathml/opentype/munderover-window-resize-expected.html:4
> > +  <title>Open Type MATH - under-over operator</title>
> 
> We usually don't bother with titles in tests.
> 

Removed.  I copied it from LayoutTests\mathml\opentype\opentype-stretchy-horizontal.html.

> > LayoutTests/mathml/opentype/munderover-window-resize-expected.html:5
> > +  <meta charset="utf-8">
> 
> You don't need this.
>

Removed.  I copied it from LayoutTests\mathml\opentype\opentype-stretchy-horizontal.html.
 
> > LayoutTests/mathml/opentype/munderover-window-resize-expected.html:7
> > +    /* This font is taken from Mozilla's test suite. */
> 
> Is the comment necessary?
> 

Removed.  I copied it from LayoutTests\mathml\opentype\opentype-stretchy-horizontal.html.

> > LayoutTests/mathml/opentype/munderover-window-resize.html:41
> > +      setTimeout(function(){testRunner.notifyDone();},7000);
> 
> We can't have a test take 7s. Imagine if every test took multiple seconds; we have tens of thousands of them!
> 
> You should change this test to do a single layout (foo.style.width = "400px") which presumably would also show the bug. You can probably do this synchronously:
> 
> <div id="container" style="width: 300px">....
> 
> <script>
> document.body.offsetWidth; // force initial layout
> document.getElementById('container').style.width = "400px";
> </script>

I tried your solution but I did not notice any overlap in the released WebKit (the one without my change) when opening the test file.  Or at least my eye could not catch any change so I was not sure if the layout was forced before the resizing or not.  I kept the CSS animation but I shortened its period to be 1 second only.

[Said Abou-Hallawa]

Created attachment 239639 [details]
Patch

[Said Abou-Hallawa]

Resubmitting the reference test only because the original patch has landed.

[Said Abou-Hallawa]

Created attachment 239649 [details]
Patch

[Said Abou-Hallawa]

I removed the CSS animation and I am forcing the layout synchronously by changing the width of the container <div> element.

[Dean Jackson]

Committed r174614: <http://trac.webkit.org/changeset/174614>