Closed Bug 1251198 Opened 8 years ago Closed 7 years ago

Align document.createEvent() supported events with spec

Categories

(Core :: DOM: Events, defect)

Product:
Core
Component:
DOM: Events
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox47 --- affected
firefox55 --- fixed

People

(Reporter: mozilla, Assigned: ayg)

References

(
URL
)

Details

(Keywords: dev-doc-complete, site-compat, testcase, Whiteboard: dom-triaged btpp-active)

Attachments

(1 file)

Bug 1251198 - Remove various obsolete events from document.createEvent
59 bytes, text/x-review-board-request
smaug
: review+
Details 
Per steps 1-3 of https://dom.spec.whatwg.org/#dom-document-createevent ,
document.createEvent("DragEvent") ought to throw a NOT_SUPPORTED_ERR since there is no entry for DragEvent in the table in step 2 of the algorithm.

Steps to reproduce:
1. Open http://w3c-test.org/dom/nodes/Document-createEvent.html in Firefox

Expected result:
The testcase should fully succeed.

Actual result:
Two tests in the testcase related to DragEvent failed due to the aforementioned deviation from the DOM spec.
Patches welcome.

What do other browsers do here?
Whiteboard: dom-triaged
(In reply to Olli Pettay [:smaug] (HIGH REVIEW LOAD) from comment #1)
> Patches welcome.

The hacktivation energy of browser engines is too high for one-off contributions to be plausible.

> What do other browsers do here?

* Safari doesn't even implement the DragEvent class in the first place, so N/A.
* Chrome doesn't throw yet (but they're working on it; https://bugs.chromium.org/p/chromium/issues/detail?id=569690 )
* MS Edge doesn't throw yet

But Firefox already throws for all the other non-whitelisted event classes I tried, so not throwing for only DragEvent is weirdly self-inconsistent.
Ok, so we don't actually know whether this is a web compatible change.
Given comment #3, I'm marking this as backlog.
Whiteboard: dom-triaged → dom-triaged btpp-backlog
Per https://github.com/w3c/web-platform-tests/pull/2626#issue-136627191 , there are several other event classes which are also affected:
> This reveals that Gecko (Firefox Nightly) supports some of these as well, namely
> BeforeUnloadEvent, CompositionEvent, DeviceMotionEvent,
> DeviceOrientationEvent, DragEvent, DragEvents, HashChangeEvent,
> MutationEvent, MutationEvents, PopStateEvent, SVGEvent, SVGEvents,
> SVGZoomEvent, SVGZoomEvents, StorageEvent, TextEvent, and TextEvents.
The spec whitelists only 11 event names, and we whitelist about 44:

http://hg.mozilla.org/mozilla-central/file/d62963756d9a/dom/events/EventDispatcher.cpp#l848

smaug, Anne: Is there any convincing reason that the spec shouldn't just be updated to include all of Gecko's whitelist?  Once the spec has a whitelist already, why should we bother making it possibly not web-compatible?  We could try to drop these classes and re-add only those that cause breakage, but is it the worth the time and annoyance?
Flags: needinfo?(bugs)
Flags: needinfo?(annevk)
I would support adding those that work in three out of four of Firefox, Safari, Chrome, and Edge. Note that in https://github.com/whatwg/dom/issues/148 we ended up removing one that was only in Firefox (bug 1261874).
Flags: needinfo?(annevk)
Test:

<!DOCTYPE html>
<script>
var tests = [
  "beforeunloadevent",
  "commandevent",
  "commandevents",
  "compositionevent",
  "customevent",
  "datacontainerevent",
  "datacontainerevents",
  "devicemotionevent",
  "deviceorientationevent",
  "dragevent",
  "dragevents",
  "event",
  "events",
  "hashchangeevent",
  "htmlevents",
  "keyboardevent",
  "keyevents",
  "messageevent",
  "mouseevent",
  "mouseevents",
  "mousescrollevents",
  "mutationevent",
  "mutationevents",
  "notifypaintevent",
  "pagetransition",
  "popstateevent",
  "popupevents",
  "scrollareaevent",
  "simplegestureevent",
  "storageevent",
  "svgevent",
  "svgevents",
  "svgzoomevent",
  "svgzoomevents",
  "textevent",
  "textevents",
  "timeevent",
  "timeevents",
  "touchevent",
  "uievent",
  "uievents",
  "xulcommandevent",
  "xulcommandevents",
];
document.documentElement.textContent = "";
for (var i = 0; i < tests.length; i++) {
  try {
    document.createEvent(tests[i]);
    document.documentElement.appendChild(document.createTextNode(tests[i]));
    document.documentElement.appendChild(document.createElement("br"));
  } catch (e) {}
}
</script>

Chrome 49 appears to match the spec here, except they also throw for touchevent.  So we can probably go ahead with this.
Flags: needinfo?(bugs)
IE 11 also throws for most of these, although their whitelist is slightly larger than the spec's.  I don't have a Mac and wasn't willing to compile WebKit from source, so I didn't test them.
Assignee: nobody → ayg
Status: NEW → ASSIGNED
Whiteboard: dom-triaged btpp-backlog → dom-triaged btpp-active
WebKit Nightly's whitelist includes these additional entries:
CompositionEvent
HashChangeEvent
MutationEvent
StorageEvent
SVGZoomEvent
TextEvent
WheelEvent
(In reply to :Aryeh Gregor (working until May 8) from comment #8)
> Chrome 49 appears to match the spec here, except they also throw for
> touchevent.  So we can probably go ahead with this.

I was wrong -- it only throws if the event names are lowercase.  More research required to get a list that's likely to be web-compatible.
What should I do about Gecko-only events like XULCommandEvent, MouseWheelEvent, MouseScrollEvent, DataContainerEvent, etc.?  They have no constructors defined, so the only way to access them is createEvent().  For things like XULCommandEvent and DataContainerEvent which are ChromeOnly anyway, is there some way to special-case chrome code?  For the others, do we need to keep them for now in case there's Gecko-specific code that uses them instead of standard alternatives?  (If so, it might make sense to just add them to the spec, unless that would cause other UAs to break due to browser-sniffing . . .)
Flags: needinfo?(bzbarsky)
The safest option (so that we don't hopefully break addons) for chrome only events which createEvent may create is to make createEvent to take JSContext* and if it is non-null and non-chrome, don't let such events to be created.
'Document' in Bindings.conf could have implicitJSContext for createEvent.

Events which are normally dispatched to content too, and have support in createEvent need to be handled in separately, like in a different bug to the one dealing with chrome-only events so that possibly backouts are easier to track.
And first thing for those non-chrome-only events is to add some warning (in case non-chrome creates them) and telemetry.


Gecko doesn't have MouseWheelEvent.
Flags: needinfo?(bzbarsky)
How do I tell if a JSContext is chrome?
Flags: needinfo?(bugs)
Event::IsChrome is one way. Though, you don't need worker support, so, in case aCx is non-null. 
xpc::AccessCheck::isChrome(js::GetContextCompartment(aCx))
Flags: needinfo?(bugs)
Or even more simply:

  if (!aCx || nsContentUtils::ThreadsafeIsCallerChrome()) 

and document that non-null JSContext should only be passed in if that JSContext represents the caller and you want to be restricted by the sorts of createEvent calls that caller can make.

You could also do:

  if (LegacyIsCallerNativeCode() || IsCallerChrome())

or 

  if (LegacyIsCallerChromeOrNativeCode())

and check with bholley, but that would prevent callers from deciding whether they should be bound by how they themselves were called.
Investigating the non-chrome-only input strings to be removed (all of which are Firefox-only):

* CommandEvent, CommandEvents: I cannot find anywhere in the codebase that fires this event, or any references on MDN or Google.  Maybe it should be removed?
* NotifyPaintEvent: This is fired by nsPresContext::FireDOMPaintEvent, but it is not standard and I don't see any documentation or references on MDN or Google.  Seems unlikely any site is trying to create one.
* SimpleGestureEvent: Cannot find any references anywhere.  We have a quite long test that uses it, but I don't see any actual code that fires it.  Maybe can be removed, or made chrome-only.
* ScrollAreaEvent: Cannot find any references, or anywhere in the code that we fire it.
TimeEvent, TimeEvents: Can't find any references, or where it's fired in the code.
* DragEvents, KeyEvents, MouseScrollEvents, PageTransition (no "Event"), PopUpEvents, SVGEvent, TextEvents: Aliases that no other browser implements.
Gesture events are fired in widget level using  WidgetSimpleGestureEvent, similar with CommandEvent, and ScrollAreaEvent and others.
Summary: document.createEvent("DragEvent") should throw NOT_SUPPORTED_ERR → Align document.createEvent() supported events with spec
Checking the telemetry for all the non-chrome-only events to be removed, from around August 22 to August 30 (n/a means it doesn't appear on the list):

* CommandEvent: 1
* CommandEvents: n/a
* DragEvents: 2.1k
* KeyEvents: 7.35k
* MouseScrollEvents: 1.48k
* NotifyPaintEvent: n/a
* PageTransition (no "Event"): n/a
* PopUpEvents: 1
* SimpleGestureEvent: n/a 
* ScrollAreaEvent: n/a
* SVGEvent: n/a
* TimeEvent: n/a
* TimeEvents: n/a
* TextEvents: n/a

I think DragEvents, KeyEvents, and MouseScrollEvents are probably used in our own JS.  These should be switched to use the standard variants and then re-evaluated.

All the others look safe to remove.  I assume that if there's no entry, it's because no data at all was received.  Compare to the data for "Event", which received 193.29k samples.

Keep in mind that every single one of these event names is implemented by no other UA.
So MouseScrollEvents may pose a bit of a problem.  Some sites seemingly use it, but no other UA supports it as a createEvent() argument, and indeed Edge and Chrome do not have window.MouseScrollEvent.  It is presumably being used in Gecko-specific code via browser sniffing in old sites.  What do we do?  I don't know if we can drop support, but I don't know if we can spec it either.  If we do spec it, presumably we'll have to spec the interface as well, because compat-wise it's probably useless to return something that doesn't work from createEvent().

What should we do?
Flags: needinfo?(bugs)
Flags: needinfo?(annevk)
We can't drop DOMMouseScroll nor MozPixelScroll event. They are used effectively when mousewheel is used with other browsers. mousewheel isn't spec'ed either. For example by Google Spreadsheets use these legacy events.

All the sites _should_ use wheel event which is supported by everyone.
Flags: needinfo?(bugs)
I guess we could start warn about use of the legacy events and hint that wheel should be used.
It looks like we'll need to support MouseScrollEvents for a long time until pages migrate to the wheel event.  Does it make sense to change the telemetry for this one event to not expire so that we'll be able to watch it over time and remove support if it ever eventually gets close to zero, maybe years from now?
Flags: needinfo?(benjamin)
Sounds like you have a plan of sorts. Deprecation makes sense if nobody else is going to support it.
Flags: needinfo?(annevk)
It might make sense, if telemetry is a valid signal for the question you want to answer. I don't think telemetry actually tells you that much, though.
Flags: needinfo?(benjamin)
Total submissions received for 51:

* CommandEvent: 7
* CommandEvents: 1
* DragEvents: 27 (from September 2 onward, bug 1299453)
* KeyEvents: 25.33k (bug 1299453 doesn't seem to have helped)
* MouseScrollEvents: 7.07k
* NotifyPaintEvent: 4
* PageTransition (no "Event"): 1
* PopUpEvents: 6
* ScrollAreaEvent: 4
* SimpleGestureEvent: 4 
* SVGEvent: 1
* TextEvents: 1
* TimeEvent: 4
* TimeEvents: 1

For contrast, 1.01m submissions hit "Event".  So for all of these except KeyEvents and MouseScrollEvents, we're talking about something like 1 in 100,000-1,000,000 telemetry submissions that hit the relevant event, where one telemetry submission will often include a day's usage.  I don't think it's possible to expect anything lower, so I suggest support for all of these be dropped except KeyEvents and MouseScrollEvents, and possibly DragEvents.

What do you think?  We could request that all other browsers support all event types that we support even though usage is virtually zero and no other browser supports any of them, but that doesn't seem very reasonable.
Flags: needinfo?(bugs)
By the way, in bug 1031051 comment 33 and bug 1031051 comment 34 you asked for "popstateevent" and "pagetransition" to be removed, but they were left in by mistake.
sounds ok to me to remove use of those rarely used events, but please check also addon usage for these events.
Flags: needinfo?(bugs)
* PopStateEvent: 8

This is currently in the spec, bug <https://github.com/whatwg/dom/issues/362> suggests that it be removed.  We only support it by mistake anyway, bug 1031051 comment 33 asked for it to be removed and it wasn't.

I haven't been able to check usage of these events in add-ons, because DXR is giving me errors when I search.  But the telemetry should show add-on usage too, right?
Comment on attachment 8860035 [details]
Bug 1251198 - Remove various obsolete events from document.createEvent

Does this MozillaFileLogger.js change look good to you?  I can't find anywhere in the tree that uses this event, so I assume it's an out-of-tree user that needs to be updated somehow?  You just need to change e.getData("foo") to e.detail.foo.

By the way, I don't think JSON.stringify is needed here, if you want to pass the object directly instead of a serialization while we're at it.  (It doesn't look like it was ever needed.)
Attachment #8860035 - Flags: feedback?(jmaher)
Comment on attachment 8860035 [details]
Bug 1251198 - Remove various obsolete events from document.createEvent

this looks great to me- just verify that talos runs and posts to perfherder properly.
Attachment #8860035 - Flags: feedback?(jmaher) → feedback+
How do I do that?  Are any further changes needed?  The consumer of the event will presumably no longer work after this change, because it needs to access .detail instead of .getData().
Flags: needinfo?(jmaher)
I think pushing to try is all I need with the talos runs done.
Flags: needinfo?(jmaher)
Attachment #8860035 - Flags: review?(bugs)
(Note the test update to test_bug790732.html that I just pushed.  That needs review.)
Comment on attachment 8860035 [details]
Bug 1251198 - Remove various obsolete events from document.createEvent

https://reviewboard.mozilla.org/r/132058/#review135668

::: commit-message-722fd:12
(Diff revision 3)
> +
> +CommandEvent and SimpleGestureEvent: These are not supposed to be
> +web-exposed APIs, so I hid the interfaces from web content too
> +(necessary to avoid test_all_synthetic_events.html failures).
> +
> +DataContainerEvent: This was a non-standard substitute for CustomEvent

It is not a substitute for CustomEvent. Very different API. But removing it is fine.

::: commit-message-722fd:16
(Diff revision 3)
> +
> +DataContainerEvent: This was a non-standard substitute for CustomEvent
> +that seemed to have only one user, so I removed it entirely and switched
> +the user (MozillaFileLogger.js) to CustomEvent.
> +
> +KeyEvents and MouseScrollEvents: Unfortunately we seem to still have

Useless comment in a commit message

::: commit-message-722fd:20
(Diff revision 3)
> +
> +KeyEvents and MouseScrollEvents: Unfortunately we seem to still have
> +high usage, despite the fact that no other browser supports them, so I
> +didn't remove them.
> +
> +MutationEvent(s): I didn't remove these, even though they're not in the

Also useless comment in this commit message.

::: dom/events/EventDispatcher.cpp:1114
(Diff revision 3)
>    }
>  
> +  // Only allow these events for chrome
> +  if (aAllowChrome == AllowChrome::eAlways ||
> +      !nsContentUtils::GetCurrentJSContext() ||
> +      nsContentUtils::ThreadsafeIsCallerChrome()) {

Please no more IsCallerChrome usage.
Webidl has a way to pass the caller type if needed.
Attachment #8860035 - Flags: review?(bugs) → review-
Does this look okay?  I don't know if I did it correctly -- I don't see any results for Talos.  https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6e5da8e31df
Flags: needinfo?(jmaher)
talos only runs on opt, can you push with: try -b o -p linux64 -u none -t all
Flags: needinfo?(jmaher)
Comment on attachment 8860035 [details]
Bug 1251198 - Remove various obsolete events from document.createEvent

https://reviewboard.mozilla.org/r/132058/#review135824

::: testing/talos/talos/scripts/MozillaFileLogger.js:12
(Diff revision 5)
>  function contentDispatchEvent(type, data, sync) {
>    if (typeof(data) === "undefined") {
>      data = {};
>    }
>  
> -  var element = document.createEvent("datacontainerevent");
> +  var element = new CustomEvent("contentEvent", {

So the API to access the data is different. Why you don't need to update the listener?
Attachment #8860035 - Flags: review?(bugs) → review+
Does this look good to you?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ee126064077938b7f2f383690b8affc16e41dc7

(In reply to Olli Pettay [:smaug] from comment #45)
> So the API to access the data is different. Why you don't need to update the
> listener?

Maybe nothing actually uses this.  I'm relying on jmaher here.
Flags: needinfo?(jmaher)
oh, thanks for the try push!
Flags: needinfo?(jmaher)
all good btw
Comment on attachment 8860035 [details]
Bug 1251198 - Remove various obsolete events from document.createEvent

https://reviewboard.mozilla.org/r/132058/#review135824

> So the API to access the data is different. Why you don't need to update the listener?

(replying on ReviewBoard although I already replied on the bug)

jmaher said to do a try run with Talos, and I did and it was green, so he said it's fine.  Maybe there are no actual listeners.
Pushed by ayg@aryeh.name:
https://hg.mozilla.org/integration/autoland/rev/7d70c64683b8
Remove various obsolete events from document.createEvent r=smaug
https://hg.mozilla.org/mozilla-central/rev/7d70c64683b8
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1359747
Hello.

This bug may have contributed to a sudden change in the Telemetry probe CREATE_EVENT_XULCOMMANDEVENT[1] which appears to have occurred in Nightly 20170426[2]. There was a significant decrease in counts across the board[3] which probably reflects the use of the XUlCommandEvent ctor instead of CreateEvent.

Is this a good thing?

Is this intentional?

Is this probe measuring something useful?

[1]: http://alerts.telemetry.mozilla.org/index.html#/detectors/1/metrics/1959/alerts/?from=2017-04-26&to=2017-04-26
[2]: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a30dc237c3a600a5231f2974fc2b85dfb5513414&tochange=0f5ba06c4c5959030a05cb852656d854065e2226
[3]: https://mzl.la/2pxuFsD

(( would've ni?ayg, but ayg's not accepting ni atm. To :smaug then ))
Flags: needinfo?(bugs)
> Is this a good thing?

Probably.

> Is this intentional?

Yes.

> Is this probe measuring something useful?

Good question.  At this point it's measuring calls to createEvent("xulcommandevent") from chrome code.  If this ever goes to zero, we can probably remove the chrome-only support we still have for that.
Flags: needinfo?(bugs)
I've had a go at documenting this. The only real mention I could see was this page:

https://developer.mozilla.org/en-US/docs/Web/API/Document/createEvent

In the notes section (https://developer.mozilla.org/en-US/docs/Web/API/Document/createEvent#Notes) we had a big table full of event types, but it was largely out of date. I've deleted most of it and just left in the information about the non-standard gecko event types we still support, and linked to the table in the DOM spec for the supported types. This way it will be up to date and stay current.

I've also added a note to the Fx55 release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/55#DOM_HTML_DOM

Let me know if this looks OK. Thanks!
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: