Closed Bug 1502658 Opened Last year Closed Last year
SVG ignores HREF attribute changes if these changes were made while SVG is hidden
46 bytes, text/x-phabricator-request
|Details | Review|
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?
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.
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 firstname.lastname@example.org: 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.
Upstream PR merged
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?
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+
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.
You need to log in before you can comment on or make changes to this bug.