Closed Bug 1502658 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: SVG, defect)

63 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla65
Tracking Status
firefox-esr60 --- unaffected
firefox63 --- wontfix
firefox64 + verified
firefox65 + verified

People

(Reporter: CoolCmd, Assigned: emilio)

References

Details

(Keywords: regression)

Attachments

(1 file)

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
this is regression. Firefox 62 does not have this bug.
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?
Blocks: 1450250
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(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 :)
Assignee: nobody → emilio
Most of those shouldn't rely on a frame to run.
[Tracking Requested - why for this release]: Regression shipped in 63, would be good not to ship it in 64.
Flags: needinfo?(emilio)
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
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... :(
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/autoland/rev/627720590ce2
Remove SVG use attribute change handling code from nsSVGUseFrame to SVGUseElement. r=longsonr
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/13754 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
https://hg.mozilla.org/mozilla-central/rev/627720590ce2
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
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
Attachment #9020587 - Flags: approval-mozilla-beta?
Flags: qe-verify+
Flags: in-testsuite+
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.
Attachment #9020587 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Depends on: 1502936
Depends on: 1503196
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.