Bug 79490

Summary: Percent width/height SVG not always scaled on window resize
Product: WebKit Reporter: Florin Malita <fmalita>
Component: SVGAssignee: Florin Malita <fmalita>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, fmalita, schenney, webkit.review.bot, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 79888    
Bug Blocks:    
Attachments:
Description Flags
SVG not scaling on window resize
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch zimmermann: review+, webkit.review.bot: commit-queue-

Description Florin Malita 2012-02-24 08:04:14 PST
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.
Comment 1 Florin Malita 2012-02-24 08:23:35 PST
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.
Comment 2 Florin Malita 2012-02-24 10:59:26 PST
Created attachment 128762 [details]
Patch

First pass.
Comment 3 Nikolas Zimmermann 2012-02-25 00:31:05 PST
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!
Comment 4 Florin Malita 2012-02-27 09:40:01 PST
(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 :)
Comment 5 Florin Malita 2012-02-27 10:35:14 PST
Created attachment 129066 [details]
Patch
Comment 6 Nikolas Zimmermann 2012-02-27 11:51:18 PST
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.
Comment 7 Nikolas Zimmermann 2012-02-27 11:53:29 PST
(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)
Comment 8 Florin Malita 2012-02-27 16:41:32 PST
(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.
Comment 9 Florin Malita 2012-02-27 16:57:12 PST
Created attachment 129134 [details]
Patch
Comment 10 WebKit Review Bot 2012-02-27 18:45:35 PST
Comment on attachment 129134 [details]
Patch

Attachment 129134 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11640363
Comment 11 Early Warning System Bot 2012-02-27 18:52:33 PST
Comment on attachment 129134 [details]
Patch

Attachment 129134 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11643339
Comment 12 Florin Malita 2012-02-27 19:25:30 PST
Created attachment 129168 [details]
Patch
Comment 13 Nikolas Zimmermann 2012-02-28 06:46:03 PST
Comment on attachment 129168 [details]
Patch

Wonderful patch, r=me!
Comment 14 WebKit Review Bot 2012-02-28 07:59:17 PST
The commit-queue encountered the following flaky tests while processing attachment 129168 [details]:

perf/object-keys.html bug 63769 (author: ojan@chromium.org)
The commit-queue is continuing to process your patch.
Comment 15 WebKit Review Bot 2012-02-28 08:01:35 PST
Comment on attachment 129168 [details]
Patch

Clearing flags on attachment: 129168

Committed r109104: <http://trac.webkit.org/changeset/109104>
Comment 16 WebKit Review Bot 2012-02-28 08:01:40 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 Nikolas Zimmermann 2012-02-29 01:33:31 PST
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?
Comment 18 Florin Malita 2012-02-29 07:03:32 PST
(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.
Comment 19 Florin Malita 2012-02-29 11:41:15 PST
Created attachment 129483 [details]
Patch

Updated tests to use <div> resizing instead of window.resize{To,By}.
Comment 20 Nikolas Zimmermann 2012-03-01 00:38:41 PST
Comment on attachment 129483 [details]
Patch

Looks great, thanks Florin! r=me.
Comment 21 WebKit Review Bot 2012-03-01 02:24:18 PST
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
Comment 22 Nikolas Zimmermann 2012-03-01 04:30:41 PST
Comment on attachment 129483 [details]
Patch

Landed this patch manually, after verifying this indeed fixes the problems.
Comment 23 Nikolas Zimmermann 2012-03-01 04:31:33 PST
Landed fix in r109334.