[Webkit-unassigned] [Bug 79274] Fix <msubsup> formatting

[bugzilla-daemon at webkit.org]

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


Julien Chaffraix <jchaffraix at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #128627|review?, commit-queue?      |
               Flag|                            |




--- Comment #20 from Julien Chaffraix <jchaffraix at webkit.org>  2012-02-25 12:41:02 PST ---
(From update of attachment 128627)
View in context: https://bugs.webkit.org/attachment.cgi?id=128627&action=review

There are several logical issues with the underlying code. It looks like MathML is abusing the CSS logic pretty badly.

Now the patch is making the situation slightly worse by re-creating 2 RenderStyle every time we layout (:-(). I would love to understand why.

As Eric mentioned, the padding computation should be moved to a new function for clarity. Thanks for reviving this code.

> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:52
>  PassRefPtr<RenderStyle> RenderMathMLBlock::createBlockStyle()

The naming is bad. If you are returning an anonymous style, please say so. Looking at the call site, you don't create anonymous renderer but they will get anonymous style which is wrong (sic!). I can't think of any nasty consequences but you are definitely abusing the rendering logic.

> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:66
> +    RefPtr<RenderStyle> newStyle = RenderStyle::createAnonymousStyle(child->style());
> +    newStyle->setDisplay(BLOCK);
> +    wrapper->setStyle(newStyle.release());

This should use the function above.

> Source/WebCore/rendering/mathml/RenderMathMLBlock.h:84
> +    static void checkBlockWrapper(RenderObject* wrapper);

The naming is bad. It doesn't really "check" the block.

I have no idea why you would need to recreate your style *every time* you do a layout. This is likely a hidden bug in your implementation as you really shouldn't need to.

> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:139
> +    RenderObject* superscriptWrapper = m_scripts->firstChild();
> +    RenderObject* subscriptWrapper = m_scripts->lastChild();

Could we get some ASSERT here that you are actually getting the right objects? I fear this could break very badly if you allow CSS generated content (:before / :after) on MathML elements...

> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:143
> +    RenderObject* superscript = superscriptWrapper->firstChild();
> +    RenderObject* subscript = subscriptWrapper->firstChild();

Ditto.

> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:161
> +        superPaddingBottom += - basePaddingTop;

s/+= -/-=/

> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:167
> +    baseWrapper->setNeedsLayout(true);

This is super dangerous (I see it was already present not strictly your fault). Here you will end up trying to mark your parents for layout and because you are in the middle of your layout that could confuse the delayed-layout code.

The good pattern is to mark *only* your children if you need to (see RenderTable::layout() for example which marks only its <caption> children if our logical width changes) and *before* actually laying them.

> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:171
> +    superscriptWrapper->setNeedsLayout(true);

Ditto.

> Source/WebCore/rendering/mathml/RenderMathMLSubSup.h:-42
> -    virtual LayoutUnit baselinePosition(FontBaseline, bool firstLine, LineDirectionMode, LinePositionMode = PositionOnContainingLine) const;

IMHO this is a good change as our baseline is not impacted by subscript / superscript.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.