Bug 91790

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.

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…

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

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

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.

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

Created attachment 225157 [details] Patch

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.

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

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

<rdar://problem/19853296>

Created attachment 254578 [details] Patch

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.

Created attachment 254611 [details] Patch

(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 on attachment 254611 [details] Patch Clearing flags on attachment: 254611 Committed r185395: <http://trac.webkit.org/changeset/185395>

All reviewed patches have been landed. Closing bug.

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

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

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

Thanks!!

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?

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

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

11.0.3, original test case is still relevant http://jsfiddle.net/simurai/7GCGr/