1314388 - Remove SVGZoomEvent

Status: RESOLVED FIXED

(Core :: DOM: Events, defect, P3)

Description

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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

Comment 1

[Robert Longson [:longsonr]]

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.

Comment 2

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 4

[Robert Longson [:longsonr]]

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

Comment 5

[Daniel Holbert [:dholbert]]

Per info from longsonr in comment 1 and comment 4, it sounds like we should just remove this event.

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Related: https://github.com/whatwg/dom/issues/283

Comment 7

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Bug 1314388 - Remove SVGZoomEvent;

XX Some code in SVGSVGElement.cpp might be unnecessary now, please
advise when reviewing.

Review commit: https://reviewboard.mozilla.org/r/124684/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/124684/

Comment 8

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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

Comment 9

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

SVGSVGElement::SetCurrentScaleTranslate shouldn't get simplified further?  The comment suggests that a bunch of the code is only there for SVGZoomEvent.

Comment 10

[Robert Longson [:longsonr]]

Perhaps, maybe best looked into as a followup though.

Comment 11

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 8852478 [details]
Bug 1314388 - Remove SVGZoomEvent;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124684/diff/1-2/

Comment 12

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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

Comment 13

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 8852478 [details]
Bug 1314388 - Remove SVGZoomEvent;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124684/diff/2-3/

Comment 14

[Paul Adenot (:padenot)]

Comment on attachment 8852478 [details]
Bug 1314388 - Remove SVGZoomEvent;

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

Comment 15

[Robert Longson [:longsonr]]

Comment on attachment 8852478 [details]
Bug 1314388 - Remove SVGZoomEvent;

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

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7fab6a89c1d
Remove SVGZoomEvent; r=longsonr,smaug

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/804d8199aeca
Fix bustage on CLOSED TREE

Comment 19

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

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

Comment 20

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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?

Comment 21

[Robert Longson [: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.

Comment 22

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

I don't know who cam is, could someone needinfo him also?

Comment 23

[Jonathan Watt [:jwatt]]

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

Comment 24

[Jonathan Watt [: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.

Comment 25

[Jonathan Watt [:jwatt]]

Attachment: 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.

Comment 26

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

I've been told that Edge does actually alert "onzoom" and "aEL SVGZoom" here.  Now what?

Comment 27

[Jonathan Watt [: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.)

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6cbd404c845
Remove SVGZoomEvent; r=longsonr,smaug

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b6cbd404c845

Comment 30

[Kohei Yoshino [:kohei]]

Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2017/svgzoomevent-has-been-removed/

Comment 31

[Chris Mills [:cmills]]

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.