1502658 - SVG ignores HREF attribute changes if these changes were made while SVG is hidden

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[CoolCmd]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:63.0) Gecko/20100101 Firefox/63.0

Steps to reproduce:

see test case: https://jsfiddle.net/CoolCmd/wbm624sn/

there is an SVG on the page, initially hidden. JS sets HREF and shows SVG.


Actual results:

page shows empty square



Expected results:

page should show the black circle

Comment 1

[CoolCmd]

this is regression. Firefox 62 does not have this bug.

Comment 2

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=38e729d91ed2873b708bd4800b01b8433ffceabd&tochange=453d0a08f00c125ff6cc3905265e76efe641524e

Regressed by: Bug 1450250

@:emilio
Your bunch of patch seems to cause the regression. Could you please look into this?

Comment 3

[Emilio Cobos Álvarez (:emilio)]

The underlying problem was sorta-pre-existing:

  https://searchfox.org/mozilla-central/rev/a21ac3d8e53cd37b6725f982b185d92fe47d8835/layout/svg/nsSVGUseFrame.cpp#67

Shouldn't take long to fix, sorry for the breakage, and thanks as always Alice for the regression range :)

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Remove SVG use attribute change handling code from nsSVGUseFrame to SVGUseElement.

Most of those shouldn't rely on a frame to run.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

[Tracking Requested - why for this release]: Regression shipped in 63, would be good not to ship it in 64.

Comment 6

[Robert Longson [:longsonr]]

Comment on attachment 9020587 [details]
Remove SVG use attribute change handling code from nsSVGUseFrame to SVGUseElement.

SMIL animation targets frames. https://dxr.mozilla.org/mozilla-central/source/dom/svg/nsSVGElement.cpp#1548 So I assume this patch breaks SMIL animation of use elements.

Write some SMIL animation tests and make sure they still work.

This is how I've worked around the SMIL problem elsewhere FWIW:
https://dxr.mozilla.org/mozilla-central/source/layout/svg/nsSVGImageFrame.cpp#149
https://dxr.mozilla.org/mozilla-central/source/dom/svg/SVGImageElement.cpp#180

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Nice catch!

The nice thing is that we already have a test for this (layout/reftests/svg/smil/anim-use-length-01.svg) which caught it:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=893afee65ba807ab22b023a413db774cb1c5ed69

So I'll fix. SMIL should actually change the attribute value, probably... :(

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/627720590ce2
Remove SVG use attribute change handling code from nsSVGUseFrame to SVGUseElement. r=longsonr

Comment 9

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/13754 for changes under testing/web-platform/tests

Comment 10

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 11

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/627720590ce2

Comment 12

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9020587 [details]
Remove SVG use attribute change handling code from nsSVGUseFrame to SVGUseElement.

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1450250

User impact if declined: See comment 0.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Moves some attribute change handling code around. Not risk-free, but not very risky I'd say.

String changes made/needed: none

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9020587 [details]
Remove SVG use attribute change handling code from nsSVGUseFrame to SVGUseElement.

[Triage Comment]
Fixes an SVG rendering regression. Includes a new automated test. Approved for 64.0b5.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c28a4c172db8

Comment 16

[Emil Ghitta, QA [:emilghitta]]

I have managed to reproduce this issue using Firefox 65.0a1 (BuildId:20181027220121) on Windows 10 64bit.

This issue is verified fixed using Firefox 65.0a1 (BuildId:20181031223503)and Firefox 64.0b5 (BuildId:20181029164536) on Windows 10 64bit, macOS 10.13.6 and Ubuntu 16.04 32bit.