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.
Created attachment 225294 [details] Patch
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 on attachment 225294 [details] Patch Looks like a good start.
(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 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.
Created attachment 225364 [details] Patch
Created attachment 225365 [details] Patch for landing
(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.
(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 on attachment 225365 [details] Patch for landing Clearing flags on attachment: 225365 Committed r164804: <http://trac.webkit.org/changeset/164804>
All reviewed patches have been landed. Closing bug.
(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.
(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.
(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?
(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..
I think that this change caused a crash, see bug 129498.
(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.
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.
(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()?
(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.)
Reopening to attach new patch.
Created attachment 225640 [details] Patch
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); }
Created attachment 225641 [details] Patch
Comment on attachment 225641 [details] Patch r=me
Comment on attachment 225641 [details] Patch Clearing flags on attachment: 225641 Committed r164983: <http://trac.webkit.org/changeset/164983>