Bug 91790

Summary: SVG Fragment is not rendered if it is the css background image of an HTML element
Product: WebKit Reporter: Ken Collins <ken>
Component: SVGAssignee: Antoine Quint <graouts>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, donggwan.kim, d-r, fmalita, graouts, gyuyoung.kim, japhet, koivisto, krit, mail, pdr, sabouhallawa, schenney, sergio, simulus, webkit-bug-importer, whoward.tke, zimmermann
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: Unspecified   
URL: http://jsfiddle.net/simurai/7GCGr/
Bug Depends on:    
Bug Blocks: 91791    
Attachments:
Description Flags
Version 13.0.1 of Firefox renders the SVG fragment identifier in CSS background correctly.
none
Patch
none
Patch
none
Patch none

Description Ken Collins 2012-07-19 15:02:57 PDT
Created attachment 153357 [details]
Version 13.0.1 of Firefox renders the SVG fragment identifier in CSS background correctly.

A upcoming technique for SVG sprites, dubbed stacks, has been described here. http://simurai.com/post/20251013889/svg-stacks

It relies on using an SVG fragment identifier (target) in the IRI. Tho Webkit supports this in many other places, it does not support the fragment identifier when used in a CSS background. Here is a JSFiddle linked form the above article that demonstrates the issue. http://jsfiddle.net/simurai/7GCGr/
Comment 1 Dirk Schulze 2013-03-04 06:35:19 PST
SVG stacks will not be supported for CSS properties taking CSS Image values. This includes, but is not limited to, background-image, mask-image, border-image.

This is a resolution of the SVG and CSS WG to differ between resources (like SVG gradients, masks, clipPath) and image values during parse time of CSS. This is a security requirement to protect the users privacy and safety.

See following discussions for further information:
http://lists.w3.org/Archives/Public/www-style/2012Oct/0406.html
http://lists.w3.org/Archives/Public/www-style/2012Oct/0765.html



As an alternative, use SVG image sprites. They work exactly like normal image sprites.

For CSS properties:
Short-term: use background-position to shift your sprites
Long-term: the fragment identifier #xywh=0,0,100,100 will allow to get a sprite out of an image.

For image element:
Short-term: use the fragment identifier #viewbox(0,0,100,100)
Long-term: the fragment identifier #xywh=0,0,100,100 will allow to get a sprite out of an image.
Comment 2 Antoine Quint 2014-02-24 07:13:14 PST
Dirk, you say "This is a resolution of the SVG and CSS WG to differ between resources (like SVG gradients, masks, clipPath) and image values during parse time of CSS. This is a security requirement to protect the users privacy and safety."

In the example, the reference is not to a paint server, but to an SVG image which should respect the :target pseudo-class to display only the element referenced by the identifier in the URL. It seems odd to me that opening http://example.com/sprite.svg#foo would render something if opened as a standalone document but wouldn't if referenced as a CSS background-image.

