Bug 79682 – http://shinydemos.com/clock/ doesn't seem to work

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Sam Weinig]

The demo http://shinydemos.com/clock/ doesn't seem to work all too well.  If you make the window really short, you can see some of a clock, but it only repaints on resizing the window.

[Sam Weinig]

Created attachment 129068 [details]
Reduction showing lack of repaint

[Florin Malita]

*** Bug 86197 has been marked as a duplicate of this bug. ***

[Florin Malita]

There are (at least) two issues at play here:

* the mask is not updated when the text changes - I think I narrowed this down to RenderSVGResourceContainer::layout() being too conservative when it comes to invalidation: it only invalidates if (everHadLayout() && selfNeedsLayout()); that should probably be relaxed to (everHadLayout() && needsLayout()).

* positioning is off: y="80%" appears to be relative to the viewport instead of the element that the mask is applied to.

[Florin Malita]

(In reply to comment #3)
> * positioning is off: y="80%" appears to be relative to the viewport instead of the element that the mask is applied to.

Regarding this second issue: after some investigation, this appears to be the correct behavior. The variance  among browsers stems from the way they size the SVG viewport.

According to the spec, in the absence of size-related CSS properties, the outermost SVG element width/height attributes (which default to 100%) should establish the viewport. Hence, the viewport will vary depending on the browser window size, and so will the relatively position text offset.

http://www.w3.org/TR/SVG/coords.html#ViewportSpace
http://www.w3.org/TR/SVG/struct.html#SVGElementHeightAttribute


So this is probably a problem with the sample: it should specify a fixed sizing for the SVG element such the the relatively position text doesn't 'float' depending on window size.

Other browser's behavior:

* IE9 - same as WK
* FF12 - same as WK when SVG width/height are specified to be 100% (this appears to be a bug as 100% should be the default)
* Opera11 - falls back to intrinsic replaced CSS sizing (300px x 150px) - in my reading of the spec, this is incorrect.


I'll get a patch up for the repaint problem.

[Florin Malita]

Created attachment 142791 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 142791 [details]
Patch

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

> Source/WebCore/rendering/svg/RenderSVGResourceContainer.cpp:56
> +    if (everHadLayout() && needsLayout())

It's unclear to me if this is the right fix.  Why is <g> compensating for descendent elements not doing the right thing?

[Nikolas Zimmermann]

Comment on attachment 142791 [details]
Patch

I think Eric is correct - this seems artificial.
Note that the everHadLayout() && selfNeedsLayout() idiom is used in more places than this - if this is fixed here, why not in the other instances of this code?

[Robert Longson]

(In reply to comment #4)
> * FF12 - same as WK when SVG width/height are specified to be 100% (this appears to be a bug as 100% should be the default)

It's not a bug.

See https://bugzilla.mozilla.org/show_bug.cgi?id=668163#c12 https://bugzilla.mozilla.org/show_bug.cgi?id=668163#c26 and https://bugzilla.mozilla.org/show_bug.cgi?id=668163#c27

[Florin Malita]

(In reply to comment #6)
> (From update of attachment 142791 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142791&action=review
> 
> > Source/WebCore/rendering/svg/RenderSVGResourceContainer.cpp:56
> > +    if (everHadLayout() && needsLayout())
> 
> It's unclear to me if this is the right fix.  Why is <g> compensating for descendent elements not doing the right thing?

(plain <g> should not be affected - RenderSVGResourceContainer is the superclass for <mask>, <filter>, <clip>, etc. renderers, right?)

The assumption is that layout changes for elements that are part of a <mask> (for example) should always invalidate cached resources associated with it. This seems to be a simpler condition to track than trying to hook all possible child mutations, but I think you and Niko are implying that it's too generic and could trigger unneeded invalidations - am I reading that right? IOW: are there cases where changes to a <mask>'s children layout should *not* cause a resource cache invalidation for the mask?

The fundamental problem with this demo is that the node being added is not an SVG element (it's a plain text node), and thus it doesn't know about SVG resources, and doesn't handle their invalidation. Currently, the invalidation path (when adding a <rect> child to a <mask> for example) looks like this:

SVGStyledElement::attach()->RenderSVGModelObject::updateFromElement()->SVGResourcesCache::clientUpdatedFromElement()->RenderSVGResource::markForLayoutAndParentResourceInvalidation()->RenderSVGResourceMasker::removeAllClientsFromCache()

This breaks in two ways:

* element removal - SVGStyledElement::detach() is not overridden like attach() is to trigger the invalidation (see the patch test)
* non SVGStyledElement-derived node handling (text node - see the demo)

An alternate approach would be to trigger the resource invalidation from RenderSVGText::{subtreeChildWasAdded,subtreeChildWasRemoved} and plug the SVGStyledElement::detach() hole. But this seems more involved than the simple childNeedsLayout()-based invalidation, so I'm trying to understand why the latter won't fly :)

[Florin Malita]

http://trac.webkit.org/changeset/118608 fixes most of the problems.

One issue is still left though: rendererCanHaveResources() excludes SVGInlineText nodes, and causes an early exit from clientWasAddedToTree/clientWillBeRemovedFromTree without invalidating parent resources.

[Florin Malita]

Created attachment 145100 [details]
Patch

[Nikolas Zimmermann]

(In reply to comment #10)
> http://trac.webkit.org/changeset/118608 fixes most of the problems.
Heh, I didn't expect that!
 
> One issue is still left though: rendererCanHaveResources() excludes SVGInlineText nodes, and causes an early exit from clientWasAddedToTree/clientWillBeRemovedFromTree without invalidating parent resources.
Great, checking your patch.

[Nikolas Zimmermann]

Comment on attachment 145100 [details]
Patch

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

> Source/WebCore/rendering/svg/SVGResourcesCache.cpp:157
> +    // SVGInlineText cannot have resources.

I'd leave the static inline bool renderercanHaveResources, instead of replicating the comment. It should have no run-time overhead and reads better IMHO.

[Florin Malita]

Created attachment 145307 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 145307 [details]
Patch for landing

Clearing flags on attachment: 145307

Committed r119241: <http://trac.webkit.org/changeset/119241>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.