Bug 129387 - Respect SVG fragment identifiers in <img> src attribute
Summary: Respect SVG fragment identifiers in <img> src attribute
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-02-26 13:54 PST by Antoine Quint
Modified: 2014-03-03 03:51 PST (History)
13 users (show)

See Also:


Attachments
Patch (7.54 KB, patch)
2014-02-26 14:03 PST, Antoine Quint
koivisto: review+
Details | Formatted Diff | Diff
Patch (7.97 KB, patch)
2014-02-27 06:47 PST, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch for landing (7.97 KB, patch)
2014-02-27 06:53 PST, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (3.27 KB, patch)
2014-03-03 02:01 PST, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (4.37 KB, patch)
2014-03-03 02:25 PST, Antoine Quint
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2014-02-26 13:54:30 PST
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.
Comment 1 Antoine Quint 2014-02-26 14:03:07 PST
Created attachment 225294 [details]
Patch
Comment 2 Dirk Schulze 2014-02-27 05:56:42 PST
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?
Comment 3 Antti Koivisto 2014-02-27 06:05:41 PST
Comment on attachment 225294 [details]
Patch

Looks like a good start.
Comment 4 Antoine Quint 2014-02-27 06:08:23 PST
(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.
Comment 5 Antti Koivisto 2014-02-27 06:08:41 PST
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.
Comment 6 Antoine Quint 2014-02-27 06:47:34 PST
Created attachment 225364 [details]
Patch
Comment 7 Antoine Quint 2014-02-27 06:53:52 PST
Created attachment 225365 [details]
Patch for landing
Comment 8 Dirk Schulze 2014-02-27 07:30:25 PST
(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.
Comment 9 Antoine Quint 2014-02-27 07:45:25 PST
(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.
Comment 10 WebKit Commit Bot 2014-02-27 07:47:00 PST
Comment on attachment 225365 [details]
Patch for landing

Clearing flags on attachment: 225365

Committed r164804: <http://trac.webkit.org/changeset/164804>
Comment 11 WebKit Commit Bot 2014-02-27 07:47:03 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Dirk Schulze 2014-02-27 07:51:54 PST
(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.
Comment 13 Antoine Quint 2014-02-27 07:59:22 PST
(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.
Comment 14 Dirk Schulze 2014-02-27 08:11:34 PST
(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?
Comment 15 Antoine Quint 2014-02-27 08:17:56 PST
(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..
Comment 16 Alexey Proskuryakov 2014-02-28 11:11:48 PST
I think that this change caused a crash, see bug 129498.
Comment 17 Antoine Quint 2014-02-28 11:40:42 PST
(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.
Comment 18 Dirk Schulze 2014-02-28 14:06:16 PST
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.
Comment 19 Antoine Quint 2014-03-01 04:59:54 PST
(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()?
Comment 20 Dirk Schulze 2014-03-01 06:16:35 PST
(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.)
Comment 21 Antoine Quint 2014-03-03 02:01:08 PST
Reopening to attach new patch.
Comment 22 Antoine Quint 2014-03-03 02:01:15 PST
Created attachment 225640 [details]
Patch
Comment 23 Dirk Schulze 2014-03-03 02:15:45 PST
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); }
Comment 24 Antoine Quint 2014-03-03 02:25:40 PST
Created attachment 225641 [details]
Patch
Comment 25 Dirk Schulze 2014-03-03 03:20:27 PST
Comment on attachment 225641 [details]
Patch

r=me
Comment 26 WebKit Commit Bot 2014-03-03 03:51:52 PST
Comment on attachment 225641 [details]
Patch

Clearing flags on attachment: 225641

Committed r164983: <http://trac.webkit.org/changeset/164983>
Comment 27 WebKit Commit Bot 2014-03-03 03:51:57 PST
All reviewed patches have been landed.  Closing bug.