[Webkit-unassigned] [Bug 179682] Incorrect bounds inside <mover>/<munder> when a stretchy operator is present

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=179682

Frédéric Wang (:fredw) <fred.wang at free.fr> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #328323|review?                     |review-
              Flags|                            |

--- Comment #48 from Frédéric Wang (:fredw) <fred.wang at free.fr> ---
Comment on attachment 328323
  --> https://bugs.webkit.org/attachment.cgi?id=328323
Patch

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

We are getting closer, but I'm doing r- at least because the patch disables an important pixel test for horizontal stretching and  causes a regression with the base case (no embellishments). It seems some things are wrong/unclear in the relayout, let's see if we can improve that a bit too in the next iteration.

> Source/WebCore/ChangeLog:24
> +        what it tests is covered in the new test case.

mathml/opentype/opentype-stretchy-horizontal.html is a pixel test to check the rendering of stretchy operators with a base size, variant size and glyph assembly. The rendering is *not* tested by the new test so you should not disable it. Operator spacing is irrelevant for this mathml/opentype/opentype-stretchy-horizontal.html, though so please open a preliminary bug that sets lspace="0px" rspace="0px" on the <mo> operators of this mathml/opentype/opentype-stretchy-horizontal.html and updates the PNG/txt expectations, so that we can really see what this patch is changing (hopefully it should not change anything).

> Source/WebCore/rendering/mathml/RenderMathMLOperator.h:49
> +    void setStretchWidthLocked(bool stretchWidthLock) { m_isStretchWidthLocked = stretchWidthLock; }

nit: The argument name should be "stretchWidthLocked"

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:73
> +    stretchyOperator->setStretchWidthLocked(true);

This leaves the stretchyOperator locked, I guess you mean setStretchWidthLocked(false) instead?

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:102
>              child->layout();

I wonder whether that should be stretchyOperator->setNeedsLayout(); child->layoutIfNeeded();

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:113
> +            stretchyOperators[i]->stretchTo(stretchWidthForNonBase());

This is wrong as it does not work the base case (no embellishments, first row of attachment 328081). I think what you want is two LayoutUnit variables stretchWidthForScripts and stretchWidthForBase both are set to the maximum of the width of non-stretchy children in the first loop as this is what is clear from the spec. Then you can adjust stretchWidthForScripts and stretchWidthForBase to include other widths of stretchy children in the calculation.

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.h:52
> +    LayoutUnit stretchWidthForNonBase() const { return firstChildBox()->logicalWidth(); };

I'm not sure you really need these helper functions. The variables and extra comments should be enough.

> LayoutTests/TestExpectations:568
> +mathml/opentype/opentype-stretchy-horizontal.html [ Skip ]

This should not be disabled.

> LayoutTests/mathml/opentype/munderover-stretch-width.html:46
> +        runCase(2, 'simple stretchy under', 'red', 'op');

The base case (no embellishments) is the most important one and is clear from the spec, so I propose you move it into a separate test and performs exhaustive testing of the combinations:

- munder/mover/munderover
- one, two or three stretchy operators.
- base/under/over as the widest non-stretchy operator (is there is one) OR comparing with the max width of unstretched operators (if all operators are stretchy). You can put the operator in a <mo stretchy="false"> or an <mtext> to get the unstretched size.

Currently, your test is only checking munder/mover, one stretchy operator as the script and one non-stretchy as the base so you can not see that your code is wrong.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20171204/23853e96/attachment-0001.html>