Closed
Bug 1357432
Opened 7 years ago
Closed 7 years ago
SVGUseElement::LookupHref does not handle local reference well
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: u459114, Assigned: u459114)
References
Details
Attachments
(4 files)
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•7 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•7 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•7 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•7 years ago
|
||
(In reply to Cameron McCormack (:heycam) from comment #12) > Is this a regression? No. it's not
Flags: needinfo?(cku)
Comment 15•7 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•7 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•7 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•7 years ago
|
||
Great, thanks for the pointers to that information.
Comment 19•7 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•7 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•7 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•7 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: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 32•6 years 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)
Comment 33•2 years ago
|
||
(In reply to W. v. Kuipers from comment #32)
This seems to be broken again in FF 61 (also in the beta of 62):
I can confirm that your codepens don't match in Nightly 2018-08-31 (the day that this^ comment was posted) -- the letter labels (M
, N
, O
, P
, etc.) are missing in the "broken" one.
But they look the same (letters are present in both) in current Nightly. So this must've been fixed again.
You need to log in
before you can comment on or make changes to this bug.
Description
•