Remove SVGZoomEvent

RESOLVED FIXED in Firefox 55

Status

()

Core
DOM: Events
P3
normal
RESOLVED FIXED
2 years ago
8 months ago

People

(Reporter: ayg, Assigned: ayg)

Tracking

(Depends on: 1 bug, {dev-doc-complete, site-compat})

unspecified
mozilla55
dev-doc-complete, site-compat
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

I looked at the code briefly, but couldn't see what the problem is.  Every other event type seems to be fine.

Test: http://w3c-test.org/dom/nodes/Document-createEvent.html (lots of noise, search for "SVGZoomEvent")
SVGZoomEvent is going away in SVG 2 per https://www.w3.org/TR/SVG2/changes.html so what that's testing is basically that there isn't an SVGZoomEvent. 

We have one defined but I don't think we dispatch it so removing it shouldn't be a problem.
What it's testing is this: https://dom.spec.whatwg.org/#dom-document-createevent "Initialize event’s type attribute to the empty string."  createEvent() is always supposed to initialize to the empty string, but here it doesn't.

There appears to be no way to initialize the values, so if we don't dispatch it anywhere, and it's not supposed to exist per new specs, maybe we can try to remove it entirely.  What do you think, smaug?  Note that Safari and Edge support document.createEvent("SVGZoomEvents") (but not "SVGZoomEvent") but have no way to initialize it AFAICT, and Chrome does not seem to support SVGZoomEvent at all (window.SVGZoomEvent undefined).
Flags: needinfo?(bugs)

Comment 3

2 years ago
dholbert or longsonr might have an opinion on this. 

It is mystery to me why SVGZoomEvent::SVGZoomEvent passes eSVGZoom as type, when everybody else uses eVoidEvent.

Do we have telemetry data about the usage?
Flags: needinfo?(bugs) → needinfo?(dholbert)
It's eSVGZoom because that's what the SVG 1.1 specification says in https://www.w3.org/TR/SVG/script.html#InterfaceSVGZoomEvent

The UI event type for a zoom event is:

SVGZoom

Having said that this event is removed entirely in SVG 2 and Gecko has never dispatched it anyway (It was supposed to fire when the user initiates an action which causes the current view of the SVG document fragment to be rescaled).
Per info from longsonr in comment 1 and comment 4, it sounds like we should just remove this event.
Flags: needinfo?(dholbert)

Updated

2 years ago
Priority: -- → P3

Updated

