1398806 - Wrong percent stroke-width for <use> content

Status: RESOLVED FIXED

(Core :: SVG, defect, P2)

Description

[Florens Verschelde :fvsch]

Attachment: stroke-width-percent.html

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.

Comment 1

[Florens Verschelde :fvsch]

Attachment: Screenshot of test case in Nightly and Chrome

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (using percent "width" attribute instead)

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

Comment 3

[Daniel Holbert [:dholbert]]

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

Comment 4

[u459114]

I will look into it

Comment 5

[Gerry Chang [:gchang]]

We are about to build 56 RC and it's too late for 56, mark 56 as won't fix.

Comment 6

[Amelia Bellamy-Royds]

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!

Comment 7

[Amelia Bellamy-Royds]

(Sigh, just downloaded stable to check & the bug is already there. This is going to break a lot of my examples/demos.)

Comment 8

[Robert Longson [:longsonr]]

Attachment: viewport.txt

Comment 9

[Robert Longson [:longsonr]]

Attachment: reftest

Comment 10

[Daniel Holbert [: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).

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 8916245 [details] [diff] [review]
reftest

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

r=me

Comment 12

[Robert Longson [:longsonr]]

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
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

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/813d4e250712

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 18

[Robert Longson [:longsonr]]

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

Comment 19

[Robert Longson [:longsonr]]

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

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8916231 [details] [diff] [review]
viewport.txt

low risk, recent regression, Beta57+

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6f56256c3f17

Comment 22

[Andrei Vaida [:avaida]]

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