[Webkit-unassigned] [Bug 75091] [SVG] Text does not show after scripted scaling.

[bugzilla-daemon at webkit.org]

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





--- Comment #10 from Philip Rogers <pdr at google.com>  2012-02-16 07:12:24 PST ---
(In reply to comment #8)
> (From update of attachment 127235 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127235&action=review
> 
> Pre-review: Very interesting patch, looking forward to the next version :-)
> 
> > Source/WebCore/css/CSSStyleSelector.h:-38
> > -enum ESmartMinimumForFontSize { DoNotUseSmartMinimumForFontSize, UseSmartMinimumForFontFize };
> 
> You could land this separated, but it's not a must :-)

Removed :)

> > Source/WebCore/rendering/svg/RenderSVGTransformableContainer.cpp:42
> >  
> > +bool RenderSVGTransformableContainer::transformToRootChanged()
> > +{
> > +    RenderObject* ancestor = parent();
> 
> Interessting approach, though very differently from what we do for eg. setNeedsBoundariesUpdate.
> 
> void RenderObject::setNeedsBoundariesUpdate()
> {
>     if (RenderObject* renderer = parent())
>         renderer->setNeedsBoundariesUpdate();
> }
> 
> We could implement setTransformToRootChanged the same way. Default impl: pass to parent, and in RenderSVGTransformableContainer::setTransformToRootChanged, you can set it to true. Then just look at each place where we call setNeedsTransformUpdate, and also call setTransformToRootChanged, to make sure each ancestor is notified about the change. That would avoid the need to crawl the tree again when doing layout(), as it's already done eg. from SVGStyedTransformableElement::svgAttributeChanged, if 'transform' changed. (Of course there are more places, just an example).
> 
> What do you think?

I think the transform case is sufficiently different because it is the case where the child nodes need to know information from their ancestors, whereas setBoundariesUpdate() is for notifying ancestors of changes in the children.

The core issue is that global transform changes do not currently propagate down to children, because for most children this does not matter. For text metrics though, it does. There is one other class of renderers that relies on calculateTransformationToOutermostSVGCoordinateSystem: filters/etc, and they handle this by always updating on layout via SVGResourcesCache::clientLayoutChanged().

I talked with schenney at some length about the approach I took here and we boiled the options down to:
1) When the outer transform changes, all children can be notified by adding a boolean to every container.
2) When the outer transform changes, we can store that in the transform container and children can query it by walking back up the tree.

I think it's somewhat of a toss-up in terms of performance between needlessly propagating this transform change notification down the tree versus having to crawl back up in some cases. In this patch I took approach #2 because I think it's likely that the unnecessary work in #1 is high, and I implemented a caching behavior at each transform container so that we only need to look at the text's parent in most cases.

What do you think?

> > Source/WebCore/rendering/svg/SVGRenderSupport.cpp:249
> > +static inline bool transformToRootChanged(RenderObject* start)
> 
> You probably want to share this in eg. SVGRenderSupport, or a more specific class.
D'Oh! Done :)

> > LayoutTests/svg/text/text-rescale.html:20
> > +    function runRepaintTest() {
> 
> This function needs to be named repaintTest(), otherwise you're overriding the function from repaint.js.
> You're currently not using layoutTestController.display() at all - you're not tracking repainting, thus no grey rect is painted over the view after the initial painting.
> Using reftests together with repaint tests, thus doensn't make much sense, otherwise you'd need to paint the gray overlay rects and white highlight rects yourself in the expected.html file!
> 
> I'd advice to make this a pixel test using repaint.js only, and not make it a reftest. Whenever we want to test repainting reliable, this is the way to go.
> For any static testcase reftests are the way to go in the future.
> 
> > LayoutTests/svg/text/text-rescale.html:22
> > +        if (window.layoutTestController)
> > +            layoutTestController.waitUntilDone();
> 
> This is not needed anymore, once you fix the repaintTest() name clash.
> 
> > LayoutTests/svg/text/text-rescale.html:30
> > +            if (window.layoutTestController)
> > +                layoutTestController.notifyDone();
> > +        }, 1);
> 
> Ditto.

I think I finally got this right, but can you check one more time? I also updated the test to test for the following:
1) Text in a transformed container
2) Tspan in text in a transformed container (as pointed out by Krit)
3) Text in a foreignobject
4) Text in a nested svg element

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