2 years ago
Summary: document.createEvent("SVGZoomEvent").type == "SVGZoom" but should be "" → Make document.createEvent("SVGZoomEvent") throw
Assignee: nobody → ayg
Status: NEW → ASSIGNED
Summary: Make document.createEvent("SVGZoomEvent") throw → Remove SVGZoomEvent
Comment hidden (mozreview-request)
Attachment #8852478 - Flags: review?(longsonr)
The submitted patch has a try run with failures in one file that I fixed, so it should be green:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=07e9600487b589ecd190fcf304743c70cc3f63cc&selectedJob=87051365
Attachment #8852478 - Flags: review?(longsonr) → review+
SVGSVGElement::SetCurrentScaleTranslate shouldn't get simplified further?  The comment suggests that a bunch of the code is only there for SVGZoomEvent.
Flags: needinfo?(longsonr)
Perhaps, maybe best looked into as a followup though.
Flags: needinfo?(longsonr)
Comment hidden (mozreview-request)
(I don't see how to autoland it without you re-reviewing it, although docs suggest there should be a way, because I have L3 commit access . . .)
Depends on: 1352027
Comment hidden (mozreview-request)
Attachment #8852478 - Flags: review?(longsonr) → review?(padenot)

Comment 14

a year ago
mozreview-review
Comment on attachment 8852478 [details]
Bug 1314388 - Remove SVGZoomEvent;

https://reviewboard.mozilla.org/r/124684/#review127608
Attachment #8852478 - Flags: review?(padenot) → review+
Attachment #8852478 - Flags: review?(bugs)
Keywords: dev-doc-needed
Attachment #8852478 - Flags: review+

Comment 16

a year ago
mozreview-review
Comment on attachment 8852478 [details]
Bug 1314388 - Remove SVGZoomEvent;

https://reviewboard.mozilla.org/r/124684/#review128156

Based on the feedback from dholbert this should be fine.
Attachment #8852478 - Flags: review?(bugs) → review+

Comment 17

a year ago
Pushed by ayg@aryeh.name:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7fab6a89c1d
Remove SVGZoomEvent; r=longsonr,smaug
Backed out for timing out in dom/svg/test/test_zoom.xhtml:

https://hg.mozilla.org/integration/mozilla-inbound/rev/89b885c2cb80f6f346205d986933fef47001738b

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=804d8199aecadaf1948044b1b4520963056fee67&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=88143220&repo=mozilla-inbound

05:14:53     INFO - TEST-START | dom/svg/test/test_zoom.xhtml
05:20:05     INFO - TEST-INFO | started process screencapture
05:20:05     INFO - TEST-INFO | screencapture: exit 0
05:20:05     INFO - TEST-UNEXPECTED-FAIL | dom/svg/test/test_zoom.xhtml | Test timed out. 
05:20:05     INFO -     reportError@SimpleTest/TestRunner.js:120:7
05:20:05     INFO -     TestRunner._checkForHangs@SimpleTest/TestRunner.js:141:7
05:20:05     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:162:5
05:20:05     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:162:5
05:20:05     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:162:5
05:20:05     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:162:5
05:20:05     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:162:5
05:20:05     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:162:5
05:20:05     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:162:5
05:20:05     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:162:5
05:20:05     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:162:5
05:20:05     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:162:5
05:20:05     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:162:5
05:20:05     INFO -     TestRunner.runTests@SimpleTest/TestRunner.js:379:5
05:20:05     INFO -     RunSet.runtests@SimpleTest/setup.js:190:3
05:20:05     INFO -     RunSet.runall@SimpleTest/setup.js:169:5
05:20:05     INFO -     hookupTests@SimpleTest/setup.js:262:5
05:20:05     INFO - parseTestManifest@http://mochi.test:8888/manifestLibrary.js:36:5
05:20:05     INFO - getTestManifest/req.onload@http://mochi.test:8888/manifestLibrary.js:49:11
05:20:05     INFO - EventHandlerNonNull*getTestManifest@http://mochi.test:8888/manifestLibrary.js:45:3
05:20:05     INFO -     hookup@SimpleTest/setup.js:242:5
05:20:05     INFO - EventHandlerNonNull*@http://mochi.test:8888/tests?autorun=1&closeWhenDone=1&consoleLevel=INFO&hideResultsTable=1&manifestFile=tests.json&dumpOutputDirectory=%2Fvar%2Ffolders%2F9j%2F83h02rbx6bn60ztt0ypr7lsm00000w%2FT:11:1
Flags: needinfo?(ayg)
Is it correct that this test breaks?  It seems like onzoom previously actually fired an event when .currentScale was changed, and now it does not.  I thought we didn't actually fire an event here.  Should I just remove this test?
Flags: needinfo?(ayg) → needinfo?(longsonr)
I guess we do fire a zoom event. I imagine we can't remove this after all. Not sure what jwatt and cam think.
Flags: needinfo?(longsonr)
I don't know who cam is, could someone needinfo him also?
Flags: needinfo?(jwatt)
Flags: needinfo?(cam)
(In reply to Aryeh Gregor (:ayg) (working March 28-April 26) from comment #20)
> Is it correct that this test breaks?  It seems like onzoom previously
> actually fired an event when .currentScale was changed, and now it does not.
> I thought we didn't actually fire an event here.

Indeed we do fire an event - in fact you're removing the HandleDOMEventWithTarget() call that dispatches it from SVGSVGElement::SetCurrentScaleTranslate in your patch.
Flags: needinfo?(jwatt)
(In reply to Aryeh Gregor (:ayg) (working March 28-April 26) from comment #2)
> Note that Safari and Edge
> support document.createEvent("SVGZoomEvents") (but not "SVGZoomEvent")

Bizarre.

> Chrome does not seem to support
> SVGZoomEvent at all (window.SVGZoomEvent undefined).

That doesn't mean it's not defined. It could just be [NoInterfaceObject]. We should also check if other browsers actually fire the the thing.
Created attachment 8853777 [details]
Test if SVG zoom events fire

Chrome and Safari don't fire SVG zoom events. Someone should really check Edge on this testcase too, but removing the testcase and relanding is fine by me.
I've been told that Edge does actually alert "onzoom" and "aEL SVGZoom" here.  Now what?
Flags: needinfo?(jwatt)
I think if Chrome didn't see enough demand to fix their SVGZoom event behavior and keep it (before they removed it last year) then there isn't enough demand to keep it. For performance and UX reasons we'd also prefer to provide declarative ways to achieve the viewport relative and zoom agnostic positioning/sizing of elements that SVGZoom enables. So let's just remove it. (We can add new and better facilities as and when demand requires it.)
Flags: needinfo?(jwatt)

Comment 28

a year ago
Pushed by ayg@aryeh.name:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6cbd404c845
Remove SVGZoomEvent; r=longsonr,smaug
Flags: needinfo?(cam)

Comment 29

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b6cbd404c845
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2017/svgzoomevent-has-been-removed/
Keywords: site-compat
I've removed any mention of the onzoom attribute that appeared on 
https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute

I've also added a note to the Fx55 release notes:
https://developer.mozilla.org/en-US/Firefox/Releases/55#SVG

(We didn't document SVGZoomEvent/SVGZoomEvents in the first place).

I think that covers it.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.