Bug 179682 – Incorrect bounds inside <mover>/<munder> when a stretchy operator is present

• Component: MathML
• Status: RESOLVED FIXED

[Minsheng Liu]

The current layout algorithm for <mover>/<munder> does not give the correct bounds for each part (base/over/under> if a part contains a stretched operator. Instead, the bound is calculated by layout-ing the children of each part individually. The behavior is buggy as:

1. It is against the logical idea of bounds.
2. It is confusing when a user examines the bounds of those elements, like when inspecting elements.
3. It requires a dirty fix in RenderMathMLOperator::paint, which leads to the other bug https://bugs.webkit.org/show_bug.cgi?id=170272.

[Minsheng Liu]

Created attachment 327598 [details]
Patch

[Minsheng Liu]

As you can see from the patch, one test case is updated:
mathml/opentype/opentype-stretchy-horizontal.html

I have updated it for platform "mac". How could I update test results for other platforms as well?

[Minsheng Liu]

Created attachment 327599 [details]
Patch

[EWS Watchlist]

Attachment 327599 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Minsheng Liu]

Created attachment 327600 [details]
Patch

[EWS Watchlist]

Comment on attachment 327600 [details]
Patch

Attachment 327600 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5369772

New failing tests:
mathml/opentype/opentype-stretchy-horizontal.html

[EWS Watchlist]

Created attachment 327602 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Minsheng Liu]

The iOS simulator has the old expectation, so the failure is expected. Am I expected to simply delete the expectation file? Also, gtk also contains an outdated expectation file, but it is not included in EWS

[Frédéric Wang (:fredw)]

Comment on attachment 327600 [details]
Patch

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

Thanks Minsheng. I think we will really have to rewrite the function (see below). For the tests, you can edit TestExpectations files. There are platform-specific expectations in Layout/platform: 

fred@igalia-fwang:~/src-obj/webkit/WebKit/LayoutTests$ find . -type f -name TestExpectations
./platform/ios-device-wk2/TestExpectations
./platform/gtk-wk2/TestExpectations
./platform/mac/TestExpectations
./platform/wk2/TestExpectations
./platform/ios-wk2/TestExpectations
./platform/ios-simulator/TestExpectations
./platform/ios-wk1/TestExpectations
./platform/ios-simulator-wk1/TestExpectations
./platform/ios-simulator-wk2/TestExpectations
./platform/ios-device/TestExpectations
./platform/gtk/TestExpectations
./platform/mac-wk2/TestExpectations
./platform/ios-device-wk1/TestExpectations
./platform/win/TestExpectations
./platform/gtk-wayland/TestExpectations
./platform/wpe/TestExpectations
./platform/ios/TestExpectations
./platform/mac-wk1/TestExpectations
./platform/mac-elcapitan/TestExpectations
./TestExpectations

And there are corresponding test expectations (e.g. platform/ios/mathml/radical-fallback-expected.txt) that you can update too.

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:54
>  {

Can you add an ASSERT(isValid()) here? Then we are sure that there are exactly 2 or 3 children.

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:57
> +    Vector<RenderBox*, 2> stretchingBlocks;

Maybe, we should use 3 as the initial size here, since that's the maximum we can get.

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:58
>  

So I think we should maybe try to understand the algorithm we want, check with the Math WG and explain with comments here. See https://www.w3.org/TR/MathML3/chapter3.html#id.3.2.5.8.2. For vertical operators they say "cover ... non-stretchy direct sub-expressions" but for horizontal operators, they say "cover ... other direct sub-expressions" which seems inconsistent, we should ask them clarification. Later they indicate that "If a stretchy operator is required to stretch, but all other expressions in the containing element (as described above) are also stretchy, all elements that can stretch should grow to the maximum of the normal unstretched sizes of all elements in the containing object, if they can grow that large". The "Skipping the embellished op" comment suggests that it won't work if we skip non-stretchy (as we do for the vertical rule in RenderMathMLRow::computeLineVerticalStretch) and indeed we should really consider the non-strechy parts of an embellished operators for the size computation. Note that the Math WG is "on hold" ( https://lists.w3.org/Archives/Public/www-math/2016Jul/0001.html ) so we should probably not wait for them to reply soon.

In any case, there is an issue here: The target stretch width calculated here might change after a second layout now that RenderMathMLOperator::stretchTo changes the logical width to give the correct stretched size of the operator instead of the unstretched size. I think we will have no other choice than calculating the target size again with unstretched operators. Performance will probably not be good when we have many nested embellished operators, but that's not common in practice and I think Gecko does something similar. 

So for now in this bug, I think we should just rewrite things that way:

1) If none of the child is a horizontal stretchy operators, then just call layoutIfNeeded() on children and exit.
2) If there is at least one horizontal stretchy operator, then
   * reset the stretch size for all of them and force them to relayout.
   * call layoutIfNeeded on other children.
   * calculate the target width as the maximum of all child->logicalWidth() [note that at this point, the operators are unstretched]
   * call stretchTo on horizontal stretchy operators.
   * force them to relayout again with the correct size [probably you can skip that for direct <mo> children, as you did here].

[Minsheng Liu]

Created attachment 327641 [details]
a corner case

I am still trying to processing your comment and the standard, but I just realize some other corner case. Consider

\over{some short equation}{\over{long horizontal brace}{some long text}}

The brace shall not be stretched wider than the equation, but our algorithm does!
> calculate the target width as the maximum of all child->logicalWidth()
So is the existing implementation in WebKit. By the way, Firefox handles it correct.

[Minsheng Liu]

Created attachment 327642 [details]
a corner case

sorry the previous example contains inappropriate language

[Minsheng Liu]

The standard covers this case:
> should stretch to cover the width of the **other** direct sub-expressions in the given element (or in the same table column), given the constraints mentioned above.

[Frédéric Wang (:fredw)]

