Bug 81513 - Relative-height block SVG root not layed out on container height change
Summary: Relative-height block SVG root not layed out on container height change
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Florin Malita
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-19 08:51 PDT by Florin Malita
Modified: 2012-03-19 17:37 PDT (History)
2 users (show)

See Also:


Attachments
The SVG element doesn't get adjusted on container height change. (608 bytes, text/html)
2012-03-19 08:52 PDT, Florin Malita
no flags Details
Patch (10.01 KB, patch)
2012-03-19 09:12 PDT, Florin Malita
no flags Details | Formatted Diff | Diff
Patch (9.19 KB, patch)
2012-03-19 13:44 PDT, Florin Malita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Florin Malita 2012-03-19 08:51:00 PDT
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.
Comment 1 Florin Malita 2012-03-19 08:52:52 PDT
Created attachment 132589 [details]
The SVG element doesn't get adjusted on container height change.
Comment 2 Florin Malita 2012-03-19 09:12:23 PDT
Created attachment 132593 [details]
Patch
Comment 3 Nikolas Zimmermann 2012-03-19 09:31:25 PDT
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.
Comment 4 Florin Malita 2012-03-19 13:44:14 PDT
Created attachment 132655 [details]
Patch

updated patch
Comment 5 Florin Malita 2012-03-19 14:02:26 PDT
(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.
Comment 6 Dirk Schulze 2012-03-19 15:33:06 PDT
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.
Comment 7 WebKit Review Bot 2012-03-19 17:37:47 PDT
Comment on attachment 132655 [details]
Patch

Clearing flags on attachment: 132655

Committed r111279: <http://trac.webkit.org/changeset/111279>
Comment 8 WebKit Review Bot 2012-03-19 17:37:51 PDT
All reviewed patches have been landed.  Closing bug.