I'll also note that the <img src=""> fails as well. I think both of these should just work…
Comment 3 Dirk Schulze 2014-02-24 08:03:09 PST
(In reply to comment #2)
> Dirk, you say "This is a resolution of the SVG and CSS WG to differ between resources (like SVG gradients, masks, clipPath) and image values during parse time of CSS. This is a security requirement to protect the users privacy and safety."
> 
> In the example, the reference is not to a paint server, but to an SVG image which should respect the :target pseudo-class to display only the element referenced by the identifier in the URL. It seems odd to me that opening http://example.com/sprite.svg#foo would render something if opened as a standalone document but wouldn't if referenced as a CSS background-image.
> 
> I'll also note that the <img src=""> fails as well. I think both of these should just work…

The resolution changed. The UA doesn't deal with it at parse time anymore. (Which makes implementing URL correctly a bit harder in WebKit for CSS though.)

Instead, all resources (image or SVG resources like <linearGradient>, <clipPath> or <mask>) are loaded with the same restrictions. On rendering we shall decide if we interpret the resources as image or SVG resources.

The restrictions say that the resource in question is not allowed to fetch any other resources (CSS, images or scripts) and is not allowed to execute scripts.

This keep the door open for SVG stacks IMO. Even if me comments was correct at the time, it isn't anymore.

Note that the original resolution just affected CSS's url() function. <img src=""> was never affected.

Reopening the bug.
Comment 4 Antoine Quint 2014-02-25 07:02:20 PST
When loading an SVG file standalone, the fragment identifier is parsed and results in a call to Document::setCSSTarget() as a result of shouldPerformFragmentNavigation() returning true in FrameLoader::loadWithDocumentLoader(). This doesn't happen when loading the same URL through an <img>.
Comment 5 Antoine Quint 2014-02-25 07:56:15 PST
I think the problem actually stems from the fact that when we create the SVGDocument for an SVG image loaded by an <img>, SVGImage::dataChanged() is called higher up in the call stack and calls the following line:

        loader.activeDocumentLoader()->writer().begin(URL()); // create the empty document

Note the empty URL, which eventually causes us to bail in FrameView::scrollToFragment() when we notice that we don't have a fragment identifier for the URL nor do we have a pre-set CSS target.

I think we might be able to deal with this in SVGImage::dataChanged() by getting the loaded resource's URL through the image observer's resource request.
Comment 6 Antoine Quint 2014-02-25 09:12:46 PST
My proposed fix is to pass a new URL parameter when creating an SVGImage in CachedImage::createImage(). Patch coming up.
Comment 7 Antoine Quint 2014-02-25 09:37:10 PST
Created attachment 225157 [details]
Patch
Comment 8 Antoine Quint 2014-02-25 10:20:27 PST
Hmm, as it turns out the original test case at http://jsfiddle.net/simurai/7GCGr/ isn't fixed, it seems SVG resources served through the network fail to load completely.
Comment 9 Antoine Quint 2014-02-26 06:18:36 PST
MemoryCache::removeFragmentIdentifierIfNeeded() is stripping the resource identifier when loading an SVG resource for an <img> element via HTTP:

URL MemoryCache::removeFragmentIdentifierIfNeeded(const URL& originalURL)
{
    if (!originalURL.hasFragmentIdentifier())
        return originalURL;
    // Strip away fragment identifier from HTTP URLs.
    // Data URLs must be unmodified. For file and custom URLs clients may expect resources 
    // to be unique even when they differ by the fragment identifier only.
    if (!originalURL.protocolIsInHTTPFamily())
        return originalURL;
    URL url = originalURL;
    url.removeFragmentIdentifier();
    return url;
}
Comment 10 Antoine Quint 2014-02-26 13:30:21 PST
Comment on attachment 225157 [details]
Patch

Marking my original patch as obsolete, it would only work with non-HTTP URLs (file and custom protocols). A complete patch will be more complex, I'll break this into individual tasks for the various paths needed, starting with <img src> support.
Comment 11 Antoine Quint 2014-02-26 13:55:16 PST
I'll start with https://bugs.webkit.org/show_bug.cgi?id=129387 to make fragment identifiers work with SVG images via <img>.
Comment 12 Radar WebKit Bug Importer 2015-02-16 15:42:56 PST
<rdar://problem/19853296>
Comment 13 Said Abou-Hallawa 2015-06-09 10:31:59 PDT
Created attachment 254578 [details]
Patch
Comment 14 Darin Adler 2015-06-09 11:13:51 PDT
Comment on attachment 254578 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        SVG Fragment Identifier With CSS Backgrounds

Not A Good Bug Title; might be worth fixing it to state the problem we are fixing

> Source/WebCore/ChangeLog:23
> +        (WebCore::CachedImage::load):
> +        (WebCore::CachedImage::checkShouldPaintBrokenImage): being consistent.

Sorry, don’t understand this comment and rationale for changing these. Is the url() function exactly the same as resourceRequest().url(), or is there a possible behavior change?

> Source/WebCore/loader/cache/CachedImage.cpp:345
> +        RefPtr<SVGImage> svgImage = SVGImage::create(this, url());

As long as you are changing this, I suggest changing the argument type to ImageObserver& instead of ImageObserver*, since null is not legal.

> Source/WebCore/svg/graphics/SVGImageCache.cpp:68
> +    Image* image = imageForRenderer(renderer);
> +    return image == Image::nullImage() ? m_svgImage->size() : image->size();

Seems messy to map null pointers to nullImage inside imageForRenderer and then have to check explicitly for nullImage. Maybe a helper function named findImageForRenderer or something that returns null, and imageForRenderer can be a wrapper that adds in the nullImage.

> Source/WebCore/svg/graphics/SVGImageCache.cpp:78
> +    ImageForContainerMap::const_iterator it = m_imageForContainerMap.find(renderer);

Better to use auto here than a specific type.

> Source/WebCore/svg/graphics/SVGImageCache.cpp:82
>      RefPtr<SVGImageForContainer> imageForContainer = it->value;

Type here should be auto&; no need to copy the RefPtr out of the map. I’m not even sure we should bother with a local variable for this.

> Source/WebCore/svg/graphics/SVGImageCache.cpp:84
>      return imageForContainer.get();

I think this whole function could be written like this:

    auto* image = renderer ? m_imageForContainerMap.get(renderer) : nullptr;
    return image ? image : Image::nullImage();

I understand that the code wants to assert that the image size is not empty, but I really think the two line version is way easier to read than the version with many lines of code that uses find and iterators. If the assertion is really needed, we could do this five line version:

    auto* image = renderer ? m_imageForContainerMap.get(renderer) : nullptr;
    if (!image)
         image = Image::nullImage();
    ASSERT(!image->size().isEmpty());
    return image;

> Source/WebCore/svg/graphics/SVGImageCache.h:50
> +    Image* imageForRenderer(const RenderObject*) const;

Does this really need to be callable on a null renderer? Might be worth looking at the call sites and figuring out if the type can be perhaps const RenderElement& instead of const RenderObject*. Probably a project for later.
Comment 15 Said Abou-Hallawa 2015-06-09 16:07:44 PDT
Created attachment 254611 [details]
Patch
Comment 16 Said Abou-Hallawa 2015-06-09 16:14:46 PDT
(In reply to comment #14)
> Comment on attachment 254578 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=254578&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        SVG Fragment Identifier With CSS Backgrounds
> 
> Not A Good Bug Title; might be worth fixing it to state the problem we are
> fixing
> 

Done. The bug title was changed.

> > Source/WebCore/ChangeLog:23
> > +        (WebCore::CachedImage::load):
> > +        (WebCore::CachedImage::checkShouldPaintBrokenImage): being consistent.
> 
> Sorry, don’t understand this comment and rationale for changing these. Is
> the url() function exactly the same as resourceRequest().url(), or is there
> a possible behavior change?
> 

Done. The comment was fixed. And yes, url(), resourceRequest().url() and m_resourceRequest.url() are all the same.

> > Source/WebCore/loader/cache/CachedImage.cpp:345
> > +        RefPtr<SVGImage> svgImage = SVGImage::create(this, url());
> 
> As long as you are changing this, I suggest changing the argument type to
> ImageObserver& instead of ImageObserver*, since null is not legal.
> 

Done. Type of the argument was changed to be reference.

> > Source/WebCore/svg/graphics/SVGImageCache.cpp:68
> > +    Image* image = imageForRenderer(renderer);
> > +    return image == Image::nullImage() ? m_svgImage->size() : image->size();
> 
> Seems messy to map null pointers to nullImage inside imageForRenderer and
> then have to check explicitly for nullImage. Maybe a helper function named
> findImageForRenderer or something that returns null, and imageForRenderer
> can be a wrapper that adds in the nullImage.
> 

Done. A new helper function: findImageForRenderer() was added.

> > Source/WebCore/svg/graphics/SVGImageCache.cpp:78
> > +    ImageForContainerMap::const_iterator it = m_imageForContainerMap.find(renderer);
> 
> Better to use auto here than a specific type.
> 
> > Source/WebCore/svg/graphics/SVGImageCache.cpp:82
> >      RefPtr<SVGImageForContainer> imageForContainer = it->value;
> 
> Type here should be auto&; no need to copy the RefPtr out of the map. I’m
> not even sure we should bother with a local variable for this.
> 
> > Source/WebCore/svg/graphics/SVGImageCache.cpp:84
> >      return imageForContainer.get();
> 
> I think this whole function could be written like this:
> 
>     auto* image = renderer ? m_imageForContainerMap.get(renderer) : nullptr;
>     return image ? image : Image::nullImage();
> 
> I understand that the code wants to assert that the image size is not empty,
> but I really think the two line version is way easier to read than the
> version with many lines of code that uses find and iterators. If the
> assertion is really needed, we could do this five line version:
> 
>     auto* image = renderer ? m_imageForContainerMap.get(renderer) : nullptr;
>     if (!image)
>          image = Image::nullImage();
>     ASSERT(!image->size().isEmpty());
>     return image;
> 

Done. I kept the assertion but I think we should return if (!image) immediately since we should not assert in this case.

> > Source/WebCore/svg/graphics/SVGImageCache.h:50
> > +    Image* imageForRenderer(const RenderObject*) const;
> 
> Does this really need to be callable on a null renderer? Might be worth
> looking at the call sites and figuring out if the type can be perhaps const
> RenderElement& instead of const RenderObject*. Probably a project for later.

Yes. I confirmed that this function can be called with a null renderer. So I kept the check for null renderer as is.
Comment 17 WebKit Commit Bot 2015-06-09 17:46:31 PDT
Comment on attachment 254611 [details]
Patch

Clearing flags on attachment: 254611

Committed r185395: <http://trac.webkit.org/changeset/185395>
Comment 18 WebKit Commit Bot 2015-06-09 17:46:36 PDT
All reviewed patches have been landed.  Closing bug.