whatwg / dom

DOM Standard
https://dom.spec.whatwg.org/
Other
1.56k stars 289 forks source link

document.createEvent() supports lots of events that most UAs do not support initializers for #362

Closed ayg closed 7 years ago

ayg commented 7 years ago

The current whitelist of event names for document.createEvent() is based on events that at least two of Chrome, Firefox, Safari, Edge support. However, when generating the list, I didn't test whether they support a corresponding init*Event() method on the interface. Without that, support in createEvent() seems pointless. Here's a new test that lists not only what interfaces are supported by document.createEvent(), but what init methods are found:

https://jsfiddle.net/s53w679y/1/

The result is that a lot of them are supported with an initializer only in Edge, and supported without an initializer in Chrome (and in a few cases Edge as well). To wit, all of the following are unsupported in Firefox and Safari, and are supported in Chrome but without an initializer method:

And similarly:

I suggest all of these be removed from the spec, unless we think pages really use events that they can't initialize. Maybe they use the default values for all the properties? In some cases there are still initializers like initUIEvent from base classes.

@domenic, do you know why Chrome supports so many event types from createEvent() even though it has no initializer? Would you have a problem with not supporting these in createEvent() anymore?

@bzbarsky, do you know why Gecko supports SVGZoomEvent(s) from createEvent() even though it has no initializer?

@khuey In https://hg.mozilla.org/mozilla-central/rev/1f60138bce8d, why did you leave PopStateEvent support in createEvent but removed the initializer? Just to match Chrome? It looks like https://bugzilla.mozilla.org/show_bug.cgi?id=1031051#c33 asked to remove it, but you didn't.

A couple of other event types that are interesting:

ayg commented 7 years ago

In case I (or someone else without Safari) needs it, here's a link to the Safari results from the test-case, courtesy of gsnedders: http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=4631

domenic commented 7 years ago

@domenic, do you know why Chrome supports so many event types from createEvent() even though it has no initializer? Would you have a problem with not supporting these in createEvent() anymore?

As flattered as I am being your go-to person for Chrome questions, @foolip is probably a better choice, as he's closer to the implementation work :). In this case I think I can anticipate what he would say though. From what I understand, for a long time Chrome automatically made all events that were added automatically work with createEvent(). Recently @foolip locked that down so that new ones no longer get added to the works-with-createEvent list, freezing that list in place, and he's hoping to over time drive that list down to the one that matches the spec.

It seems plausible to me that Chrome would be able to cut down the list even more, especially if in practice createEvent()ing these events is useless due to lack of init*Event(), but I'll defer to @foolip there.

khuey commented 7 years ago

@khuey In https://hg.mozilla.org/mozilla-central/rev/1f60138bce8d, why did you leave PopStateEvent support in createEvent but removed the initializer? Just to match Chrome? It looks like https://bugzilla.mozilla.org/show_bug.cgi?id=1031051#c33 asked to remove it, but you didn't.

Most likely. Must have missed that review comment. I don't remember much about this (it was a couple years ago) beyond what's in the bug.

bzbarsky commented 7 years ago

do you know why Gecko supports SVGZoomEvent(s) from createEvent() even though it has no initializer

No idea offhand. Probably because that's what SVG specified, but it's possible it happened by accident in https://bugzilla.mozilla.org/show_bug.cgi?id=234455. It's been a while since we've had this behavior, as you can tell from the dates on that bug.

Note that this event interface has no constructor either....

foolip commented 7 years ago

@domenic has the history exactly right in https://github.com/whatwg/dom/issues/362#issuecomment-257628252.

I would be quite happy to reduce the list of events further. In https://bugs.chromium.org/p/chromium/issues/detail?id=603614#c7 and beyond I took @ayg to be skeptical of that enterprise, but that was before considering init*Event methods.

These are the events mentioned in the first comment, linked to Blink's use counters for use with document.createEvent:

FocusEvent and TransitionEvent stand out in terms of usage. Investigating what's going on with those using httparchive analysis might reveal something interesting.

Both AnimationEvent and TransitionEvent are also special because Blink also supports WebKitAnimationEvent and WebKitTransitionEvent, and the later has curiously high usage.

For completeness, these are the other events still supported and measured in Blink that aren't mentioned above:

So I'd add "DragEvent" and "HashChangeEvent" the the list of endangered strings.

foolip commented 7 years ago

@ayg, WDYT, what's the set of strings you'd like to get rid of?

ayg commented 7 years ago

@foolip I'm happy to get rid of all the events in the first post here. If you can't remove FocusEvent or TransitionEvent right now, then removing all the others is a good start. I don't see any need to remove events that are already implemented by existing UAs -- it just creates work and possible site compat issues for no advantage. It's more useful to drop events that some UAs don't support, if the UAs that do support them are willing to drop support.

ayg commented 7 years ago

@foolip So practically, is Blink willing to drop all the events in the first comment here (except BeforeUnloadEvent)? In particular, I'm not entirely sure about WheelEvent.

foolip commented 7 years ago

@tkent-google has recently removed createEvent support for WebKitAnimationEvent and WebKitTransitionEvent, which means there's no reason to keep AnimationEvent or TransitionEvent beyond their own usage.

Of the things mentioned in the first comment, these have so low usage that I think removal is likely to work out in Blink:

Those that would require some investigation are FocusEvent and TransitionEvent.

As stated in https://github.com/whatwg/dom/issues/362#issuecomment-259532878, I'd also support removing DragEvent and HashChangeEvent from the spec.