(In reply to Minsheng Liu from comment #11)
> Created attachment 327642 [details]
> a corner case
> 
> sorry the previous example contains inappropriate language

Good point. Note that the brace in

<mover>
   <mo>⏞</mo>
   <mrow><mi>k</mi><mspace width="thinmathspace"></mspace><mtext>some long text goes here, really long</mtext></mrow>
</mover>

renders the same width as the long expression in Gecko and WebKit. With,

       <mover>
          <mrow>
            <mi>x</mi>
            <mo>+</mo>
            <mo>...</mo>
            <mo>+</mo>
            <mi>x</mi>
          </mrow>
          <mover>
            <mo>⏞</mo>
            <mrow><mi>k</mi><mspace width="thinmathspace"></mspace><mtext>some long text goes here, really long</mtext></mrow>
          </mover>
        </mover>
 
the brace is embellished and should stretch to cover the maximum width of the "other direct sub-expressions" per the MathML spec. So you are right, we should exclude the width of the inner mover for the stretching of the outer mover. (I think we should actually only include non-stretchy operators but the spec only says that for vertical stretching and the "Skipping the embellished op" in our code says we shouldn't, so I'm not sure...)

With the algorithm I proposed and taking into account "other", the brace will initially have the wide size when laying out the inner mover but then will recover its final size when laying out the outer mover. IIRC, Gecko does something more clever and delay the stretching of the operator until we arrive at the highest level of the embellished op hierarchy for the operator. But maybe that's a bit more difficult to do.

[Minsheng Liu]

Created attachment 327649 [details]
<munderover> with narrow base

Consider this example of <munderover> with a narrow base. I think you would agree that the stretchy operator should not stretch any more than the base, as adopted in the math typesetting convention. So, it is not really “cover the width of the other direct sub-expressions in the given element” but the base sub-expression. Firefox gives the desired result.

If we stretch to base width when necessary, I would say just throw away the for loop and do things manually. How do you think?

[Minsheng Liu]

My proposed algorithm would then be:

1. layout base() and gets its width, which I will refer to as stretchWidth
2. check if under() exists, if so, stretch its embellished operator (if exists) to stretchWidth, and then layout under()
3. check if over() exists, if so, stretch its embellished operator (if exists) to stretchWidth, and then layout over()

The nested case is also handled, consider
\over{base}{\over{brace}{comment}}
By the time we are layouting the inner over, the brace’s logical width has already been set, and we layout brace, which does nothing (Does it change anything, like reverting the logical size to the default? I am unsure of the code base and unfamiliar with the rendering process. Maybe I should take a look at it.), and then we layout the comment part, which might or might not stretch to the brace’s width depending on whether it is an embellished operator for the brace.

[Frédéric Wang (:fredw)]

Using the base as the stretch width won't work for simple case like

<mrow>
  <mi> x </mi>
  <munder>
    <mo> &#x2192;<!--RIGHTWARDS ARROW--> </mo>
    <mtext> maps to </mtext>
  </munder>
  <mi> y </mi>
</mrow>

taken from https://www.w3.org/TR/MathML/chapter3.html#presm.horiz.stretch.rules

I think in order to make you use case work, we really need to consider only  the width of non-stretchy operators (as the spec says for vertical stretching). But again, I wonder about the "Skipping embellished op" comment in the WebKit source.

I'm not sure I understand your comment, but note that the layout is performed bottom-to-top. So we first layout inner expression and then outer expression. Also, if you modify some parts of the rendering tree (via JS, CSS etc) the whole layout is not necessarily performed again, for optimization purpose. So you have to think that operators might be already laid out / stretched and their logical widths set and hence you might need to unstretch them and force a relayout if necessary.

[Minsheng Liu]

Here is my latest solution:

1. layout all sub-expressions and set the stretchWidth to be the maximal logical width
2. stretch base() or the embellished op at base()’s core to stretchWidth
3. stretch under()/over() or the embellished ops at their cores to base()’s logical width

It satisfies and violates the standard in the following way:
* This algorithm’s output only stretches stretchy operators to non-stretchy parts’ maximal width. This can be proven inductively. After step 1, each sub-expression satisfies the invariant so those sub-expressions’ logical width is the same as the maximal width of non-stretchy parts. Subsequent stretching operations only stretch stretchy operators to the maximal width of the logical width of those sub-expressions, i.e. non-stretch parts’ maximal width.
* This algorithm violates the "other direct sub-expressions" condition we discuss earlier. More specifically, it stretches an embellished operator inside base() to the maximal width of not only over()/under()—which are “other direct sub-expressions”—but also base() itself. The motivation is for the following test case:

<mrow>
  <mi> x </mi>
  <mover>
    <munder>
      <mo> &#x2192;<!--RIGHTWARDS ARROW--> </mo>
      <mtext> some long long text </mtext>
    </munder>
    <mtext> maps to </mtext>
  </mover>
  <mi> y </mi>
</mrow>

If we look at the outer <mover>, then the operator should stretch to as wide as the shorter text “maps to”, which I found quite strange. How do you think?

Note that Firefox does not follow my algorithm’s behavior and sticks with the standard in this situation. In any case, my algorithm could be easily changed to follow Firefox’s practice: in step 1, just set the stretchWidth to be max(over()->logicalWidth, under()->logicalWidth()).

<mover>
  <mrow>
    <mi>x</mi>
    <mo>+</mo>
    <mo>...</mo>
    <mo>+</mo>
    <mi>x</mi>
  </mrow>
  <mover>
    <mo>⏞</mo>
    <mtext>some long over text goes here, really long</mtext>
  </mover>
</mover>

Namely, the brace is as wide as the equation but shorter than the text comment.

* For under()/over(), this algorithm violates the "other direct sub-expressions" condition as it really just stretches the embellished operators to the width of the base, the motivation behind which is justified by the attachment “<munderover> with narrow base”.

[Minsheng Liu]

Created attachment 327719 [details]
the algorithm in action (just for visual comparison, not a patch to review)

I add a helper variable to RenderMathMLOperator to lock the stretch width of those operators. The motivation should hopefully be lucid: after stretching the operator and re-layout its ancestors, the recursive call will automatically revert the stretched width, an undesired behavior.

The other trick is that I set stretchWidth to base()’s width in the second loop. It is a bit tricky (I am not sure if such code is welcome here for WebKit) but it reduces code reduplication.

[Frédéric Wang (:fredw)]

Comment on attachment 327719 [details]
the algorithm in action (just for visual comparison, not a patch to review)

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

Thanks for working on this. Good point about the lock/unlock. I've added comments to make it closer to the MathML spec / Firefox and less hacky. Can you please try it, test with the various testcases and run the MathML tests?

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:194
> +    

This should be setStretchWidthLocked(bool stretchWidthLocked) const { m_isStretchedWidthLocked = stretchWidthLocked; }.

Also setStretchWidthLocked and isStretchWidthLocked are small getter/setter so they can just be moved into the *.h file.

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:55
> +    ASSERT(isValid());

Can you also repeat the ASSERT(needsLayout()) here? So that we are sure that the extra work in this function only happens when layout is needed.

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:56
> +    

You might also consider introducing a helper function:

RenderOperator* toHorizontalStretchyOperator(RenderBox* box)
{
    if (is<RenderMathMLBlock>(box)) {
        if (auto renderOperator = downcast<RenderMathMLBlock>(*box).unembellishedOperator()) {
            if (renderOperator->isStretchy() && !renderOperator->isVertical())
                return renderOperator;
        }
    }
    return nullptr;   
}

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:-65
> -                    }

Can you skip non-stretchy operators with?

if (toHorizontalStretchyOperator(child))
    continue;

This is copying the vertical stretchy rule "of the *non-stretchy* direct sub-expressions" of 3.2.5.8.2, but for horizontal stretching. That will cover the case of stretchy under/over and a non-stretchy base.

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:62
> +    

I think you should first count the number of children satisfying toHorizontalStretchyOperator(child). If it is < number of children then you run the loop above and can even exit just after if it is == 0. It is is == number of children then you can instead do:

for (auto* child = firstChildBox(); child; child = child->nextSiblingBox()) {
    auto renderOperator = toHorizontalStretchyOperator(child);
    ASSERT(renderOperator);
    renderOperator->resetStretchSize();
    renderOperator->setStretchWidthLocked(true);
    child->setNeedsLayout();
    child->layout();
    renderOperator->setStretchWidthLocked(false);
    stretchWidth = std::max(stretchWidth, child->logicalWidth());
}

