Percent width/height SVG not always scaled on window resize

• Component: SVG
• Status: RESOLVED FIXED

[Florin Malita]

Created attachment 128737 [details]
SVG not scaling on window resize

http://code.google.com/p/chromium/issues/detail?id=114026

In the attached sample the SVG element is 75%x75% of its container (a 100% absolute div). The initial layout/paint is correct, but resizing the browser window (say half the original width) clips the SVG root element instead of scaling it to fit its container.

[Florin Malita]

I tracked this down to a couple of problems:

* After the SVG/CSS width/height property separation, RenderReplaced::computePreferredLogicalWidths() no longer catches SVG percent widths; m_minPreferredLogicalWidth is set on the first layout and never reset again (even if the window is shrunk).

* For vertical-only resize, no layout is being triggered on the SVG root element as RenderBlock short-circuits the process. There is some infrastructure for making RenderBlock aware of percent-height descendants - I think we'll have to make use of it and register percent-height SVG elements as such.

I have a patch addressing these issues coming shortly.

[Florin Malita]

Created attachment 128762 [details]
Patch

First pass.

[Nikolas Zimmermann]

Comment on attachment 128762 [details]
Patch

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

Nice patch & testcases - here are some suggestions/questions:

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1453
> +    layoutPercentHeightDescendants();

Doesn't this cause changes for inline children of RenderBlocks in general? Are we doing unnecessary work for HTML? Is HTML maybe affected as well by this bug (inline percent height elements not reacting to size changes).

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:195
> +            containingBlock()->addPercentHeightDescendant(const_cast<RenderSVGRoot*>(this));

Excellent catch.

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:288
> +        RenderBlock::removePercentHeightDescendant(const_cast<RenderSVGRoot*>(this));

Hrm, I'm slightly worried about this approach. What happens if the <svg> height attribute gets dynamically changed from a percentage to a non-perc value?
Grepping through rendering/, shows another callee of removePercentHeightDescendant: RenderBox::styleDidChange. It always deregisteres itself as percent-height descendant, based on the oldStyle information. The direct calls to oldStyle->logicalHeight().isPercent() have to be avoided. See below for an idea how to fix it:

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:419
> +

Hm, this feels hackish. How about changing RenderReplaced::computePreferredLogicalWidths() to avoid asking the style() whether its dimensions are relative. Changing
if (style()->width().isPercent() || style()->height().isPercent() || ....
    m_minPrefLogWidth = 0;
..

to

if (hasRelativeLogicalWidth(style()) || hasRelativeLogicalHeight(style())))
    m_minPrefLogWidth = 0;

and move style()->logicalWidth().isPercent() etc. checks into RenderBox::hasRelativeLogicalWidth/Height(RenderStyle*), mark it virtual and override it in RenderSVGRoot, to respect SVG width/height properties. It needs to be in RenderBox, so styleDidChange can use it as well.

