Bug 91790 – SVG Fragment is not rendered if it is the css background image of an HTML element

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

[Ken Collins]

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/

[Dirk Schulze]

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.

[Antoine Quint]

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…

[Dirk Schulze]

(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.

[Antoine Quint]

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>.

[Antoine Quint]

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.

[Antoine Quint]

My proposed fix is to pass a new URL parameter when creating an SVGImage in CachedImage::createImage(). Patch coming up.

[Antoine Quint]

Created attachment 225157 [details]
Patch

[Antoine Quint]

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.

[Antoine Quint]

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;
}

[Antoine Quint]

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.

[Antoine Quint]

I'll start with https://bugs.webkit.org/show_bug.cgi?id=129387 to make fragment identifiers work with SVG images via <img>.

[Radar WebKit Bug Importer]

<rdar://problem/19853296>

[Said Abou-Hallawa]

Created attachment 254578 [details]
Patch

[Darin Adler]

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.

[Said Abou-Hallawa]

Created attachment 254611 [details]
Patch

[Said Abou-Hallawa]

(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.

[WebKit Commit Bot]

Comment on attachment 254611 [details]
Patch

Clearing flags on attachment: 254611

Committed r185395: <http://trac.webkit.org/changeset/185395>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Steven Vachon]

How is this fixed? The fragments are ignored and all are rendered: http://codepen.io/chriscoyier/pen/GndhE

[Said Abou-Hallawa]

(In reply to comment #19)
> How is this fixed? The fragments are ignored and all are rendered:
> http://codepen.io/chriscoyier/pen/GndhE

Thanks for reporting this problem and providing a test case.

There are two issues here:

1. The SVG fragment is not respected in the <img> element if the source is an HTTP URL (i.e. <img src="some_http_url#fragment_id">). It happens because the SVGImage gets only the image URL without the fragment in the case of HTTP URL. See MemoryCache::shouldRemoveFragmentIdentifier(). The reason for removing the fragment is the CachedImage in this case is loaded from the network only once. The CachedImage and hence the SVGImage are shared among multiple RenderImages. Because the SVGImage gets the URL without the fragment, it does not call FrameView::scrollToFragment() when drawing. I filed https://bugs.webkit.org/show_bug.cgi?id=163811 to track this issue.

2. The SVG fragment is not respected if the SVG image is drawn as the background of an element and the fragment is the id of a <view> element (i.e. <div style= "background: url(some_url#some_view_element_id);"></div>). The reason is the following. We call SVGSVGElement::scrollToAnchor() to set the viewBox of the root <svg> to the viewBox of the <view> element and we call RenderSVGResource::markForLayoutAndParentResourceInvalidation() for the root renderer but we never run the layout for the SVGImage before drawing it. I filed https://bugs.webkit.org/show_bug.cgi?id=163812 to track this issue.

[Said Abou-Hallawa]

I also filed https://bugs.webkit.org/show_bug.cgi?id=163822.

[Steven Vachon]

Thanks!!

[Ian Yang]

I'm confused. This bug still hasn't been fixed (http://caniuse.com/#feat=svg-fragment), but the Status has been changed to RESOLVED FIXED?

[Said Abou-Hallawa]

(In reply to Ian Yang from comment #23)
> I'm confused. This bug still hasn't been fixed
> (http://caniuse.com/#feat=svg-fragment), but the Status has been changed to
> RESOLVED FIXED?

I am not sure if I understand your question. Do you have a test case which does not work after the resolution of the bug? If this is the case, please attach it to this bug or open a new bug and describe what you expect and what WebKit currently renders.

[Pavel]

(In reply to Said Abou-Hallawa from comment #24)
> (In reply to Ian Yang from comment #23)
> > I'm confused. This bug still hasn't been fixed
> > (http://caniuse.com/#feat=svg-fragment), but the Status has been changed to
> > RESOLVED FIXED?
> 
> I am not sure if I understand your question. Do you have a test case which
> does not work after the resolution of the bug? If this is the case, please
> attach it to this bug or open a new bug and describe what you expect and
> what WebKit currently renders.

Testcase: http://plnkr.co/edit/oaifzmh2g8pMkPtisyFP?p=preview

Not working on Safari with 603. Nothing being rendered at all.

[Silac]

11.0.3, original test case is still relevant

http://jsfiddle.net/simurai/7GCGr/