Bug 129387 – Respect SVG fragment identifiers in <img> src attribute

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

[Antoine Quint]

When using an SVG resource as the source of an <img> element, the URL fragment identifier may have importance since it could change the rendering of the SVG image if the file uses the :target pseudo-class to customise the rendering of the element identified by the fragment identifier. However, we do not respect those identifiers in WebKit.

[Antoine Quint]

Created attachment 225294 [details]
Patch

[Dirk Schulze]

Comment on attachment 225294 [details]
Patch

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

> Source/WebCore/svg/graphics/SVGImageCache.cpp:95
> +    Node* node = renderer->node();
> +    if (node && isHTMLImageElement(node))
> +        imageForContainer->setURL(toHTMLImageElement(node)->src());

Ok, this basically does not allow <image> to do the same thing. Is there anyway to make it more general? Maybe you pass the fragment identifier into SVGImage and handle this there?

[Antti Koivisto]

Comment on attachment 225294 [details]
Patch

Looks like a good start.

[Antoine Quint]

(In reply to comment #2)
> Ok, this basically does not allow <image> to do the same thing. Is there anyway to make it more general? Maybe you pass the fragment identifier into SVGImage and handle this there?

The trick is to read the URL which will need to be done specifically for the various types of elements and CSS properties that support referencing to an SVG file on the network. So, indeed, <svg:image> will require special machinery, and so will a rather long series of CSS properties where we'll need to get at the various StyleImage to read their URLs which we would have recorded upon their creation, likely within CSSImageValue::cachedOrPendingImage().

Antti invited me to start with a fairly focused patch to test the theory that this could be done using an approach where we work out the URL upon retrieving/creating the image from the SVGImageCache. There's more work ahead.

[Antti Koivisto]

Comment on attachment 225294 [details]
Patch

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

>> Source/WebCore/svg/graphics/SVGImageCache.cpp:95
>> +    if (node && isHTMLImageElement(node))
>> +        imageForContainer->setURL(toHTMLImageElement(node)->src());
> 
> Ok, this basically does not allow <image> to do the same thing. Is there anyway to make it more general? Maybe you pass the fragment identifier into SVGImage and handle this there?

This should be HTMLImageElement::imageSourceURL() I think. That covers srcset too.

[Antoine Quint]

Created attachment 225364 [details]
Patch

[Antoine Quint]

Created attachment 225365 [details]
Patch for landing

[Dirk Schulze]

(In reply to comment #3)
> (From update of attachment 225294 [details])
> Looks like a good start.

I would like have seen more investigation how to archive this directly. This just handles one use case. There are many more starting with CSS Images going on to SVG images. I am sad that we didn't discuss this before giving r+. Especially because I commented before.

[Antoine Quint]

(In reply to comment #8)
> (In reply to comment #3)
> > (From update of attachment 225294 [details] [details])
> > Looks like a good start.
> 
> I would like have seen more investigation how to archive this directly. This just handles one use case. There are many more starting with CSS Images going on to SVG images. I am sad that we didn't discuss this before giving r+. Especially because I commented before.

We had some chats on #webkit yesterday where Antti advised to start small. We also discussed how make this work with images referenced to from CSS. Sorry if I didn't try harder to include you in the conversation, I'll make sure to give you a heads-up on future work in this area.

[WebKit Commit Bot]

Comment on attachment 225365 [details]
Patch for landing

Clearing flags on attachment: 225365

Committed r164804: <http://trac.webkit.org/changeset/164804>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Dirk Schulze]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #3)
> > > (From update of attachment 225294 [details] [details] [details])
> > > Looks like a good start.
> > 
> > I would like have seen more investigation how to archive this directly. This just handles one use case. There are many more starting with CSS Images going on to SVG images. I am sad that we didn't discuss this before giving r+. Especially because I commented before.
> 
> We had some chats on #webkit yesterday where Antti advised to start small. We also discussed how make this work with images referenced to from CSS. Sorry if I didn't try harder to include you in the conversation, I'll make sure to give you a heads-up on future work in this area.

It is usually best practice to summarize the discussions on the bug and let others response to this first. Again, since I wrote a comment and didn't have the time to response to your answer, I think that the procedure here was less than optimal.

[Antoine Quint]

(In reply to comment #12)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > (In reply to comment #3)
> > > > (From update of attachment 225294 [details] [details] [details] [details])
> > > > Looks like a good start.
> > > 
> > > I would like have seen more investigation how to archive this directly. This just handles one use case. There are many more starting with CSS Images going on to SVG images. I am sad that we didn't discuss this before giving r+. Especially because I commented before.
> > 
> > We had some chats on #webkit yesterday where Antti advised to start small. We also discussed how make this work with images referenced to from CSS. Sorry if I didn't try harder to include you in the conversation, I'll make sure to give you a heads-up on future work in this area.
> 
> It is usually best practice to summarize the discussions on the bug and let others response to this first. Again, since I wrote a comment and didn't have the time to response to your answer, I think that the procedure here was less than optimal.

I recognize that and will be more forthcoming on future work.

I'll note however that you hadn't participated in the discussion of https://bugs.webkit.org/show_bug.cgi?id=91790 over the last couple of days as I was working on the broader issue before deciding to focus on the <img> case (this bug) first. I had even emailed you directly when the first patch was up for review and the patch was just sitting there, bug comments unanswered.

[Dirk Schulze]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #9)
> > > (In reply to comment #8)
> > > > (In reply to comment #3)
> > > > > (From update of attachment 225294 [details] [details] [details] [details] [details])
> > > > > Looks like a good start.
> > > > 
> > > > I would like have seen more investigation how to archive this directly. This just handles one use case. There are many more starting with CSS Images going on to SVG images. I am sad that we didn't discuss this before giving r+. Especially because I commented before.
> > > 
> > > We had some chats on #webkit yesterday where Antti advised to start small. We also discussed how make this work with images referenced to from CSS. Sorry if I didn't try harder to include you in the conversation, I'll make sure to give you a heads-up on future work in this area.
> > 
> > It is usually best practice to summarize the discussions on the bug and let others response to this first. Again, since I wrote a comment and didn't have the time to response to your answer, I think that the procedure here was less than optimal.
> 
> I recognize that and will be more forthcoming on future work.
> 
> I'll note however that you hadn't participated in the discussion of https://bugs.webkit.org/show_bug.cgi?id=91790 over the last couple of days as I was working on the broader issue before deciding to focus on the <img> case (this bug) first.

If there is no comment, you obviously don't need to wait for it. If there is, like here, then it is different. Wouldn't you agree?

[Antoine Quint]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > (In reply to comment #9)
> > > > (In reply to comment #8)
> > > > > (In reply to comment #3)
> > > > > > (From update of attachment 225294 [details] [details] [details] [details] [details] [details])
> > > > > > Looks like a good start.
> > > > > 
> > > > > I would like have seen more investigation how to archive this directly. This just handles one use case. There are many more starting with CSS Images going on to SVG images. I am sad that we didn't discuss this before giving r+. Especially because I commented before.
> > > > 
> > > > We had some chats on #webkit yesterday where Antti advised to start small. We also discussed how make this work with images referenced to from CSS. Sorry if I didn't try harder to include you in the conversation, I'll make sure to give you a heads-up on future work in this area.
> > > 
> > > It is usually best practice to summarize the discussions on the bug and let others response to this first. Again, since I wrote a comment and didn't have the time to response to your answer, I think that the procedure here was less than optimal.
> > 
> > I recognize that and will be more forthcoming on future work.
> > 
> > I'll note however that you hadn't participated in the discussion of https://bugs.webkit.org/show_bug.cgi?id=91790 over the last couple of days as I was working on the broader issue before deciding to focus on the <img> case (this bug) first.
> 
> If there is no comment, you obviously don't need to wait for it. If there is, like here, then it is different. Wouldn't you agree?

I don't dispute that. I wanted to point out that there was time to participate in the conversation (well more of a monologue) that had started on https://bugs.webkit.org/show_bug.cgi?id=91790 and then on IRC.

Now, in order to have a broad fix that applies to CSS properties as well, suggestions are very welcome. Feel free to comment on https://bugs.webkit.org/show_bug.cgi?id=91790..

[Alexey Proskuryakov]

I think that this change caused a crash, see bug 129498.

[Antoine Quint]

(In reply to comment #16)
> I think that this change caused a crash, see bug 129498.

Thanks for the report Alexey, I'll look into it tomorrow.

[Dirk Schulze]

Now that I had a chance to look into this more closely a couple of questions:

Something that we use everywhere is an SVGImage. We use it for CachedImages in CSS and for <img> as well as <image>.

So why not take the url and use it within SVGImage? Then you can call scrollToFragment() in SVGImage::draw(). At this point you have a guaranteed Frame and the crash should not appear as well.

It is also a way forward when looking at CSS Images later. CSS Images are a bit more trickier. At this point we have a higher priority than SVG stacks for CSS images. I am happy to chat about the problems that I see once you get there.

[Antoine Quint]

(In reply to comment #18)
> Now that I had a chance to look into this more closely a couple of questions:
> 
> Something that we use everywhere is an SVGImage. We use it for CachedImages in CSS and for <img> as well as <image>.
> 
> So why not take the url and use it within SVGImage? Then you can call scrollToFragment() in SVGImage::draw(). At this point you have a guaranteed Frame and the crash should not appear as well.
> 
> It is also a way forward when looking at CSS Images later. CSS Images are a bit more trickier. At this point we have a higher priority than SVG stacks for CSS images. I am happy to chat about the problems that I see once you get there.

Are you suggesting that calling setURL() on SVGImageForContainer should simply forward the call to the `SVGImage* m_image` and then move the call to scrollToFragment() in SVGImage::draw()?

[Dirk Schulze]

(In reply to comment #19)
> (In reply to comment #18)
> > Now that I had a chance to look into this more closely a couple of questions:
> > 
> > Something that we use everywhere is an SVGImage. We use it for CachedImages in CSS and for <img> as well as <image>.
> > 
> > So why not take the url and use it within SVGImage? Then you can call scrollToFragment() in SVGImage::draw(). At this point you have a guaranteed Frame and the crash should not appear as well.
> > 
> > It is also a way forward when looking at CSS Images later. CSS Images are a bit more trickier. At this point we have a higher priority than SVG stacks for CSS images. I am happy to chat about the problems that I see once you get there.
> 
> Are you suggesting that calling setURL() on SVGImageForContainer should simply forward the call to the `SVGImage* m_image` and then move the call to scrollToFragment() in SVGImage::draw()?

Definitely the latter assuming you can not access SVGImage directly. (Which seems to be the case here.)

[Antoine Quint]

Reopening to attach new patch.

[Antoine Quint]

Created attachment 225640 [details]
Patch

[Dirk Schulze]

Comment on attachment 225640 [details]
Patch

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

Small comments but otherwise looks good.

> Source/WebCore/svg/graphics/SVGImage.cpp:255
> +    if (!m_url.isEmpty())

Could you add an ASSERT(view) right behind FrameView* view = frameView(); earlier in this method please?

> Source/WebCore/svg/graphics/SVGImageForContainer.cpp:57
>  void SVGImageForContainer::setURL(const URL& url)

If it is possible to remove #include "SVGImage.h" from SVGImageForContainer.h, then do this in this patch as well please.

If not, remove the header include from the SVGImageForContainer.cpp and call m_image->setURL(url) from the header:

void setURL(const URL& url) { m_image->setURL(url); }

[Antoine Quint]

Created attachment 225641 [details]
Patch

[Dirk Schulze]

Comment on attachment 225641 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 225641 [details]
Patch

Clearing flags on attachment: 225641

Committed r164983: <http://trac.webkit.org/changeset/164983>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.