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

[bugzilla-daemon at webkit.org]

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

--- Comment #19 from Frédéric Wang (:fredw) <fred.wang at free.fr> ---
Comment on attachment 327719
  --> https://bugs.webkit.org/attachment.cgi?id=327719
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.

-- 
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/20171128/c40a9cf6/attachment.html>