Closed Bug 1398806 Opened 3 years ago Closed 2 years ago

Wrong percent stroke-width for <use> content

Categories

(Core :: SVG, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- wontfix
firefox57 --- fixed
firefox58 --- fixed

People

(Reporter: fvsch, Assigned: longsonr)

References

Details

(Keywords: regression)

Attachments

(5 files)

In a regression from Firefox stable, using `stroke-width: 1%` will result in a bigger value than expected when applied to the content of a <use xlink:href="…"/> element.

I’m attaching a minimal test case, but this can also be seen on e.g. https://fvsch.com/code/svg-icons/how-to/#percent-stroke-width

Haven’t run mozregression, but I wonder if this is related to bug 265894.
Priority: -- → P3
This doesn't just affect stroke-width -- it affects percent "width" attributes on <rect> as well, for example, as shown in this testcase.

(In Firefox Nightly, the lower orange box is much wider than the upper one. In Chrome, both orange boxes are skinny.)
(In reply to Florens Verschelde from comment #0)
> Haven’t run mozregression, but I wonder if this is related to bug 265894.

Good guess! Regression range:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=edf38274225e72c180b87fde8774c4954b3cc595&tochange=bf1c714b2e1394aca3590ac7ad661bdc735bbb35

--> bug 265894

CJ, maybe you can you take a look?  It's probably too late to fix this in 56, so it'll probably ship as a regression in that version.  But we might be able to get a fix in for 57...
Blocks: 265894
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(cku)
Keywords: regression
I will look into it
Assignee: nobody → cku
Flags: needinfo?(cku)
Status: NEW → ASSIGNED
We are about to build 56 RC and it's too late for 56, mark 56 as won't fix.
Priority: P3 → P2
Here's another test case (the red and green circles should exactly coincide in both SVGs): https://codepen.io/AmeliaBR/pen/ee1f3cebbb67c4157d739586197d5743?editors=1100

This isn't about stroke-width. The re-used symbol is not being recognized as an SVG viewport container that defines the meanings of percentage lengths for its child content.  This previously wouldn't have been an issue, since the re-used symbols were implemented as hidden <svg>.

As much as I've been eagerly anticipating the new use/symbol changes, please don't let this bug reach stable!
(Sigh, just downloaded stable to check & the bug is already there. This is going to break a lot of my examples/demos.)
Attached patch viewport.txtSplinter Review
Attachment #8916231 - Flags: review?(dholbert)
Attachment #8916231 - Flags: review?(cku)
Attached patch reftestSplinter Review
Attachment #8916245 - Flags: review?(dholbert)
Comment on attachment 8916231 [details] [diff] [review]
viewport.txt

Review of attachment 8916231 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for fixing this!

This is just missing a commit message to describe the change (which would've been useful for review purposes, too, to contextualize the overall aim of the patch).  After reading through the whole thing, I think it's refactoring nsSVGElement::GetCtx() to return the closest SVGViewportElement (<svg> or <symbol>) ancestor, correct?  That's what it looks like to me, at least.

Anyway, I think this is r=me with a clear explanatory commit message.

::: dom/svg/SVGContentUtils.cpp
@@ +384,4 @@
>        if (element->IsSVGElement(nsGkAtoms::foreignObject)) {
>          return nullptr;
>        }
> +      return static_cast<SVGViewportElement*>(element);

Could you add an assertion about the tag here, before we do the static-cast, to be sure the static_cast is valid?

Something like:
    MOZ_ASSERT(element->IsAnyOfSVGElements(nsGkAtoms::svg,
                                           nsGkAtoms::symbol),
               "upcoming static_cast is only valid for "
               "SVGViewportElement subclasses");


This assertion will be kind of tautilogical based on how EstablishesViewport() is implemented, but it seems important in case we change the EstablishesViewport impl in the future and we forget to update this code accordingly. (e.g. if we added another foreignObject-like tag, which establishes a viewport but is not a SVGViewportElement subclass).
Attachment #8916231 - Flags: review?(dholbert) → review+
Comment on attachment 8916245 [details] [diff] [review]
reftest

Review of attachment 8916245 [details] [diff] [review]:
-----------------------------------------------------------------

r=me
Attachment #8916245 - Flags: review?(dholbert) → review+
Yes, it's basically implementing this requirement correctly: https://www.w3.org/TR/SVG2/coords.html#EstablishingANewSVGViewport

proposed commit message:
GetCtx and GetNearestViewport should return the nearest svg or symbol element which is now an SVGViewportElement rather than only returning the nearest svg element because a symbol establishes a viewport too.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b2051c849f9080880d60e55f0b15a9600e45103
Pushed by longsonr@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/813d4e250712
GetCtx and GetNearestViewport should return the nearest svg or symbol element which is now an SVGViewportElement rather than only returning the nearest svg element because a symbol establishes a viewport too. r=dholbert
Flags: in-testsuite+
Assignee: cku → longsonr
Attachment #8916231 - Flags: review?(cku)
https://hg.mozilla.org/mozilla-central/rev/813d4e250712
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
The threshold for 57 uplifts is basically "would require a dot release if we don't fix it" which I don't think this bug meets. Calling 57 wontfix but feel free to set the status back to affected and nominate it for approval if you feel strongly otherwise.
Duplicate of this bug: 1408735
Duplicate of this bug: 1407482
Comment on attachment 8916231 [details] [diff] [review]
viewport.txt

Approval Request Comment
[Feature/Bug causing the regression]: bug 265894
[User impact if declined]: wrong display of many SVG files that contain <use> elements. We seem to be getting dups of this reasonably frequently.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]:  no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: pretty straightforward change swapping one class for its base class and returning that instead
[String changes made/needed]: none
Attachment #8916231 - Flags: approval-mozilla-beta?
(In reply to Ryan VanderMeulen [:RyanVM] from comment #15)
> The threshold for 57 uplifts is basically "would require a dot release if we
> don't fix it" which I don't think this bug meets. Calling 57 wontfix but
> feel free to set the status back to affected and nominate it for approval if
> you feel strongly otherwise.

Given the dups that we're getting I think we should try to do something here.
Comment on attachment 8916231 [details] [diff] [review]
viewport.txt

low risk, recent regression, Beta57+
Attachment #8916231 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Robert Longson [:longsonr] from comment #18)
> [Is this code covered by automated tests?]: yes
> [Has the fix been verified in Nightly?]: yes
> [Needs manual test from QE? If yes, steps to reproduce]:  no

Setting qe-verify- based on Robert's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
Duplicate of this bug: 1409437
Depends on: 1414291
Depends on: 1415861
You need to log in before you can comment on or make changes to this bug.