SVGUseElement::LookupHref does not handle local reference well

RESOLVED FIXED in Firefox 55

Status

()

Core
SVG
RESOLVED FIXED
a month ago
a month ago

People

(Reporter: cjku, Assigned: cjku)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Assignee)

Description

a month ago
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)
(Assignee)

Updated

a month ago
Component: DOM → SVG
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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

a month 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)
(Assignee)

Updated

a month ago
Duplicate of this bug: 1352979
(Assignee)

Updated

a month ago
Attachment #8859237 - Flags: review?(cam)
Attachment #8859289 - Flags: review?(cam)
Attachment #8859222 - Flags: review?(cam)
Attachment #8859238 - Flags: review?(cam)
Is this a regression?
Flags: needinfo?(cku)

Comment 13

a month 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

a month ago
(In reply to Cameron McCormack (:heycam) from comment #12)
> Is this a regression?
No. it's not
Flags: needinfo?(cku)

Comment 15

a month 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+
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

a month 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)
Great, thanks for the pointers to that information.

Comment 19

a month 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

a month 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

a month 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
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
Last Resolved: a month ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Updated

a month ago
See Also: → bug 1121708
Duplicate of this bug: 1121708
You need to log in before you can comment on or make changes to this bug.