foolip commented 7 years ago

Actually, in @tkent-google did some research for WebKitTransitionEvent, and the same code would also trigger the TransitionEvent use counter. So it's quite possible it can be removed as well.

ayg commented 7 years ago

Usage of HashChangeEvent is negligible in Firefox, so I don't mind removing that too. DragEvent is low but not as low and I don't think we want to get rid of it unless there's good reason. I'm

We have super-high usage for (BeforeUnloadEvent)[https://telemetry.mozilla.org/new-pipeline/evo.html#!aggregates=submissions&cumulative=0&end_date=2016-08-28&keys=!__none__!__none__&max_channel_version=nightly%252F51&measure=CREATE_EVENT_BEFOREUNLOADEVENT&min_channel_version=nightly%252F48&processType=*&product=Firefox&sanitize=0&sort_keys=submissions&start_date=2016-08-22&trim=0&use_submission_date=0], but that's probably because we use it internally. I'd have to remove the internal user in order to have useful telemetry. But we'd also have to add a constructor here, no? Or do we want the event to be fired by the browser but authors can't create it?

@annevk So it looks like the following are all either unsupported in Firefox/Chrome/Safari already, or we're willing to remove support:

Edge supports all of these events, I'm guessing just because they're following the spec and not because sites depend on them.

So I think the right course of action now is to ask Safari if they're willing to drop support for WheelEvent, and get confirmation from Chrome that they can drop TransitionEvent, and then drop all of these from the spec.

We also need to figure out what to do about BeforeUnloadEvent. It's fired by the browser, and has no constructor, so do we want to keep createEvent support? Or add a constructor? Or not let authors create it although the browser fires it? (Is the last what Edge/Safari do now?)

annevk commented 7 years ago

@cdumez can probably comment on behalf of Safari whether they can drop WheelEvent and @foolip should be able to comment on TransitionEvent. @domenic touched BeforeUnloadEvent last.

foolip commented 7 years ago

Edge supports all of these events, I'm guessing just because they're following the spec and not because sites depend on them.

Support probably predates the spec, maybe the supported createEvent for all event interfaces by default, like Blink and WebKit did.

foolip commented 7 years ago

Regarding TransitionEvent, document.createEvent("TransitionEvent") already doesn't work in Safari, so now it's supported by Chrome and Edge only. I can't promise to attempt the removal myself in Blink, but I think it'd be fine to remove it from the spec and add it back only if we (or Edge) eventually request it due to compat concerns.

domenic commented 7 years ago

I have mixed feelings about BeforeUnloadEvent. On the one hand, constructors are a good thing, and maybe people want it for mocking or something. On the other hand, beforeunload is a terrible event that nobody should use, so making it more "magic" by making it only browser-constructible might be nice.

Given that nobody implements a constructor, I'm not sure it's worth adding one. I'm not sure where that leaves us for createEvent though.

foolip commented 7 years ago

For BeforeUnloadEvent, removing createEvent support without adding a constructor sounds fine. We could punt on that until @ayg (or someone) has removed the internal usage and seen usage drop in Gecko though.

ayg commented 7 years ago

I filed Mozilla bug 1358893 on removing our internal usage of document.createEvent("beforeunloadevent").

cdumez commented 7 years ago

I am willing to try dropping WheelEvent in WebKit, assuming other major browsers do not support it.

ayg commented 7 years ago

I think our list is finalized for now, then. It will take time for Mozilla to get data on if we can drop BeforeUnloadEvent (although I'd guess we can). Per whatwg/html#2575, though, I think we should add a constructor, just for consistency with other specced events.

tkent-google commented 7 years ago

Actually, in @tkent-google did some research for WebKitTransitionEvent, and the same code would also trigger the TransitionEvent use counter. So it's quite possible it can be removed as well.

Dropping TransitionEvent isn't safe. Popular libraries have the following code, and dropping both of WebKitTransitionEvent and TransitionEvent changes the behavior.

cssTransitions: function() {
  var b=!1;
  return a.each(["WebKitTransitionEvent","TransitionEvent","OTransitionEvent"],function(a,c){
    try{
      document.createEvent(c),b=!0
    } catch(d){}
  }),b
}
foolip commented 7 years ago

@tkent-google, do you have any context about how cssTransitions ends up being used? Firefox ought to already be taking the code paths where something.cssTransitions() is false, so perhaps it's not a serious problem?

This is from http://projects.nickstakenburg.com/tipped and http://projects.nickstakenburg.com/lightview, so @staaky FYI.

tkent-google commented 7 years ago

@tkent-google, do you have any context about how cssTransitions ends up being used?

I took a look at Tipped code, and found removing both of WebKitTransitionEvent and TransitionEvent would cause:

foolip commented 7 years ago

That sounds quite promising, the kind of compat risk we'd probably accept if removing a Blink-only misfeature. Here there are 3 engines that'd have to change. I still lean towards removal, but @cdumez, WDYT?

ayg commented 7 years ago

document.createEvent("TransitionEvent") is only supported in Chrome and Edge, so I count only two that have to change? That said, supporting it with no initializer is silly but not difficult or a compat risk, so if you don't want to change because of compat risk, I'm fine with adding it to Firefox.

foolip commented 7 years ago

Oops, I just assumed Safari support. Then I think we should definitely try dropping TransitionEvent as well.

annevk commented 7 years ago

Note that while this is fixed it might take some time before this reaches the published version due to some infrastructure issue. Sorry about that.