Closed Bug 1357432 Opened 3 years ago Closed 3 years ago

SVGUseElement::LookupHref does not handle local reference well

Categories

(Core :: SVG, defect)

Product:
Core
Component:
SVG
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: u459114, Assigned: u459114)

References

Details

Attachments

(4 files)

Bug 1357432 - Part 3. Resolve local-ref in SVGUseElement::LookupHref by nsSVGEffects::GetBaseURLForLocalRef.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1357432 - Part 1. Move IsLocalRefURL to nsContentUtils to reuse this function.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1357432 - Part 4. Reftest for using local-ref as xlink:href value.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1357432 - Part 2. Implement nsSVGEffects::GetBaseURLForLocalRef to export local-ref-url-resolving logic.
59 bytes, text/x-review-board-request
heycam
: review+
Details 
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().
Component: DOM → SVG
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)
(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)
Duplicate of this bug: 1352979
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 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+
(In reply to Cameron McCormack (:heycam) from comment #12)
> Is this a regression?
No. it's not
Flags: needinfo?(cku)
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)
(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 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 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+
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
See Also: → 1121708
Duplicate of this bug: 1121708
Duplicate of this bug: 693428
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.