SVGUseElement::LookupHref does not handle local reference well
RESOLVED
FIXED
in Firefox 55
Status
()
People
(Reporter: u459114, Assigned: u459114)
Tracking
Firefox Tracking Flags
(firefox55 fixed)
Details
Attachments
(4 attachments)
In bug 652991 comment 238, prodigysim reported an issue as to use local reference in use::href: https://bug652991.bmoattachments.org/attachment.cgi?id=8858347 The root cause is becasue we do not specially handle localRef well in inside SVGUseElement::LookupHref().
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
||
Comment 5•2 years ago
|
||
C.J Ku: Robert thinks this patch will fix bug 1352979 and looking at the patch, it might. Could you try out attachment 8853960 [details] so we can dupe it?
Flags: needinfo?(cku)
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
Assignee | |
Comment 10•2 years ago
|
||
(In reply to Anthony Ricaud (:rik) from comment #5) > C.J Ku: Robert thinks this patch will fix bug 1352979 and looking at the > patch, it might. Could you try out attachment 8853960 [details] so we can > dupe it? I tried. Yes, it's a dup.
Flags: needinfo?(cku)
Attachment #8859237 -
Flags: review?(cam)
Attachment #8859289 -
Flags: review?(cam)
Attachment #8859222 -
Flags: review?(cam)
Attachment #8859238 -
Flags: review?(cam)
|
||
Comment 13•2 years ago
|
||
| mozreview-review | ||
Comment on attachment 8859237 [details] Bug 1357432 - Part 1. Move IsLocalRefURL to nsContentUtils to reuse this function. https://reviewboard.mozilla.org/r/131258/#review134132 ::: commit-message-bb38d:3 (Diff revision 2) > +Bug 1357432 - Part 1. Move IsLocalRefURL to nsContentUtils to reuse this function. > + > +IsLocalRerURL is originally designed to be used by URLValue only. Since we need IsLocalRefURL
Attachment #8859237 -
Flags: review?(cam) → review+
|
|
Assignee | |
Comment 14•2 years ago
|
||
(In reply to Cameron McCormack (:heycam) from comment #12) > Is this a regression? No. it's not
Flags: needinfo?(cku)
|
|
||
Comment 15•2 years ago
|
||
| mozreview-review | ||
Comment on attachment 8859289 [details] Bug 1357432 - Part 2. Implement nsSVGEffects::GetBaseURLForLocalRef to export local-ref-url-resolving logic. https://reviewboard.mozilla.org/r/131300/#review134136 ::: commit-message-fa441:5 (Diff revision 1) > +Bug 1357432 - Part 2. Implement nsSVGEffects::GetBaseURLForLocalRef to export local-ref-url-resolving logic. > + > +ResolveURLUsingLocalRef is designed to be internally used by > +nsSVGEffects::Get-{SVGEffect}-URI functions. Since we also need it in > +SVGUseElement::LookupHref, public it from nsSVGEffects. s/public it from/make it public in/ ::: layout/svg/nsSVGEffects.h:675 (Diff revision 1) > + static already_AddRefed<nsIURI> > + GetBaseURLForLocalRef(nsIContent* content, nsIURI* aDocURI); Please add a comment above this explaining what the function does. ::: layout/svg/nsSVGEffects.cpp:914 (Diff revision 1) > InvalidateDirectRenderingObservers(content->AsElement(), aFlags); > } > } > > -static already_AddRefed<nsIURI> > -ResolveURLUsingLocalRef(nsIFrame* aFrame, const css::URLValueData* aURL) > +already_AddRefed<nsIURI> > +nsSVGEffects::GetBaseURLForLocalRef(nsIContent* content, nsIURI* aDocURI) aContent
Attachment #8859289 -
Flags: review?(cam) → review+
|
|
||
Comment 16•2 years ago
|
||
Is resolving <use xlink:href="#local"> like this what we should be doing? The reason we do it for url(#local) values in CSS properties, IIRC, is because it makes it difficult to reference resources in the main document when url(#local) is specified in an external style sheet, which is why the wording in https://drafts.csswg.org/css-values-3/#local-urls was added. As far as I know (though I didn't check) we don't have any spec wording requiring this for <use xlink:href="#local">. Do other browsers do this? If they do we should probably do it and make sure a spec requires it. If they don't, then I'm not sure we should, although I'd be happy to hear arguments we should.
Flags: needinfo?(cku)
|
|
Assignee | |
Comment 17•2 years ago
|
||
(In reply to Cameron McCormack (:heycam) from comment #16) > Is resolving <use xlink:href="#local"> like this what we should be doing? > The reason we do it for url(#local) values in CSS properties, IIRC, is > because it makes it difficult to reference resources in the main document > when url(#local) is specified in an external style sheet, which is why the > wording in https://drafts.csswg.org/css-values-3/#local-urls was added. As > far as I know (though I didn't check) we don't have any spec wording > requiring this for <use xlink:href="#local">. Do other browsers do this? > If they do we should probably do it and make sure a spec requires it. If > they don't, then I'm not sure we should, although I'd be happy to hear > arguments we should. There are some discussion in bug 1352979, in summary: 1. discussion thread: https://github.com/w3c/svgwg/issues/61 Although, there is a contradiction between svg 2 spec and css-value spec currently. (Please refers to the latest comment, written by Robert, in this thread) 2. The reality chrome and safari: had resolve against current doc Edge: will resolve against current doc(https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/11490803/)
Flags: needinfo?(cku)
|
|
||
Comment 18•2 years ago
|
||
Great, thanks for the pointers to that information.
|
|
||
Comment 19•2 years ago
|
||
| mozreview-review | ||
Comment on attachment 8859222 [details] Bug 1357432 - Part 3. Resolve local-ref in SVGUseElement::LookupHref by nsSVGEffects::GetBaseURLForLocalRef. https://reviewboard.mozilla.org/r/131252/#review134690
Attachment #8859222 -
Flags: review?(cam) → review+
|
|
||
Comment 20•2 years ago
|
||
| mozreview-review | ||
Comment on attachment 8859238 [details] Bug 1357432 - Part 4. Reftest for using local-ref as xlink:href value. https://reviewboard.mozilla.org/r/131260/#review134692
Attachment #8859238 -
Flags: review?(cam) → review+
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
||
Comment 28•2 years ago
|
||
Pushed by cku@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2fddfb975738 Part 1. Move IsLocalRefURL to nsContentUtils to reuse this function. r=heycam https://hg.mozilla.org/integration/autoland/rev/932320750dfe Part 2. Implement nsSVGEffects::GetBaseURLForLocalRef to export local-ref-url-resolving logic. r=heycam https://hg.mozilla.org/integration/autoland/rev/92f7611393f6 Part 3. Resolve local-ref in SVGUseElement::LookupHref by nsSVGEffects::GetBaseURLForLocalRef. r=heycam https://hg.mozilla.org/integration/autoland/rev/032d23aaffe3 Part 4. Reftest for using local-ref as xlink:href value. r=heycam
|
||
Comment 29•2 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/2fddfb975738 https://hg.mozilla.org/mozilla-central/rev/932320750dfe https://hg.mozilla.org/mozilla-central/rev/92f7611393f6 https://hg.mozilla.org/mozilla-central/rev/032d23aaffe3
Status: NEW → RESOLVED
Closed: 2 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
|
||
Comment 32•10 months ago
|
||
This seems to be broken again in FF 61 (also in the beta of 62): https://codepen.io/wvankuipers/pen/rZjyGW (broken: with base) https://codepen.io/wvankuipers/pen/eLgRpb (working: without base)
You need to log in
before you can comment on or make changes to this bug.
Description
•