if (oldStyle && (oldStyle->logicalHeight().isPercent() ..
-> if (oldStyle && hasRelativeLogicalWidth(oldStyle)...

This should fix all issues I'm having with it right now :-)

(If this shows hot in profiles, we can always refactor it to use non-virtual calls to a fastHasRelativeLogicalWidth/Height inline implementation right in RenderReplaced for non-SVG RenderReplaced objects to avoid both function call overhead & virtual function call overhead)

> LayoutTests/svg/custom/svg-percent-scale-expected.html:16
> +    window.resizeTo(400, 300);

Oh my! I wasn't aware that this exists! In the past I always simulated that by enclosing the <svg> in a <div> and changing that <div>'s width/height CSS property, to simulate a window resize -- hence I never ran into this bug!

[Florin Malita]

(In reply to comment #3)
> > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1453
> > +    layoutPercentHeightDescendants();
> 
> Doesn't this cause changes for inline children of RenderBlocks in general? Are we doing unnecessary work for HTML? Is HTML maybe affected as well by this bug (inline percent height elements not reacting to size changes).

Good point. Based on my initial tracing, it seemed that this is needed to get percent height/width inline elements working. But thanks to your question I took another dive and realized that HTML inline percent elements work just fine (and without using the global percent-height descendant map) thanks to this little nugget I missed in RenderBlock::layoutInlineChildren():

  if (relayoutChildren || o->style()->width().isPercent() || o->style()->height().isPercent())
    o->setChildNeedsLayout(true, false);

We can refactor this to take advantage of your RenderBox::hasRelativeLogical{Height,Width} suggestion and then the whole percentHeightDescendantMap dance is no longer needed. It changes the HTML semantics slightly - testing max{Height,Width} & min{Height,Width} for percent units also - but I think that should be the correct behavior.


> Hm, this feels hackish. How about changing RenderReplaced::computePreferredLogicalWidths() to avoid asking the style() whether its dimensions are relative. Changing
> if (style()->width().isPercent() || style()->height().isPercent() || ....
>     m_minPrefLogWidth = 0;
> ..
> 
> to
> 
> if (hasRelativeLogicalWidth(style()) || hasRelativeLogicalHeight(style())))
>     m_minPrefLogWidth = 0;
> 
> and move style()->logicalWidth().isPercent() etc. checks into RenderBox::hasRelativeLogicalWidth/Height(RenderStyle*), mark it virtual and override it in RenderSVGRoot, to respect SVG width/height properties. It needs to be in RenderBox, so styleDidChange can use it as well.

Excellent suggestion. Now that I figured how to avoid messing with the percent height descendant map, this actually simplifies the patch quite a bit - thanks!


> > LayoutTests/svg/custom/svg-percent-scale-expected.html:16
> > +    window.resizeTo(400, 300);
> 
> Oh my! I wasn't aware that this exists! In the past I always simulated that by enclosing the <svg> in a <div> and changing that <div>'s width/height CSS property, to simulate a window resize -- hence I never ran into this bug!

It's a bit confusing because current browsers don't observe it, but thankfully it works with NRWT :)

[Florin Malita]

Created attachment 129066 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 129066 [details]
Patch

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

Almost there, still some perf issues that should be resolved:

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1489
> +                if (relayoutChildren || box->hasRelativeLogicalWidth() || box->hasRelativeLogicalHeight())

Hm, I think you can now merge these two methods. no place needs either width or height information, all callers need both width and height info.
To avoid doing two virtual calls, just use hasRelativeDimensions() - this way you can also omit the logical everywhere, as it won't matter.

Note: You're also changing HTML behavior here, which is probably a bug fix. maxWidth/maxHeight is now included in this check. Needs a testcase.

> Source/WebCore/rendering/RenderBox.cpp:3967
> +

One newline too much.

[Nikolas Zimmermann]

(In reply to comment #4)
> (In reply to comment #3)
> > > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1453
> > > +    layoutPercentHeightDescendants();
> > 
> > Doesn't this cause changes for inline children of RenderBlocks in general? Are we doing unnecessary work for HTML? Is HTML maybe affected as well by this bug (inline percent height elements not reacting to size changes).
> 
> Good point. Based on my initial tracing, it seemed that this is needed to get percent height/width inline elements working. But thanks to your question I took another dive and realized that HTML inline percent elements work just fine (and without using the global percent-height descendant map) thanks to this little nugget I missed in RenderBlock::layoutInlineChildren():

Are there cases where we still need that map? Can you investigate again to construct a testcase for this? I feel we're still failing in some cases of the percentage calculations. (I'd compare which HTML construct depends on the map, and see how it affects svg)

[Florin Malita]

(In reply to comment #6)
> > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1489
> > +                if (relayoutChildren || box->hasRelativeLogicalWidth() || box->hasRelativeLogicalHeight())
> 
> Hm, I think you can now merge these two methods. no place needs either width or height information, all callers need both width and height info.
> To avoid doing two virtual calls, just use hasRelativeDimensions() - this way you can also omit the logical everywhere, as it won't matter.

Done.


> Note: You're also changing HTML behavior here, which is probably a bug fix. maxWidth/maxHeight is now included in this check. Needs a testcase.

Done.

> > Source/WebCore/rendering/RenderBox.cpp:3967
> > +
> 
> One newline too much.

Done.

> Are there cases where we still need that map? Can you investigate again to construct a testcase for this? I feel we're still failing in some cases of the percentage calculations. (I'd compare which HTML construct depends on the map, and see how it affects svg)

Yup, looks like it breaks when anonymous blocks are inserted. I'm updating the patch to walk the anonymous container block chain similarly to RenderBox::computeReplacedLogicalHeightUsing(), and register the percent height descendant. Also adding a vertical-only SVG resize test to cover this.

The cleanup gets a bit tricky because we're not hitting the usual styleDidChange() on SVG attribute changes. One place to handle it is in RenderSVGRoot::computeReplacedLogicalHeight(), same place where we're adding to the map: if the (new) height is a percent, add ourself to the anonymous parent(s) descendant map, if not remove ourself from said map. I think this should cover the height unit change just fine - the alternative would be to hook somewhere into SVGSVGElement's svgAttributeChange().

Updating patch.

[Florin Malita]

Created attachment 129134 [details]
Patch

[WebKit Review Bot]

Comment on attachment 129134 [details]
Patch

Attachment 129134 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11640363

[Early Warning System Bot]

Comment on attachment 129134 [details]
Patch

Attachment 129134 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11643339

[Florin Malita]

Created attachment 129168 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 129168 [details]
Patch

Wonderful patch, r=me!

[WebKit Review Bot]

The commit-queue encountered the following flaky tests while processing attachment 129168 [details]:

perf/object-keys.html bug 63769 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Review Bot]

Comment on attachment 129168 [details]
Patch

Clearing flags on attachment: 129168

Committed r109104: <http://trac.webkit.org/changeset/109104>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Nikolas Zimmermann]

svg/custom/svg-percent-scale-vonly.html & svg/custom/svg-percent-scale.html	both fail for me locally, when running pixel tests.
Bug 79888, that Ossy filed, also shows that the other fast/repaint test you've added doesn't work as expected. I guess it's window.resizeTo which makes problems :(

Can you investigate?

[Florin Malita]

(In reply to comment #17)
> I guess it's window.resizeTo which makes problems :(

Yup, looks like on some platforms resizeTo has no effect. Which is strange because I picked the idea from other (presumably working) tests...

> Can you investigate?

Sure thing.

[Florin Malita]

Created attachment 129483 [details]
Patch

Updated tests to use <div> resizing instead of window.resize{To,By}.

[Nikolas Zimmermann]

Comment on attachment 129483 [details]
Patch

Looks great, thanks Florin! r=me.

[WebKit Review Bot]

Comment on attachment 129483 [details]
Patch

Rejecting attachment 129483 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/11779361

[Nikolas Zimmermann]

Comment on attachment 129483 [details]
Patch

Landed this patch manually, after verifying this indeed fixes the problems.

[Nikolas Zimmermann]

Landed fix in r109334.