Closed Bug 1357432 Opened 5 years ago Closed 5 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)

(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.