[Webkit-unassigned] [Bug 79490] Percent width/height SVG not always scaled on window resize

[bugzilla-daemon at webkit.org]

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


Nikolas Zimmermann <zimmermann at kde.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #128762|review?                     |review-
               Flag|                            |




--- Comment #3 from Nikolas Zimmermann <zimmermann at kde.org>  2012-02-25 00:31:05 PST ---
(From update of attachment 128762)
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!

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