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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Nov 29 19:53:00 PST 2017


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

Minsheng Liu <lambda at liu.ms> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #327649|0                           |1
        is obsolete|                            |

--- Comment #29 from Minsheng Liu <lambda at liu.ms> ---
Created attachment 327947

  --> https://bugs.webkit.org/attachment.cgi?id=327947&action=review

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.

-- 
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/20171130/c39d4bb6/attachment-0001.html>


More information about the webkit-unassigned mailing list