Relative-height block SVG root not layed out on container height change

• Component: SVG
• Status: RESOLVED FIXED

[Florin Malita]

https://bugs.webkit.org/show_bug.cgi?id=79490 missed a couple of explicit style()->logical{Min,Max,}Height tests in RenderView::layout() & RenderBlock::layoutBlockChildren(). These prevent relative-height block SVG elements from being resized on container height changes.

Clicking the green rect in the attached test resizes the containing <div> vertically, but the SVG root element is not re-adjusted.

[Florin Malita]

Created attachment 132589 [details]
The SVG element doesn't get adjusted on container height change.

[Florin Malita]

Created attachment 132593 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 132593 [details]
Patch

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

r=me, with some small nitpicks:

> Source/WebCore/rendering/RenderBox.cpp:4032
> +    RenderStyle* s = style();

You don't need to introduce style() local vars, its inlined.

> Source/WebCore/rendering/RenderBox.cpp:4040
> +    RenderStyle* s = style();

Ditto.

> Source/WebCore/rendering/RenderView.cpp:120
> +            // FIXME: can RenderView have non-RenderBox children to justify keeping the explicit style checks below?

Can you try this, running regression tests with this fixed, and see if something changes? Ideally before you land this, even if I set r+ :-)

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:428
> +    return svg->intrinsicWidth(SVGSVGElement::IgnoreCSSProperties).isPercent() || RenderSVGRoot::hasRelativeLogicalHeight();

I'd leave this as-is to save a virtual function call.

[Florin Malita]

Created attachment 132655 [details]
Patch

updated patch

[Florin Malita]

(In reply to comment #3) 
> > Source/WebCore/rendering/RenderBox.cpp:4032
> > +    RenderStyle* s = style();
> 
> You don't need to introduce style() local vars, its inlined.

Done.

> > Source/WebCore/rendering/RenderBox.cpp:4040
> > +    RenderStyle* s = style();
> 
> Ditto.

Done.

> > Source/WebCore/rendering/RenderView.cpp:120
> > +            // FIXME: can RenderView have non-RenderBox children to justify keeping the explicit style checks below?
> 
> Can you try this, running regression tests with this fixed, and see if something changes? Ideally before you land this, even if I set r+ :-)

Tried it (assuming child->isBox() == true), and it doesn't seem to introduce any regressions.

But then I found this snippet in FrameView::embeddedContentBox() which seems to indicate that it's still possible for RenderView to have non-RenderBox children:

    RenderView* root = rootRenderer(this);
    if (!root)
        return 0;

    RenderObject* rootChild = root->firstChild();
    if (!rootChild || !rootChild->isBox())
        return 0;

So the isBox() check is probably warranted and I will leave it in unless you think otherwise.

> > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:428
> > +    return svg->intrinsicWidth(SVGSVGElement::IgnoreCSSProperties).isPercent() || RenderSVGRoot::hasRelativeLogicalHeight();
> 
> I'd leave this as-is to save a virtual function call.

Hmm, I don't think that it would trigger a virtual dispatch because it's class-qualified. Nevertheless, it's probably not be worth the extra call so I've reverted it.

[Dirk Schulze]

Comment on attachment 132655 [details]
Patch

Looks good for me. Since Niko already gave r+ and the concerns seems to be addressed (correct me if I am wrong), r=me from me as well.

[WebKit Review Bot]

Comment on attachment 132655 [details]
Patch

Clearing flags on attachment: 132655

Committed r111279: <http://trac.webkit.org/changeset/111279>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.