This will address the case "Skipping the embellished op does not work for nested structures like <munder><mover><mo>_</mo>...</mover> <mo>_</mo></munder>" where elements should stretch to the max unstretched size I guess.

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:66
> +                if (renderOperator->isStretchy() && !renderOperator->isVertical() && !renderOperator->isStretchWidthLocked()) {

You can use

if (auto renderOperator == toHorizontalStretchyOperator()) {
    if (!renderOperator->isStretchWidthLocked()) {

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:-74
> -        stretchWidth = std::max(stretchWidth, child->logicalWidth());

Maybe move the comment in the loop I mentioned above.

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:80
> +        stretchWidth = firstChildBox()->logicalWidth();

Then you don't need this hack anymore, as it is again addressed above.

[Minsheng Liu]

While I am working on the rest of your review, could I ask a simple stupid question: for your suggested helper function toHorizontalStretchyOperator, where should I put it? As a static member of some class or just a normal function? I plan to just put inside the cpp file, but WebKit’s compiling options force me to declare a prototype. Currently I just have something like

RenderMathMLOperator* toHorizontalStretchyOperator(RenderBox* box);
    
RenderMathMLOperator* toHorizontalStretchyOperator(RenderBox* box)
{
    if (is<RenderMathMLBlock>(box)) {
        if (auto renderOperator = downcast<RenderMathMLBlock>(*box).unembellishedOperator()) {
            if (renderOperator->isStretchy() && !renderOperator->isVertical())
                return renderOperator;
        }
    }
    return nullptr;
}


But that seems stupid…What is the common practice regarding those types of helper functions?

[Frédéric Wang (:fredw)]

(In reply to Minsheng Liu from comment #20)
> But that seems stupid…What is the common practice regarding those types of
> helper functions?

mmh, you should not need do declare the prototype. See for example

static bool shouldScaleColumnsForParent(const RenderTable& table)

in Source/WebCore/rendering/AutoTableLayout.cpp. Otherwise, you can also declare it as a private function member, but that does not seem necessary here.

[Minsheng Liu]

I am unsure about your stance on the following case:

<math display="block">
<mrow>
  <munder><mover><mo>→</mo><mtext>hello world</mtext></mover> <mo>⏞</mo></munder>
</mrow>
</math>

\under{\over{\arrow}{hello world}}{\topbrace}

According to the standard, the \arrow will stretch to the maximal width of *other* sub-expressions. And since the only other sub-expression is the \topbrace, we stretch \arrow to the un-stretchy size of \topbrace. Firefox obeys the standard on this particular case (you can check it out yourself). I do not find that result intuitive/appealing though. What do you think?

[Frédéric Wang (:fredw)]

(In reply to Minsheng Liu from comment #22)
> I am unsure about your stance on the following case:
> 
> <math display="block">
> <mrow>
>   <munder><mover><mo>→</mo><mtext>hello world</mtext></mover>
> <mo>⏞</mo></munder>
> </mrow>
> </math>
> 
> \under{\over{\arrow}{hello world}}{\topbrace}
> 
> According to the standard, the \arrow will stretch to the maximal width of
> *other* sub-expressions. And since the only other sub-expression is the
> \topbrace, we stretch \arrow to the un-stretchy size of \topbrace. Firefox
> obeys the standard on this particular case (you can check it out yourself).
> I do not find that result intuitive/appealing though. What do you think?

I'm not exactly sure, but I think this falls into "maximum of the normal unstretched sizes of all elements in the containing object" (3.2.5.8.4). What's the unstretched size of the embellished op \over{\arrow}{hello world} is not clear (it may or may not include the width of "hello world").

Let's first try with the changes I proposed and see if that break regression tests or concrete use cases. For edge cases, we could ask on the Math WG mailing list.

[Minsheng Liu]

I have adopted the first part of your change of layout-ing sub-expressions with their stretchy operators’ widths set to zero. However, due to the <munderover> with narrow base case, the hacky last line cannot be removed. My code now looks like this:

void RenderMathMLUnderOver::stretchHorizontalOperatorsAndLayoutChildren()
{
    // The function will be called either "freshly"
    // or as a fix after an embellished operator is stretched.
    ASSERT(isValid());
    ASSERT(needsLayout());
    
    LayoutUnit stretchWidth = 0;
    // The loop layouts each sub-expression's width with
    // embellished stretchy operators' width set to 0,
    // unless the stretchy operators' stretch widths have been locked.
    for (auto* child = firstChildBox(); child; child = child->nextSiblingBox()) {
        auto renderOperator = toHorizontalStretchyOperator(child);
        if (renderOperator) {
            if (!renderOperator->isStretchWidthLocked())
                renderOperator->resetStretchSize();
            renderOperator->setStretchWidthLocked(true);
            // This step is necessary if the function is called as a fix,
            // since the embellished operator could be nested down deeply.
            // Each direct ancestor of the operator shall be re-layout-ed.
            child->setNeedsLayout();
        }
        child->layoutIfNeeded();
        if (renderOperator) {
            // Note that during the fix the embellished operator might be locked
            // and unlocked multiple times, so I change the implementation of the lock
            // to a counter.
            renderOperator->setStretchWidthLocked(false);
        }

        // Only base() will stretch to the maximum of all other elements,
        // but the spec requires base() to stretch to the maximum width of elements other than the base().
        // A decision is needed.
        // The behavior is not consistent with Firefox.
        stretchWidth = std::max(stretchWidth, child->logicalWidth());
    }
    
    for (auto* child = firstChildBox(); child; child = child->nextSiblingBox()) {
        if (auto renderOperator = toHorizontalStretchyOperator(child)) {
            if (!renderOperator->isStretchWidthLocked()) {
                renderOperator->resetStretchSize();
                renderOperator->stretchTo(stretchWidth);
            
                renderOperator->setStretchWidthLocked(true);
                child->setNeedsLayout();
                child->layout();
                renderOperator->setStretchWidthLocked(false);
            }
        }
        
        // For children other than base(), the stretchWidth is the width of base().
        // This is for <munderover> in which we have braces on the top and the bottom.
        // The braces should be stretched as wide as the base(), not lonoger or shorter.
        // The behavior is consistent with Firefox.
        stretchWidth = firstChildBox()->logicalWidth();
    }
}


And it causes no regression other than the expected one. I will provide a review-ready patch (without tests for now) soon.

[Minsheng Liu]

Created attachment 327767 [details]
a new algorithm

[Frédéric Wang (:fredw)]

(In reply to Minsheng Liu from comment #24)
> I have adopted the first part of your change of layout-ing sub-expressions
> with their stretchy operators’ widths set to zero. However, due to the
> <munderover> with narrow base case, the hacky last line cannot be removed.

It does not very look like what I proposed and in particular without addressing the "Can you skip non-stretchy operators" comment you won't be able to remove your hack. Anyway, I'll only go back to this tomorrow.

[Minsheng Liu]

I must admit that I am confused with your "skip non-stretchy operators".
The skipping code you gave was:

if (toHorizontalStretchyOperator(child))
    continue;

which seems more like "skip stretchy operators" than non-stretchy ones.
I try to summarize your points (based on my understanding) to make sure that
we are on the same page.

I checked your original algorithm:
> 1) If none of the child is a horizontal stretchy operators, then just call layoutIfNeeded() on children and exit.
> 2) If there is at least one horizontal stretchy operator, then
>    * reset the stretch size for all of them and force them to relayout.
>    * call layoutIfNeeded on other children.
>    * calculate the target width as the maximum of all child->logicalWidth() [note that at this point, the operators are unstretched]
>    * call stretchTo on horizontal stretchy operators.
>    * force them to relayout again with the correct size [probably you can skip that for direct <mo> children, as you did here].

Compare that with your review comment, I think you are suggesting this
algorithm. I would then take your "skip non-stretchy operators" as skipping
those operators in step 2.1. However, in my latest patch, I merely combined
all step 1, 2.1, 2.2, 2.3 into one loop.

In my code, the first loop traverses all children.
By default, layoutIfNeeded is called on each child.
In addition to that, if a child has a stretchy operator (actually, our code
really says that if a child is an embellished horizontal stretchy operator,
more on this later.), then the stretch size is reset and the child is forced
to relayout. I add some protections so that the code work as expected
in recursive cases.

Step 2.4 and 2.5 is just the second loop.

However, I have to point out that this could not really cover the case of
stretchy under/over and a non-stretchy base. You see, fundamentllay speaking,
the issue is that stretch width is different for each child.
If we take the standard literally, the stretch width for some child x is the
maximal width of all children other than x. As a result, I have to set
stretch width to something different for each child in the second loop.

Now, I realize that the vertical stretching case is actually simpler than
our situation. There, the only thing we need to worry about is to let
stretchy operators to cover the whole row. Here, things get complicated.
Operators in the base have to cover the whole element, while
in the case of non-stretchy base and stretchy braces the stretchy brace
must cover the base only. Therefore, I made the following three rules:

1. stretch width for op in base = max(width of base, width of over, widht of under)
   standard says: max(width of over, width of under)
2. stretch width for op in under = width of base
  standard says: max(width of over, width of base)
2. stretch width for op in over = width of base
  standard says: max(width of under, width of base)

New code with other parts removed and no hack:

void RenderMathMLUnderOver::stretchHorizontalOperatorsAndLayoutChildren()
{
    // omitted code

    LayoutUnit stretchWidthForBase = 0;
    // The loop layouts each sub-expression's width with
    // embellished stretchy operators' width set to 0,
    // unless the stretchy operators' stretch widths have been locked.
    for (auto* child = firstChildBox(); child; child = child->nextSiblingBox()) {
        // omitted code

        stretchWidthForBase = std::max(stretchWidthForBase, child->logicalWidth());
    }
    
    LayoutUnit stretchWidthForOthers = firstChildBox()->logicalWidth();
    
    for (auto* child = firstChildBox(); child; child = child->nextSiblingBox()) {
        LayoutUnit stretchWidth = child == firstChildBox() ? stretchWidthForBase : stretchWidthForOthers;
        
        if (auto renderOperator = toHorizontalStretchyOperator(child)) {
            if (!renderOperator->isStretchWidthLocked()) {
                renderOperator->resetStretchSize();
                renderOperator->stretchTo(stretchWidth);

                renderOperator->setStretchWidthLocked(true);
                child->setNeedsLayout();
                child->layout();
                renderOperator->setStretchWidthLocked(false);
            }
        }
    }
}

I do not quite understand the comment "Skipping the embellished op does not work for nested
structures like <munder><mover><mo>_</mo>...</mover> <mo>_</mo></munder>"
but my alogrithm does handle that case well.


I hope I have understood you correctly and explain myself clearly.
If not, could you explain your view, especially those parts I misunderstood you?
Thanks.

And, our code so far, when detecting the strechy operators, merely detects
those embellished. That might be an issue. Consider the following strcutures:

<munder><mover><mtext>...</mtext><mo>_</mo></mover><mo>_</mo></munder>

The inner <mo> is not an embellished operator, so the code will layout it
as if it is not stretchy. When <mo> contains leading/trailing space,
this becomes an issue. If the inner <mo> is not stretched, the outer <mo>
is stretched to the same width of <mtext>. However, since <mo> is not
an embellished operator, the outer <mo> will stretch to <mtext> + leading spaces
for <mo>. Without significant change to the code I cannot handle this case.

[Frédéric Wang (:fredw)]

@Minsheng: I believe things are becoming complicated but we have made some progress. We should keep in mind that the code must be readable, so mixing too many special cases and stretch width computation in the same loop or departing too much from the official spec is not a good idea. Sorry if I added more confusion with my suggestion (BTW, I really meant to skip *stretchy* children). Let's forget what was discussed above for now and focused on a simpler approach, which I believe is closer to Gecko & the spec. What do you think of this:

1) First add isStretchWidthLocked() into the helper function. That way, the algorithm will just treat such operators as non-stretchy and will never reset their stretch size or lock them again (hence you don't need a counter for it). 

static RenderMathMLOperator* toHorizontalStretchyOperator(RenderBox* box)
{
...
            if (renderOperator->isStretchy() && !renderOperator->isVertical() && !renderOperator->isStretchWidthLocked())
                return renderOperator;
...
}

2) Let's calculate the stretch width in one loop. Instead of calculating the size of the whole embellished operator as I suggested, let's take the unstretched size of the <mo> at the core. I think this is what Gecko does and will follow what you like in edge cases like in comment 22. Also, that will render attachment 327649 [details] as we want. Finally, we will not "Skip the embellished op" although I'm still not sure what this comment was about so we may just remove it. Last, but not least this avoids to do the costly lock & relayout.

// The stretch size is calculated by taking the maximum widths of children.
// For stretchy (possibly embellished) operators, the unstretched size of the <mo> operator at the core is considered.
// This allows to satisfy the following rules from the MathML specification:
// a) "cover the width of the other direct sub-expressions in the given element"
// b) "If a stretchy operator is required to stretch, but all other expressions in the containing element [...] are also stretchy, all elements that can stretch should grow to the maximum of the normal unstretched sizes of all elements in the containing object"
LayoutUnit stretchWidth = 0;
Vector<RenderBox*, 3> embellishedOperators;
for (auto* child = firstChildBox(); child; child = child->nextSiblingBox()) {
    if (auto renderOperator = toHorizontalStretchyOperator(child)) {
        renderOperators.append(stretchyOperators);
        renderOperator->resetStretchSize();
        renderOperator->setNeedsLayout();
        renderOperaror->layout();
        stretchWidth = std::max(stretchWidth, renderOperator->logicalWidth());
    } else {
        child->layoutIfNeeded();
        stretchWidth = std::max(stretchWidth, child->logicalWidth());
    }
}

3) Finally, let's stretch the embellished operators in one loop.

// Now that the stretch width has been calculated, let's stretch the (possibly embellished) operators and relayout them.
// Note that we lock the stretch width of the <mo> elements at the core of embellished operators so that the stretch size is not modified during the relayout e.g. in
//
// <munder>
//   <mtext>Base</mtext>
//   <munder>
//      <mtext>LoooongBaaaaase</mtext>
//      <mo>_</mo>
//   </munder>
// </munder>
// 
for (auto& child : embellishedOperators) {
    auto renderOperator = toHorizontalStretchyOperator(child);
    ASSERT(renderOperator);
    renderOperator->stretchTo(stretchWidth);
    renderOperator->setStretchWidthLocked(true);
    child->setNeedsLayout();
    child->layout();
    renderOperator->setStretchWidthLocked(false);
}

Can you please test the code above and tell me the result? I haven't tried it, so I may have made mistakes...

[Minsheng Liu]

Created attachment 327947 [details]
Test file

The test file covers all cases I am concerned with. I put it in a single file with comments to facilitate our communication.

I hate this bug tracker which lacks code highlight, bold text, in-line image, etc. Therefore, I post everything on an public GitHub issue (https://github.com/notcome/free-markdown-previewer/issues/1), so that you would not miss any details.

In case that GitHub bankrupts before Apple does, I copy the whole text below, which should also help you if you wish to do e-mail style citation.

-- BELOW IS MAIN TEXT --

I have two versions ready to present, one is very faithful to your suggestion (but you did make many typos) and the other is closer to my intention. Here is the visual comparison:

![visual comparison](https://i.imgur.com/X61RKYN.png)

On the left is Firefox’s result. On the right is your proposal (with typos fixed). In the middle is the combination of my previous work and your proposal.

Below is my reproduce of your code:

```C++
void RenderMathMLUnderOver::stretchHorizontalOperatorsAndLayoutChildren()
{
    ASSERT(isValid());
    ASSERT(needsLayout());

    LayoutUnit stretchWidth = 0;
    Vector<RenderBox*, 3> embellishedOperators;
    for (auto* child = firstChildBox(); child; child = child->nextSiblingBox()) {
        if (auto renderOperator = toHorizontalStretchyOperator(child)) {
            embellishedOperators.append(child);
            renderOperator->resetStretchSize();
            child->setNeedsLayout();
            child->layout();
            stretchWidth = std::max(stretchWidth, renderOperator->logicalWidth());
        } else {
            child->layoutIfNeeded();
            stretchWidth = std::max(stretchWidth, child->logicalWidth());
        }
    }

    for (auto& child : embellishedOperators) {
        auto renderOperator = toHorizontalStretchyOperator(child);
        ASSERT(renderOperator);
        renderOperator->stretchTo(stretchWidth);
        renderOperator->setStretchWidthLocked(true);
        child->setNeedsLayout();
        child->layout();
        renderOperator->setStretchWidthLocked(false);
    }
}
```

Several problems:

First, I am unsure if you mean `renderOperator->logicalWidth()` or `child->logicalWidth()`. According to the standard and your comment I believe it is the latter. However, it does not affect the output—I cannot make sense of that immediately.

Second, the over-stretched brace issue persists:

<img width="75" alt="2017-11-29 19 20 47" src="https://user-images.githubusercontent.com/2232964/33411423-706aac52-d53a-11e7-9844-329fcb37a7e2.png">

I do not see how “cover the width of the other direct sub-expressions in the given element” could be satisfied with this loop. I am a bit concerned of language here. By “direct sub-expression”, which ways of the follows should it be interpreted?

* Other siblings of the embellished operators.
* Siblings of `<mo>`.

I prefer the former, and here is the original text:
> [...] then it, or the mo element at its core, should stretch to cover the width of the other direct sub-expressions in the given element [...]

I believe “it” refers to the (embellished) operator.

Your second point on “[...] should grow to the maximum of the normal unstretched sizes of all elements in the containing object” is well-acknowledged and reflected already in yesterday’s code.

Third, look at the bottom of the screenshot, namely those nested cases:

<img width="148" alt="2017-11-29 19 27 51" src="https://user-images.githubusercontent.com/2232964/33411611-759ac2f6-d53b-11e7-9e70-ec05b60d5984.png">

You could see that the inner arrow is a bit off-center. The reason is that your proposal does not lock `stretchWidth` in the first loop:

```C++
for (auto* child = firstChildBox(); child; child = child->nextSiblingBox()) {
    if (auto renderOperator = toHorizontalStretchyOperator(child)) {
        embellishedOperators.append(child);
        renderOperator->resetStretchSize();
        child->setNeedsLayout();
        child->layout();
        stretchWidth = std::max(stretchWidth, renderOperator->logicalWidth());
    } else {
        child->layoutIfNeeded();
        stretchWidth = std::max(stretchWidth, child->logicalWidth());
    }
}
```

Therefore, when the recursive call (on the inner structure) enters the second loop, the `<mo>` is actually stretched. How does that make the arrow off-center is hard to explain, but I believe we want `<mo>` to be unstretched the whole time, so I take the liberty to add the following code:

```C++
for (auto* child = firstChildBox(); child; child = child->nextSiblingBox()) {
    if (auto renderOperator = toHorizontalStretchyOperator(child)) {
        embellishedOperators.append(child);
        renderOperator->resetStretchSize();
        renderOperator->setStretchWidthLocked(true);
        child->setNeedsLayout();
        child->layout();
        stretchWidth = std::max(stretchWidth, renderOperator->logicalWidth());
        renderOperator->setStretchWidthLocked(false);
    } else {
        child->layoutIfNeeded();
        stretchWidth = std::max(stretchWidth, child->logicalWidth());
    }
}
```

This way, the lock has to be a counter. There is still an issue. In the second loop, during the fix process, the embellished operator will be called again to re-layout after stretching. You should agree that it is the first loop in the recursive call that do the re-layout. However, since the embellished operator has its core locked, in the loop it will actually call `layoutIfNeeded()`, which does nothing. Therefore, it needs to be changed:

```C++
for (auto* child = firstChildBox(); child; child = child->nextSiblingBox()) {
    if (auto renderOperator = toHorizontalStretchyOperator(child)) {
        embellishedOperators.append(child);
        renderOperator->resetStretchSize();
        renderOperator->setStretchWidthLocked(true);
        child->setNeedsLayout();
        child->layout();
        renderOperator->setStretchWidthLocked(false);
    } else {
        child->setNeedsLayout();
        child->layout();
    }
    stretchWidth = std::max(stretchWidth, child->logicalWidth());
}
```

Now we return to the over-stretched brace:

<img width="75" alt="2017-11-29 19 20 47" src="https://user-images.githubusercontent.com/2232964/33411423-706aac52-d53a-11e7-9844-329fcb37a7e2.png">

As I said, I do not think by manipulating the for-loop could get us anywhere. The key is that the stretch width is different for base, under, and over. As I said before, I think the reasonable way to set stretch width is:
1. `base.stretchWidth = max(over.width, under.width, base.width)`
2. `under.stretchWidth = base.width`
3. `over.stretchWidth = base.width`

Recall that it is different from the standard:
1. `base.stretchWidth = max(over.width, under.width)`
2. `under.stretchWidth = max(base.width, over.width)` 
3. `over.stretchWidth = max(base.width, under.width)` 

And Firefox adopts:
1. `base.stretchWidth = max(over.width, under.width, base.width)`
2. `under.stretchWidth = max(base.width, over.width)` 
3. `over.stretchWidth = max(base.width, under.width)` 

Check out my test file (as attachment) and previous comments (which should be unnecessary as my test file should be complete)  for my motivation.

Combined together gives the final code:

```C++
void RenderMathMLUnderOver::stretchHorizontalOperatorsAndLayoutChildren()
{
    ASSERT(isValid());
    ASSERT(needsLayout());

    LayoutUnit stretchWidthForBase = 0;
    Vector<RenderBox*, 3> embellishedOperators;
    for (auto* child = firstChildBox(); child; child = child->nextSiblingBox()) {
        if (auto renderOperator = toHorizontalStretchyOperator(child)) {
            embellishedOperators.append(child);
            renderOperator->resetStretchSize();
            renderOperator->setStretchWidthLocked(true);
            child->setNeedsLayout();
            child->layout();
            renderOperator->setStretchWidthLocked(false);
        } else {
            child->setNeedsLayout();
            child->layout();
        }
        stretchWidthForBase = std::max(stretchWidthForBase, child->logicalWidth());
    }
    LayoutUnit stretchWidthForOthers = firstChildBox()->logicalWidth();

    for (auto& child : embellishedOperators) {
        auto stretchWidth = child == firstChildBox() ? stretchWidthForBase : stretchWidthForOthers;
        auto renderOperator = toHorizontalStretchyOperator(child);
        ASSERT(renderOperator);
        renderOperator->stretchTo(stretchWidth);
        renderOperator->setStretchWidthLocked(true);
        child->setNeedsLayout();
        child->layout();
        renderOperator->setStretchWidthLocked(false);
    }
}
```

Two things to point out. First, in the first loop, else branch, the force re-layout is EXPENSIVE. The only  way to be more efficient, as I can think of, is to handle the following three cases differently:
* unlocked embellished stretchy operator, lock and force layout (top-level call, initial layout)
* locked embellished stretchy operator, lock and force layout (we are fixing the layout!)
* other cases, layout if necessary

That is exactly what my yesterday’s code does, though that code is shitty. We could discuss how to get it done more elegantly later.

The other pressing matter, as I mentioned in the test file, is the following case:

<img width="100" alt="2017-11-29 19 45 35" src="https://user-images.githubusercontent.com/2232964/33412040-0715d944-d53e-11e7-85cc-10ddeb7d97a7.png">

> In all our patches, when the arrow is stretched to the width of the text, its surrounding space makes the base acutally wider than text. So we would observe the brace to be slightly wider. The issue is not present (thus a regression) because in the past the logical width of that arrow is unstretched, and the logical width of the base would be that of the text. Firefox is not affected.

(Copied from my test file.)

That is all I have to say.

[Frédéric Wang (:fredw)]

(In reply to Minsheng Liu from comment #29)
>             renderOperator->resetStretchSize();
>             child->setNeedsLayout();
>             child->layout();
>             stretchWidth = std::max(stretchWidth,
> renderOperator->logicalWidth());

Note that I said "Instead of calculating the size of the whole embellished operator as I suggested, let's take the unstretched size of the <mo> at the core".  So I really meant renderOperator->setNeedsLayout(), renderOperator->layout() and renderOperator->logicalWidth(). No "child" should appear here.

> First, I am unsure if you mean `renderOperator->logicalWidth()` or
> `child->logicalWidth()`. According to the standard and your comment I
> believe it is the latter. However, it does not affect the output—I cannot
> make sense of that immediately.

I really meant renderOperator.

> Second, the over-stretched brace issue persists:

Indeed because you layout the whole child instead of just the <mo> at the core, so the renderOperator will be stretched in the first loop.

> I do not see how “cover the width of the other direct sub-expressions in the
> given element” could be satisfied with this loop. I am a bit concerned of
> language here. By “direct sub-expression”, which ways of the follows should
> it be interpreted?
> 
> * Other siblings of the embellished operators.
> * Siblings of `<mo>`.

I think "direct sub-expression" mean children of the <munderover>. However, the trick here is to consider that the width of (possibly embellished) operators is the unstretched size of the <mo> at the core as Gecko does.

> You could see that the inner arrow is a bit off-center. The reason is that
> your proposal does not lock `stretchWidth` in the first loop:

You must not lock the stretchWidth in the first loop. Again, I'm only proposing to layout the renderOperator at the core, not the whole child.

[Minsheng Liu]

How should I get a RenderBox out of a RenderBlock? It told me that layout is a protected member of RenderBlock (and hence RenderMathMLOperator).

[Minsheng Liu]

Created attachment 327975 [details]
your algorithm’s result

So I just re-interpret the pointer. There should be some other safer way to do it.

void RenderMathMLUnderOver::stretchHorizontalOperatorsAndLayoutChildren()
{
    ASSERT(isValid());
    ASSERT(needsLayout());

    LayoutUnit stretchWidth = 0;
    Vector<RenderBox*, 3> embellishedOperators;
    for (auto* child = firstChildBox(); child; child = child->nextSiblingBox()) {
        if (auto renderOperator = toHorizontalStretchyOperator(child)) {
            embellishedOperators.append(child);
            renderOperator->resetStretchSize();
            auto* operatorBox = (RenderBox*)renderOperator;
            operatorBox->setNeedsLayout();
            operatorBox->layout();
            stretchWidth = std::max(stretchWidth, operatorBox->logicalWidth());
        } else {
            child->layoutIfNeeded();
            stretchWidth = std::max(stretchWidth, child->logicalWidth());
        }
    }

    for (auto& child : embellishedOperators) {
        auto renderOperator = toHorizontalStretchyOperator(child);
        ASSERT(renderOperator);
        renderOperator->stretchTo(stretchWidth);
        renderOperator->setStretchWidthLocked(true);
        child->setNeedsLayout();
        child->layout();
        renderOperator->setStretchWidthLocked(false);
    }
}

In any case, why this should fail about complicated case is not hard to see. You cannot get around the variable stretchWidth issue.

[Frédéric Wang (:fredw)]

(In reply to Minsheng Liu from comment #31)
> How should I get a RenderBox out of a RenderBlock? It told me that layout is
> a protected member of RenderBlock (and hence RenderMathMLOperator).

Mmmh, can't you just do

RenderBox* renderBox = renderOperator;
renderBox->...
renderBox->...

or

static_cast<RenderBox*>(renderOperator)->...

?

[Frédéric Wang (:fredw)]

(In reply to Minsheng Liu from comment #32)
> In any case, why this should fail about complicated case is not hard to see.
> You cannot get around the variable stretchWidth issue.

What about the more costly option to force a relayout of the children in the first loop?

[zalan]

(In reply to Frédéric Wang (:fredw) from comment #33)
> (In reply to Minsheng Liu from comment #31)
> > How should I get a RenderBox out of a RenderBlock? It told me that layout is
> > a protected member of RenderBlock (and hence RenderMathMLOperator).
> 
> Mmmh, can't you just do
> 
> RenderBox* renderBox = renderOperator;
> renderBox->...
> renderBox->...
> 
> or
> 
> static_cast<RenderBox*>(renderOperator)->...
> 
> ?
layoutIfNeeded() (optionally layoutBlock())

[Minsheng Liu]

> What about the more costly option to force a relayout of the children in the first loop?

I have detailed of that in the GitHub issue and lengthy reply above (https://github.com/notcome/free-markdown-previewer/issues/1). Plus I gave you a step-by-step derivation of my algorithm which does handle all cases correctly.

> layoutIfNeeded() (optionally layoutBlock())

Thanks.

Anyway, I want to reiterate the fact that the more pressing matter is the following case:

<math display="block">
  <mrow>
    <mover>
      <munder>
        <mtext>long text</mtext>
        <mo>&#x2192;</mo>
      </munder>
      <mo>⏞</mo>
    </mover>
  </mrow>
</math>

The arrow (&#x2192;) is not an embellished operator. Its surrounding lspace/rspace will make the brace a bit wider than it should be. The correct response to this would be nontrivial (involving multiple methods and potentially a new virtual one finding all operators and underovers), and we really should discuss this case now.

[Minsheng Liu]

To handle the latest edge case I provided, I think we really could benefit a lot from a logicalWidthWithoutStretchyOperators() virtual method. With that, we could have something like:

for (child : children)
  layout child if needed

this->m_logicalWidthWithoutStretchyOperator = max(children’s)

for (child : children that are embellished operators)
  let operator = ...
  stretch operator
  fixLayout(parent node = child, bad node = operator)

layout children
set logical width
et cetera 

This way we can combine the method we are working on with layoutBlock.

Also I suspect our existing fixLayout (setNeedsLayout() then layout()) is problematic for cases like an operator embellished with multiple subscript/superscript.

— Sent from my iPhone.

[Minsheng Liu]

On the one hand, it is much harder than I thought to keep a logicalWidthWithoutStretchyOperators. On the other hand, it seems that space surrounding an operator shall only be considered when it is in a <mrow>, as the spec says:

> The amount of horizontal space added around an operator (or embellished operator), when it occurs in an mrow, can be directly specified by the lspace and rspace attributes.

Therefore, the edge case is really beyond the scope of this patch, and I shall open a new bug handling it. If that is the case, then my code has properly handled all cases, with the additional flexibility to adapt to different interpretation of stretch widths fairly easily. Once we make up decision on that front, I suggest we freeze the discussion on the layout algorithm and enters the next step.

In the mean time, I will convert the manual test file into a JavaScript based test file that compares stretchy operators’ widths. I will also try to add a performance test, but I might need some help from you guys.

So again, @fred, out of the following three definitions, which one should we put into this patch:

// my proposal, it is the middle column of this picture: https://camo.githubusercontent.com/606494a142955188301373ca36780293faa669d7/68747470733a2f2f692e696d6775722e636f6d2f583631524b594e2e706e67

base.stretchWidth = max(over.width, under.width, base.width)
under.stretchWidth = base.width
over.stretchWidth = base.width

// the standard:
base.stretchWidth = max(over.width, under.width)
under.stretchWidth = max(base.width, over.width)
over.stretchWidth = max(base.width, under.width)

// Firefox: it is the left column of this picture: https://camo.githubusercontent.com/606494a142955188301373ca36780293faa669d7/68747470733a2f2f692e696d6775722e636f6d2f583631524b594e2e706e67
base.stretchWidth = max(over.width, under.width, base.width)
under.stretchWidth = max(base.width, over.width)
over.stretchWidth = max(base.width, under.width)

I do not think we should wait for the committee, as again in the future we could change our decision fairly easily, but future patches should crucially rely on a sensible logical bounds of <munderover>.

(In reply to Minsheng Liu from comment #37)
> To handle the latest edge case I provided, I think we really could benefit a
> lot from a logicalWidthWithoutStretchyOperators() virtual method. With that,
> we could have something like:
> 
> for (child : children)
>   layout child if needed
> 
> this->m_logicalWidthWithoutStretchyOperator = max(children’s)
> 
> for (child : children that are embellished operators)
>   let operator = ...
>   stretch operator
>   fixLayout(parent node = child, bad node = operator)
> 
> layout children
> set logical width
> et cetera 
> 
> This way we can combine the method we are working on with layoutBlock.
> 
> Also I suspect our existing fixLayout (setNeedsLayout() then layout()) is
> problematic for cases like an operator embellished with multiple
> subscript/superscript.
> 
> — Sent from my iPhone.

[Frédéric Wang (:fredw)]

Created attachment 328081 [details]
Tetscase with different widths

Thanks for the perseverance on this bug. For your information, I'm working on other projects right now so obviously I don't have time to read and test things in details or to reply to each C++ build error, sorry. I'm really willing to help to provide guideline and review MathML patches though, so please continue your effort!

As indicated on the WebKit wiki, the rule of thumb to get changes accepted more easily is to try and split the work in small tasks so that it's easier for people to understand and to avoid breaking things. Also as I said you can also start with smaller bugs like bug 158937, bug 180029 or bug 161306 to get more familiar with C++ and WebKit code/process (BTW, check Tools/Script/webkit-patch --help to upload a patch without asking for review).

Regarding this bug, the initial goal was to fix the logical width of stretched operators in munderover. You are right that things are getting more complicated that I thought with embellished operators (they are actually not completely implemented yet, see bug 124831 ; Gecko has bugs too) so maybe you should not spend too much time thinking on it. By far, the most important use cases is when there are no embellished operators (row 1 in the attached test case) and in that case the behavior is clear: They should stretch to the maximum width of base/under/over and this is how the spec & Gecko behave and how WebKit must remain. In practice, there is one non-stretchy element giving the width to use so the things in the spec ("other", "unstretched size") or even skipping stretchy operators (as for vertical operators) does not really affect the value of the stretch width.

When we have embellished operators, one has to include them in the width calculation (this is the comment "Skipping the embellished op..." in WebKit code) and then the spec is not really clear about how to calculate their unstretched width (do we include unstretched <mo> width? embellishement width?) and their actual stretch width (e.g. Gecko ignores embellishments and just stretch the <mo> at the core to the target size as you can see in the testcase). Again, I think these use cases are rare and in general embellishments are small (row 2 and 3 in the testcase). So I think we have some freedom on what to do here, for example if we want to cover the use cases of horizontal braces with some long text labels. However, we should try to keep the code 1) Simple and readable 2) Close to the spec, Gecko and to the base case with no embellishments 3) Efficient, especially for the base case.

That's all I can say for now, I hope this is helpful.

[Minsheng Liu]

Thanks for your patience. I plan the following two things:

1. I will summarize a patch with test cases so that when we look back in the future we have something to work/test against.

Note that some part of my code is complicated (like the nested lock structure) and expensive is due to the way we fix layout after re-stretching. I have a new method in my mind that should make everyone satisfied in terms of code efficiency and clarity.

2. If you have not done so, I will send a summary of some edge cases that the MathML 3 standard fails to clarify to their mailing list.

Both will not be done before next week though, as some schoolwork is piling up.

Thanks.

[Minsheng Liu]

Created attachment 328250 [details]
Patch

[Darin Adler]

Comment on attachment 328250 [details]
Patch

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

I’d love to say r=me, great that this includes a test and it seems logical, but I am concerned about the setNeedsLayout/layout thing done here.

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:73
> +    auto* fixingBox = static_cast<RenderBox*>(stretchyOperator);
> +    while (fixingBox != ancestor) {
> +        fixingBox->setNeedsLayout();
> +        fixingBox->layout();
> +        fixingBox = fixingBox->parentBox();
> +    }

I would write:

    RenderBox* fixingBox = stretchyOperator;

Instead of writing the static_cast.

I also would write this as a for loop:

    for (RenderBox* box = stretchyOperator; box != ancestor; box = box->parentBox()) {
        box->setNeedsLayout();
        box->layout();
    }

But also, it is really strange to do synchronous calls to layout after setNeedsLayout. We need a layout expert to weigh in on this. It seems wrong. Antti? Zalan?

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:75
> +    ancestor->setNeedsLayout();
> +    ancestor->layout();

Ditto. It seems extremely unlikely that it is OK to do this. Despite the fact that the old code was doing something like this before.

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

That same pattern again.

[Minsheng Liu]

Thanks Darin for commenting! I am completely new to WebKit. I really hope there is some detailed documentation on layout stuff. The only thing I could find is the 2007 blog article (https://webkit.org/blog/116/webcore-rendering-iii-layout-basics/) but the series feels more like a tutorial on how to write a simple browser…

In any case, if some layout expert could take a look at it, I have a question. Since the stretch width of a stretch operator depends on the width of other elements in a fairly complicated way, I am unsure how the code behaves when a stretch-width-affecting element is changed (e.g. by JavaScript). In WebKit, which part of code determines which elements become dirty after DOM change? I have not yet tested the issue as there are other layouting/painting problems regarding DOM change, page size scaling, and scrolling, but I think we should be cautious here.

[Darin Adler]

(In reply to Minsheng Liu from comment #43)
> I really hope there is some detailed documentation on layout stuff.

Sorry,no.

[zalan]

Comment on attachment 328250 [details]
Patch

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

>> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:73
>> +    }
> 
> I would write:
> 
>     RenderBox* fixingBox = stretchyOperator;
> 
> Instead of writing the static_cast.
> 
> I also would write this as a for loop:
> 
>     for (RenderBox* box = stretchyOperator; box != ancestor; box = box->parentBox()) {
>         box->setNeedsLayout();
>         box->layout();
>     }
> 
> But also, it is really strange to do synchronous calls to layout after setNeedsLayout. We need a layout expert to weigh in on this. It seems wrong. Antti? Zalan?

Normally you set the dirty bit on the renderer soon after you figure its position/size are invalid (usually result of a style change), schedule an async layout and here you would just call layoutIfNeeded(). 
However since in this function we are already have a valid layout context  (through RenderMathMLUnderOver::layoutBlock), this looks more like a multi-pass layout where the descendants need to be readjusted after some ancestors/siblings etc layouts are done. I don't know much about MathML so if that's not the case, then the setNeedsLayout calls are wrong here and they need to be moved somewhere else (see above). 
Even if it was a type of multi-pass layout, the stretch operation should probably dirty the descendants (since that's where the children get invalid) and you'd just call layoutIfNeeded here.

[Minsheng Liu]

(In reply to zalan from comment #45)
> Comment on attachment 328250 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=328250&action=review
> 
> >> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:73
> >> +    }
> > 
> > I would write:
> > 
> >     RenderBox* fixingBox = stretchyOperator;
> > 
> > Instead of writing the static_cast.
> > 
> > I also would write this as a for loop:
> > 
> >     for (RenderBox* box = stretchyOperator; box != ancestor; box = box->parentBox()) {
> >         box->setNeedsLayout();
> >         box->layout();
> >     }
> > 
> > But also, it is really strange to do synchronous calls to layout after setNeedsLayout. We need a layout expert to weigh in on this. It seems wrong. Antti? Zalan?
> 
> Normally you set the dirty bit on the renderer soon after you figure its
> position/size are invalid (usually result of a style change), schedule an
> async layout and here you would just call layoutIfNeeded(). 
> However since in this function we are already have a valid layout context 
> (through RenderMathMLUnderOver::layoutBlock), this looks more like a
> multi-pass layout where the descendants need to be readjusted after some
> ancestors/siblings etc layouts are done. I don't know much about MathML so
> if that's not the case, then the setNeedsLayout calls are wrong here and
> they need to be moved somewhere else (see above). 
> Even if it was a type of multi-pass layout, the stretch operation should
> probably dirty the descendants (since that's where the children get invalid)
> and you'd just call layoutIfNeeded here.

So what is the difference between layoutIfNeeded() and layout()? Is it that when layout() is called then a layout is carried out immediately, while when layoutIfNeeded() is called then a layout is not executed until we are doing some actual layout?

Second, this is indeed a multi-pass layout. The case I try to handle is that I am stretching a child deeply nested so after the stretching I need to layout not only the child but also all its ancestors. It is like:

<munder> // we are executing layout here
  <msub>
    <mover>
      <msup>
        <mo>...</mo> // the <mo> being stretched
      </msup>
    </mover>
  </msub>
  ...
</munder>

So if I understand you correctly, I could just set <msub>, <mover>, <msup>, all the way to <mo> dirty, and then call layoutIfNeeded() on <musb>? That does seem better! Thanks!

[Minsheng Liu]

Created attachment 328323 [details]
Patch

[Frédéric Wang (:fredw)]

Comment on attachment 328323 [details]
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 [details]). 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.

[Frédéric Wang (:fredw)]

Comment on attachment 328323 [details]
Patch

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

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:71
> +    ancestor->layoutIfNeeded();

(this comment was lost, so adding it again)

This seems complicated. Isn't it enough to call setNeedsLayout() on the stretchyOperator and layoutIfNeeded() on the direct children of <munderover>?

[Minsheng Liu]

(In reply to Frédéric Wang (:fredw) from comment #49)
> Comment on attachment 328323 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=328323&action=review
> 
> > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:71
> > +    ancestor->layoutIfNeeded();
> 
> (this comment was lost, so adding it again)
> 
> This seems complicated. Isn't it enough to call setNeedsLayout() on the
> stretchyOperator and layoutIfNeeded() on the direct children of <munderover>?

Oh that is the main point of this new patch! Consider the following case:

<munder> // we are executing layout here
  <msub>
    <mover>
      <msup>
        <mo>...</mo> // the <mo> being stretched
      </msup>
    </mover>
  </msub>
  ...
</munder>

When layoutIfNeeded() is called on <msub>, it will find its immediate child <mover> clean, so the recursion will not go down. I have tested and making sure the whole chain from the immediate child to the operator at its core dirty is crucial for correct layout.

Note that this approach only requires minimal re-layout. Essentially, only logicalWidth() and location() are updated. Therefore, I could get rid of my initially complicated first loop and counter-based lock and all other crazy stuff. I think we are optimal here.

[Frédéric Wang (:fredw)]

(In reply to Minsheng Liu from comment #50)
> When layoutIfNeeded() is called on <msub>, it will find its immediate child
> <mover> clean, so the recursion will not go down. I have tested and making
> sure the whole chain from the immediate child to the operator at its core
> dirty is crucial for correct layout.
> 
> Note that this approach only requires minimal re-layout. Essentially, only
> logicalWidth() and location() are updated. Therefore, I could get rid of my
> initially complicated first loop and counter-based lock and all other crazy
> stuff. I think we are optimal here.

Sorry, I was not clear. I mean (quickly looking to the implementation of layoutIfNeeded) it seems marking the <mo> at the core dirty will also already mark the whole ancestor chain and then you just need to layout the direct child.

[Minsheng Liu]

(In reply to Frédéric Wang (:fredw) from comment #51)
> (In reply to Minsheng Liu from comment #50)
> > When layoutIfNeeded() is called on <msub>, it will find its immediate child
> > <mover> clean, so the recursion will not go down. I have tested and making
> > sure the whole chain from the immediate child to the operator at its core
> > dirty is crucial for correct layout.
> > 
> > Note that this approach only requires minimal re-layout. Essentially, only
> > logicalWidth() and location() are updated. Therefore, I could get rid of my
> > initially complicated first loop and counter-based lock and all other crazy
> > stuff. I think we are optimal here.
> 
> Sorry, I was not clear. I mean (quickly looking to the implementation of
> layoutIfNeeded) it seems marking the <mo> at the core dirty will also
> already mark the whole ancestor chain and then you just need to layout the
> direct child.

That will be great (but unintuitive…I clearly need more understanding of those little layout functions). I will double check that.

Other replies:

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

Yeah that is a better idea. I will get it done soon. Note that the RenderTree will be different since the width of the stretchy operator will change. The pixel test should be fine, but let’s figure it out.

> This is wrong as it does not work the base case (no embellishments, first row of attachment 328081 [details]). 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.

I will look into that when I get home.

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

I will conform to whatever WebKit chooses. It does make the structure more clear and easier to modify, at least in the current iterative process.

[Frédéric Wang (:fredw)]

(In reply to Minsheng Liu from comment #52)
> Yeah that is a better idea. I will get it done soon. Note that the
> RenderTree will be different since the width of the stretchy operator will
> change. The pixel test should be fine, but let’s figure it out.
> 

Right, the change in the render tree should not be a problem, that's a fix :-) That's why I think it's better to do that step in a preliminary patch so we clearly see the diff.

But now that you mention it, I believe we can't do bug 180351 without fixing the logical width first.

[Minsheng Liu]

Created attachment 328436 [details]
Patch

[Minsheng Liu]

I have made some changes to the layout algorithm and the resulting one is more standard conforming and should behave the same as Firefox does. As for the fix layout algorithm, @fred you are right that we just need to dirty the op at the core and layoutIfNeeded the outermost wrapper for the embellished op.

As for the tests, I add one simple case that the previous version fails in the script test. The pixel test is also updated, and I believe the visual results are correct for both macOS and iOS. As for expected results for GTK and Win, I cannot generate them so I delete them. Could you help me get the desired results? Or could we let the bot do that for us?

[EWS Watchlist]

Comment on attachment 328436 [details]
Patch

Attachment 328436 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5496501

New failing tests:
mathml/opentype/munderover-stretch-width.html

[EWS Watchlist]

Created attachment 328439 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

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

[Minsheng Liu]

Stupid…I added a test case but forgot to update the expected file.

[Minsheng Liu]

Created attachment 328440 [details]
Patch

[Frédéric Wang (:fredw)]

*** Bug 180351 has been marked as a duplicate of this bug. ***

[Frédéric Wang (:fredw)]

Comment on attachment 328440 [details]
Patch

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

@Minsheng Liu: OK, I think this looks much better now! However you did not address/reply to some of my previous comments and especially the fact that the stretch operator remains locked in fixLayoutAfterStretch. We are close I guess I can r+ in the next iteration once all the comments are addressed.

Regarding the GTK / Windows, you don't have to worry about it for now, we can land the patch as it and let the port maintainers update the expectations (or ask them to do it). You can also retrieve them from https://build.webkit.org/console after the patch lands and submit a follow-up patch.

> Source/WebCore/ChangeLog:20
> +        mathml/opentype/opentype-stretchy-horizonta.html

nit: horizontal

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

nit: argument name should be "stretchWidthLocked"

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

I'm still not sure why this is not stretchyOperator->setStretchWidthLocked(false) ?

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

Does it work if you rewrite this

stretchyOperator->resetStretchSize();
fixLayoutAfterStretch(child, stretchyOperator);

?

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:101
> +    }

Ah OK, this was the trick! That makes the code much more readable and understandable indeed. I think it would be great to add a C++ comment at the top of the function to explain the algorithm and rationale (comparing with the spec & Gecko maybe) since as you see we took some time to figure it out.

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:103
> +    for (std::size_t i = 0; i < embellishedOperators.size(); i ++) {

nit 1: do you really need the namespace prefix for size_t?
nit 2: there should be no space between i and ++.

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

I really think it would be nice to have more tests for the base case. But I guess it's ok to do that in a separate patch.

> LayoutTests/mathml/opentype/munderover-stretch-width.html:52
> +        runCase(8, 'simpe stretchy under should equal over', 'red', 'op');

s/simpe/simple/

[Minsheng Liu]

Created attachment 328775 [details]
Patch

[Frédéric Wang (:fredw)]

Comment on attachment 328775 [details]
Patch

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

Thanks, I have some minor comments below but otherwise LGTM. It looks like wincairo failed, you should probably consider rebasing your patch against the latest upstream revision.

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:181
> +    

nit: please remove the trailing whitespace.

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:63
> +    

nit: please remove the trailing whitespace.

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:75
> +    ASSERT(needsLayout());

Can you please add the following comment:

// We apply horizontal stretchy rules from the MathML spec (sections 3.2.5.8.3 and 3.2.5.8.4), which 
// can be roughly summarized as "stretching operators to the maximum widths of all children" and
// minor variations of that algorithm do not affect the result. However, the spec is a bit ambiguous
// for embellished operators (section 3.2.5.7.3) and different approaches can lead to significant
// stretch size differences. We made the following decisions:
// - The unstretched size is the embellished operator width with the <mo> at the core unstretched.
// - In general, the target size is just the maximum widths of non-stretchy children because the
//   embellishments could make widths significantly larger.
// - In the edge case when all operators of stretchy, we follow the specification and take the
//   maximum of all unstretched sizes.
// - The <mo> at the core is stretched to cover the target size, even if the embellished operator
//   might become much wider.

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:76
> +    

nit: trailing whitespace.

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:81
> +    

nit: trailing whitespace

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:87
> +            fixLayoutAfterStretch(child, stretchyOperator);

It is a bit sad that we have to do this relayout when we actually only measure the unstretched size for the edge case isAllStretchyOperators == true. Are you able to move that fixLayoutAfterStretch call into the isAllStretchyOperators conditional?

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:94
> +    

nit: trailing whitespace

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:99
> +    

nit: trailing whitespace

[zalan]

Comment on attachment 328775 [details]
Patch

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

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:53
> +static RenderMathMLOperator* toHorizontalStretchyOperator(RenderBox* box)

const RenderBox& box (unembellishedOperator should really be a const function)

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:55
> +    if (is<RenderMathMLBlock>(box)) {

in WebKit, we tend to use early returns instead
if (!is<RenderMathMLBlock>(box))
    return nullptr;

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:56
> +        if (auto renderOperator = downcast<RenderMathMLBlock>(*box).unembellishedOperator()) {

if (auto* renderOperator

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:64
> +static void fixLayoutAfterStretch(RenderBox* ancestor, RenderMathMLOperator* stretchyOperator)

RenderBox& ancestor, RenderMathMLOperator& stretchyOperator

[Minsheng Liu]

First a disclaimer, I am not quite familiar with VCS. I notice that wincairo’s result says:

> Original content of LayoutTests/platform/ios/mathml/opentype/opentype-stretchy-horizontal-expected.png mismatches at C:/WebKit-EWS/WebKit\Tools\Scripts\svn-apply line 272.

And in fact that all previous patches did not apply to wincairo for the exact same reason. I am quite confused because I think all those platforms share the same branch, right? How could the patch apply to one platform but fail on the other?

Also, I am unsure if I am doing the patch creating workflow correctly. Every time I submit a new patch, I have to first revert my change to the ChangeLog, do a `git stash` to save my current work, update through the script, `git stash apply` to restore my work, and then manually fixing ChangeLog. Is there a way to (partially) automate that?

Thanks!

(In reply to Frédéric Wang (:fredw) from comment #63)
> Comment on attachment 328775 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=328775&action=review
> 
> Thanks, I have some minor comments below but otherwise LGTM. It looks like
> wincairo failed, you should probably consider rebasing your patch against
> the latest upstream revision.
> 
> > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:181
> > +    
> 
> nit: please remove the trailing whitespace.
> 
> > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:63
> > +    
> 
> nit: please remove the trailing whitespace.
> 
> > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:75
> > +    ASSERT(needsLayout());
> 
> Can you please add the following comment:
> 
> // We apply horizontal stretchy rules from the MathML spec (sections
> 3.2.5.8.3 and 3.2.5.8.4), which 
> // can be roughly summarized as "stretching operators to the maximum widths
> of all children" and
> // minor variations of that algorithm do not affect the result. However, the
> spec is a bit ambiguous
> // for embellished operators (section 3.2.5.7.3) and different approaches
> can lead to significant
> // stretch size differences. We made the following decisions:
> // - The unstretched size is the embellished operator width with the <mo> at
> the core unstretched.
> // - In general, the target size is just the maximum widths of non-stretchy
> children because the
> //   embellishments could make widths significantly larger.
> // - In the edge case when all operators of stretchy, we follow the
> specification and take the
> //   maximum of all unstretched sizes.
> // - The <mo> at the core is stretched to cover the target size, even if the
> embellished operator
> //   might become much wider.
> 
> > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:76
> > +    
> 
> nit: trailing whitespace.
> 
> > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:81
> > +    
> 
> nit: trailing whitespace
> 
> > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:87
> > +            fixLayoutAfterStretch(child, stretchyOperator);
> 
> It is a bit sad that we have to do this relayout when we actually only
> measure the unstretched size for the edge case isAllStretchyOperators ==
> true. Are you able to move that fixLayoutAfterStretch call into the
> isAllStretchyOperators conditional?
> 
> > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:94
> > +    
> 
> nit: trailing whitespace
> 
> > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:99
> > +    
> 
> nit: trailing whitespace

[Darin Adler]

(In reply to Minsheng Liu from comment #65)
> First a disclaimer, I am not quite familiar with VCS. I notice that
> wincairo’s result says:
> 
> > Original content of LayoutTests/platform/ios/mathml/opentype/opentype-stretchy-horizontal-expected.png mismatches at C:/WebKit-EWS/WebKit\Tools\Scripts\svn-apply line 272.
> 
> And in fact that all previous patches did not apply to wincairo for the
> exact same reason. I am quite confused because I think all those platforms
> share the same branch, right? How could the patch apply to one platform but
> fail on the other?

Sounds like there might be problem with "eol" handling. It’s possible that the MIME type isn’t set for this PNG file in Subversion so it is treating it as a text file and trying to do CRLF conversion in some Windows. If so, the fix is for someone to land a patch that sets the property on the PNG file.

> Also, I am unsure if I am doing the patch creating workflow correctly. Every
> time I submit a new patch, I have to first revert my change to the
> ChangeLog, do a `git stash` to save my current work, update through the
> script, `git stash apply` to restore my work, and then manually fixing
> ChangeLog. Is there a way to (partially) automate that?

There is a script called resolve-ChangeLogs that you can set up in your git configuration to make this easier. I can’t find the instructions about how to use it, but someone else on the project probably can point you to the right place.

[Frédéric Wang (:fredw)]

(In reply to Darin Adler from comment #66)
> > Also, I am unsure if I am doing the patch creating workflow correctly. Every
> > time I submit a new patch, I have to first revert my change to the
> > ChangeLog, do a `git stash` to save my current work, update through the
> > script, `git stash apply` to restore my work, and then manually fixing
> > ChangeLog. Is there a way to (partially) automate that?
> 
> There is a script called resolve-ChangeLogs that you can set up in your git
> configuration to make this easier. I can’t find the instructions about how
> to use it, but someone else on the project probably can point you to the
> right place.

It's https://trac.webkit.org/wiki/UsingGitWithWebKit#WebKitScriptsupportforGit

[Minsheng Liu]

(In reply to Frédéric Wang (:fredw) from comment #67)
> (In reply to Darin Adler from comment #66)
> > > Also, I am unsure if I am doing the patch creating workflow correctly. Every
> > > time I submit a new patch, I have to first revert my change to the
> > > ChangeLog, do a `git stash` to save my current work, update through the
> > > script, `git stash apply` to restore my work, and then manually fixing
> > > ChangeLog. Is there a way to (partially) automate that?
> > 
> > There is a script called resolve-ChangeLogs that you can set up in your git
> > configuration to make this easier. I can’t find the instructions about how
> > to use it, but someone else on the project probably can point you to the
> > right place.
> 
> It's
> https://trac.webkit.org/wiki/UsingGitWithWebKit#WebKitScriptsupportforGit

Thanks! So how should we deal with that PNG? I will not be able to touch my iMac for the next few hours, so could you help me resolve that? I will prepare a new patch tonight (ten hours later) that adapt your and zalan’s latest comments.

[Darin Adler]

(In reply to Darin Adler from comment #66)
> Sounds like there might be problem with "eol" handling. It’s possible that
> the MIME type isn’t set for this PNG file in Subversion so it is treating it
> as a text file and trying to do CRLF conversion in some Windows. If so, the
> fix is for someone to land a patch that sets the property on the PNG file.

Hmm, no that theory seems to be wrong. It says "Property svn:mime-type set to image/png" and so I don’t know why. Maybe involved in maintaining the WinCairo EWS bot can investigate?

[Frédéric Wang (:fredw)]

(In reply to Darin Adler from comment #69)
> (In reply to Darin Adler from comment #66)
> > Sounds like there might be problem with "eol" handling. It’s possible that
> > the MIME type isn’t set for this PNG file in Subversion so it is treating it
> > as a text file and trying to do CRLF conversion in some Windows. If so, the
> > fix is for someone to land a patch that sets the property on the PNG file.
> 
> Hmm, no that theory seems to be wrong. It says "Property svn:mime-type set
> to image/png" and so I don’t know why. Maybe involved in maintaining the
> WinCairo EWS bot can investigate?

@Don: Can you please help?

[Don Olmstead]

I’ll ask someone to apply the patch locally to see what happens and if it works we can just cq it.

[Don Olmstead]

Comment on attachment 328775 [details]
Patch

Fujii suspects that on the bots core.autocrlf is `true` but it should be `false`. I’ll confirm this tomorrow.

He also verified that the patch compiled on wincairo so I’m going to cq+ this to not block anyone.

[WebKit Commit Bot]

Comment on attachment 328775 [details]
Patch

Clearing flags on attachment: 328775

Committed r225736: <https://trac.webkit.org/changeset/225736>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35959544>

[Frédéric Wang (:fredw)]

(In reply to Don Olmstead from comment #72)
> Comment on attachment 328775 [details]
> Patch
> 
> Fujii suspects that on the bots core.autocrlf is `true` but it should be
> `false`. I’ll confirm this tomorrow.
> 
> He also verified that the patch compiled on wincairo so I’m going to cq+
> this to not block anyone.

Thanks for checking! Note that I set cq- because there were some remaining review feedback to fix before landing it.

@Minsheng: Can you please open a follow-up bug with a patch addressing the concerns raised by Zalan and I?

[Frédéric Wang (:fredw)]

Comment on attachment 328775 [details]
Patch

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

> LayoutTests/ChangeLog:17
> +        * platform/gtk/mathml/opentype/opentype-stretchy-horizontal-expected.txt: Removed.

Oops, this is bad! The GTK/windows tests should either have been updated or untouched so that someone can rebase them after the patch lands. You should not just remove them.

[Frédéric Wang (:fredw)]

https://trac.webkit.org/changeset/226119/

[Minsheng Liu]

(In reply to Frédéric Wang (:fredw) from comment #78)
> https://trac.webkit.org/changeset/226119/

Thanks!

I have a question: you mentioned before that test outputs can be fetched from build.webkit.org, but I cannot find any relevant test outputs on that site. Where could I find them?

[Frédéric Wang (:fredw)]

(In reply to Minsheng Liu from comment #79)
> (In reply to Frédéric Wang (:fredw) from comment #78)
> > https://trac.webkit.org/changeset/226119/
> 
> Thanks!
> 
> I have a question: you mentioned before that test outputs can be fetched
> from build.webkit.org, but I cannot find any relevant test outputs on that
> site. Where could I find them?

You'll find them in https://build.webkit.org/results ; for example https://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/r226096%20(3524)/results.html for Apple Windows port.