initEvent should not require three parameters #387

achristensen07 · 2016-12-20T23:02:21Z

I have found many instances of web content calling initEvent with only one parameter. Matching the current spec makes these pages not work, showing up as unresponsive blank white pages.

I therefore propose changing this:
void initEvent(DOMString type, boolean bubbles, boolean cancelable); // historical
to this:
void initEvent(DOMString type, optional boolean bubbles = false, optional boolean cancelable = false); // historical

achristensen07 · 2016-12-20T23:08:52Z

Maybe the type should be optional, too.

https://bugs.webkit.org/show_bug.cgi?id=166284 Reviewed by Dan Bernstein. LayoutTests/imported/w3c: * web-platform-tests/dom/interfaces-expected.txt: We regress conformance, but a change has been proposed to the spec. Hopefully this will be a temporary regression. Source/WebCore: We added this quirk in r207908 and expanded it in r209946 and r210034. Many web compatibility issues have been found, which leads us to believe that many more exist. This reverts the behavior to match how it was before r203848. This change was proposed to the spec in whatwg/dom#387 * dom/Event.cpp: (WebCore::Event::initEventForBindings): Deleted. * dom/Event.h: * dom/Event.idl: * platform/RuntimeApplicationChecks.h: * platform/RuntimeApplicationChecks.mm: (WebCore::IOSApplication::isBaiduNuomi): Deleted. (WebCore::IOSApplication::isAutoNaviAMap): Deleted. (WebCore::IOSApplication::isFlipboard): Deleted. git-svn-id: http://svn.webkit.org/repository/webkit/trunk@210045 268f45cc-cd09-0410-ab3c-d52691b4dbfc

annevk · 2016-12-21T07:00:43Z

That means those pages break in browsers other than Safari?

achristensen07 · 2016-12-21T08:36:28Z

One example was web content packaged inside an iOS app. Only WebKit would ever read it.
One example was web content that opened successfully in Chrome and Safari but not Firefox because Chrome and Safari do not require 3 parameters and Firefox does.
I couldn't easily find the original source of the other example to try loading it in Chrome and Firefox. WebKit's attempt at matching the current spec broke an iOS app that is very popular in Asia.

All the affected content had one parameter in the call site of initEvent. I don't see a reason to require the other two, especially if Chrome and Safari don't require them.

annevk · 2016-12-22T06:00:09Z

@foolip do you know if there are other instances where Chrome does not require arguments?

Does seem good to fix this if both Chrome and Safari match.

Do you know what Edge does?

achristensen07 · 2016-12-23T07:58:00Z

Edge requires three parameters.

cdumez · 2017-01-03T17:09:39Z

To clarify Alex's comment, we did not find any evidence of actual Web breakage. However, matching the specification in WebKit broke a lot of iOS apps using WebViews and at least one Safari extension.

annevk · 2017-01-03T17:39:39Z

That's generally not a good enough reason to change the standard, but if @foolip and @smaug---- et al would support adding it to Chrome and Firefox I don't see much reason to stop this. It's a rather trivial IDL change.

smaug---- · 2017-01-03T17:54:25Z

Any actual web pages breaking in FF or Edge because of this? Links?
iOS apps or extensions don't count here.

How do all the other init*Event methods behave?

cdumez · 2017-01-03T18:07:27Z

As I said, we have no evidence of actual Web breakage. Sadly, there are a lot of WebKit-specific apps / extensions calling initEvent() with a single parameter. We usually deal with this by adding quirks until those apps get fixed. However, in this case, it seems to be a common pattern and it is breaking a lot of iOS apps.

I agree that this is not a very compelling argument to get the specification changed but Alex thought it was worth trying given the pain we're currently experiencing. Also note that those 2 boolean parameters are not required when you use the Event constructor (i.e. You can call the Event constructor with only a type) and those have default values (false) for the constructor. Therefore, having initEvent() behave similarly wouldn't look too surprising.

cdumez · 2017-01-03T18:25:17Z

Also note that Chrome 55 behaves does not have initEvent() parameters as mandatory. So Safari and Chrome are in agreement at least.

smaug---- · 2017-01-03T18:28:11Z

As I asked, how do other init*Event methods behave?

cdumez · 2017-01-03T18:37:40Z

In WebKit, parameters to all init*Event() methods are optional (except for initCustomEvent). The same appears to be true for Chrome based on grep'ing of their code.

Note that the parameters to initCustomEvent() were optional in WebKit until recently (not even sure this shipped in Safari yet).

cdumez · 2017-01-06T00:09:13Z

Any thoughts on this? We had to roll this change out in WebKit because it broke way too many popular iOS apps (including Flipboard and WeChat).

foolip · 2017-01-06T00:18:21Z

Not long before this issue was filed, I suggested that @LoonyBean try fixing this in Blink, giving it good chances of working out because Firefox and Edge would already throw.

If there is a way for WebKit to make the change, then that'd be great. However, different behavior for a browser and WebView seems the only option, and not a good one.

So, I think we should make the two boolean arguments optional. I'd support doing the same for all init*Event methods, requiring arguments only up to the last argument corresponding to a required dictionary member. (Otherwise the init*Event* methods can make some attributes null. Another way would be to just make the attributes nullable and sigh.)

annevk · 2017-01-06T07:46:28Z

Is there even a legacy event that has init*Event and a required dictionary member? Seems unlikely.

cdumez · 2017-02-08T00:10:46Z

Any update on this? Would anyone oppose if I wrote to PR to update the DOM spec and the web-platform-tests?

annevk · 2017-02-08T08:55:35Z

That sounds good to me. So to recap, the proposal here is that we change all initEvent() methods, including initCustomEvent() et al, to have their arguments, except for the first, be optional and default to the same value as the corresponding event dictionary defaults them to.

This will require changes in DOM, HTML, UI Events, and maybe more?

annevk · 2017-02-08T08:56:51Z

I do think it would be good if we coordinated the changes to all these standards, to keep it all consistent and make implementation easier.

foolip · 2017-02-08T22:09:58Z

I would like to see the change across the board as well. In Blink, these are the init*Event() methods:

initCompositionEvent
initCustomEvent
initDeviceMotionEvent
initDeviceOrientationEvent
initEvent
initKeyboardEvent
initMessageEvent
initMouseEvent
initMutationEvent
initStorageEvent
initTextEvent
initUIEvent

https://software.hixie.ch/utilities/js/live-dom-viewer/saved/4873 is a quick test that confirms what our IDL files suggest, namely that initCustomEvent is the only one that requires any argument, namely all 4. Its 4th argument is of type any, so making that default to null wouldn't be an issue.

I checked all of the rest, and much to my surprise and delight didn't find any cases where an event interface member would have to be made nullable to accommodate this change. (I found many things like this when looking at event init dicts with no required members.)

foolip · 2017-02-08T22:12:33Z

To clarify, I don't think the change to DOM needs to block on every other PR being ready. In particular, https://w3c.github.io/uievents/ doesn't even have the arguments, and if there's any trouble in fixing that I think we should still fix everything in HTML and DOM.

cdumez · 2017-02-24T18:08:08Z

I am going to try and find time this week end to write the DOM / HTML spec changes, as well as updating the web-platform-tests accordingly.

…t the first Make all parameters to initEvent() / initCustomEvent() optional except the first. This aligns the specification with WebKit and gets it closer to Blink. This closes issue whatwg#387.

Make all parameters to initMessageEvent() optional except the first one. See corresponding DOM change at: whatwg/dom#417 and discussion at: whatwg/dom#387

…t the first one Make all parameters to initEvent() / initCustomEvent() optional except the first one. to match: - whatwg/dom#417 - whatwg/dom#387

Make all parameters to initMessageEvent() optional except the first one. See corresponding HTML specification change at: whatwg/html#2410 and discussion at: whatwg/dom#387

cdumez · 2017-03-05T00:08:11Z

Uploaded PRs for DOM, HTML and WPT.

cdumez · 2017-03-05T00:08:50Z

Will fully align WebKit via https://bugs.webkit.org/show_bug.cgi?id=169176.

Make all parameters to initMessageEvent() optional except the first one. See corresponding DOM change at: whatwg/dom#417 and discussion at: whatwg/dom#387

annevk · 2017-03-06T17:29:06Z

@cdumez can you file bugs against Edge and Firefox too? And does Chrome need a bug? I'll land this tomorrow. I'm satisfied with the PR and tests PR.

annevk · 2017-03-06T17:29:20Z

(And thanks for making those!)

…t the first one Make all parameters to initEvent() / initCustomEvent() optional except the first one. to match: - whatwg/dom#417 - whatwg/dom#387

…e first See whatwg/dom#387 and whatwg/dom#417 for corresponding DOM Standard changes.

annevk · 2017-03-07T08:36:55Z

I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1345044 and https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/11187286/.

@foolip can you file the Chrome issue? I wasn't entirely sure it was needed since it seems that Chrome doesn't match either the standard or this revision so likely that's already tracked?

See corresponding HTML Standard change at whatwg/html#2410 and discussion at whatwg/dom#387.

Tests: web-platform-tests/wpt#5044. See corresponding DOM Standard change at whatwg/dom#417 and discussion at whatwg/dom#387.

foolip · 2017-03-07T09:42:18Z

Yes, this is tracked by https://bugs.chromium.org/p/chromium/issues/detail?id=673698 in Chromium, and the needed change is to make the first argument required.

cdumez · 2017-03-07T17:17:35Z

Thank you for merging and filing the bugs. WebKit is now aligned with the spec:
https://trac.webkit.org/r213517

whatwg/dom#387 BUG=673698 Review-Url: https://codereview.chromium.org/2579993002 Cr-Commit-Position: refs/heads/master@{#458108}

…d:60001 of https://codereview.chromium.org/2579993002/ ) Reason for revert: It looks this broke the external/wpt/dom/interfaces.html test: https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Mac10.12/builds/448 https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Mac10.12/builds/438 ... Original issue's description: > Make initEvent's first argument non-optional > > whatwg/dom#387 > > BUG=673698 > > Review-Url: https://codereview.chromium.org/2579993002 > Cr-Commit-Position: refs/heads/master@{#458108} > Committed: https://chromium.googlesource.com/chromium/src/+/2948f11e04fba5f0dc7244229c542152c97c1624 TBR=foolip@chromium.org,lunalu@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=673698 Review-Url: https://codereview.chromium.org/2760173002 Cr-Commit-Position: refs/heads/master@{#458227}

whatwg/dom#387 BUG=673698 Review-Url: https://codereview.chromium.org/2579993002 Cr-Original-Commit-Position: refs/heads/master@{#458108} Committed: https://chromium.googlesource.com/chromium/src/+/2948f11e04fba5f0dc7244229c542152c97c1624 Review-Url: https://codereview.chromium.org/2579993002 Cr-Commit-Position: refs/heads/master@{#458443}

Tests: web-platform-tests/wpt#5044. See corresponding DOM Standard change at whatwg/dom#417 and discussion at whatwg/dom#387.

https://bugs.webkit.org/show_bug.cgi?id=166284 Reviewed by Dan Bernstein. LayoutTests/imported/w3c: * web-platform-tests/dom/interfaces-expected.txt: We regress conformance, but a change has been proposed to the spec. Hopefully this will be a temporary regression. Source/WebCore: We added this quirk in r207908 and expanded it in r209946 and r210034. Many web compatibility issues have been found, which leads us to believe that many more exist. This reverts the behavior to match how it was before r203848. This change was proposed to the spec in whatwg/dom#387 * dom/Event.cpp: (WebCore::Event::initEventForBindings): Deleted. * dom/Event.h: * dom/Event.idl: * platform/RuntimeApplicationChecks.h: * platform/RuntimeApplicationChecks.mm: (WebCore::IOSApplication::isBaiduNuomi): Deleted. (WebCore::IOSApplication::isAutoNaviAMap): Deleted. (WebCore::IOSApplication::isFlipboard): Deleted. Canonical link: https://commits.webkit.org/183664@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@210045 268f45cc-cd09-0410-ab3c-d52691b4dbfc

This was referenced Feb 23, 2017

Clearly specify initUIEvent w3c/uievents#133

Open

Specify the arguments of initCompositionEvent() w3c/uievents#134

Open

Specify the arguments of initMouseEvent() w3c/uievents#136

Open

cdumez mentioned this issue Mar 4, 2017

Make all parameters to initEvent() / initCustomEvent() optional excep… #417

Merged

cdumez mentioned this issue Mar 4, 2017

Make all parameters to initMessageEvent() optional except the first one whatwg/html#2410

Merged

cdumez mentioned this issue Mar 4, 2017

Make all parameters to initEvent() / initCustomEvent() optional excep… web-platform-tests/wpt#5043

Merged

cdumez mentioned this issue Mar 5, 2017

Make all parameters to initMessageEvent() optional except the first one web-platform-tests/wpt#5044

Merged

annevk pushed a commit to web-platform-tests/wpt that referenced this issue Mar 7, 2017

Make parameters to initEvent() / initCustomEvent() optional except th…

a863028

…e first See whatwg/dom#387 and whatwg/dom#417 for corresponding DOM Standard changes.

annevk closed this as completed in 2358735 Mar 7, 2017

annevk pushed a commit to web-platform-tests/wpt that referenced this issue Mar 7, 2017

Make parameters to initMessageEvent() optional except the first

a0e9fcb

See corresponding HTML Standard change at whatwg/html#2410 and discussion at whatwg/dom#387.

annevk pushed a commit to whatwg/html that referenced this issue Mar 7, 2017

Make parameters to initMessageEvent() optional except the first

2dafd2f

Tests: web-platform-tests/wpt#5044. See corresponding DOM Standard change at whatwg/dom#417 and discussion at whatwg/dom#387.

foolip mentioned this issue Mar 7, 2017

Reach consensus on deprecation and removal of de facto standard init*Event() methods w3c/deviceorientation#35

Closed

MXEBot pushed a commit to mirror/chromium that referenced this issue Mar 21, 2017

Make initEvent's first argument non-optional

2948f11

whatwg/dom#387 BUG=673698 Review-Url: https://codereview.chromium.org/2579993002 Cr-Commit-Position: refs/heads/master@{#458108}

alice pushed a commit to alice/html that referenced this issue Jan 8, 2019

Make parameters to initMessageEvent() optional except the first

d83475c

Tests: web-platform-tests/wpt#5044. See corresponding DOM Standard change at whatwg/dom#417 and discussion at whatwg/dom#387.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

initEvent should not require three parameters #387

initEvent should not require three parameters #387

achristensen07 commented Dec 20, 2016

achristensen07 commented Dec 20, 2016

annevk commented Dec 21, 2016

achristensen07 commented Dec 21, 2016

annevk commented Dec 22, 2016

achristensen07 commented Dec 23, 2016

cdumez commented Jan 3, 2017 •

edited

Loading

annevk commented Jan 3, 2017

smaug---- commented Jan 3, 2017

cdumez commented Jan 3, 2017

cdumez commented Jan 3, 2017

smaug---- commented Jan 3, 2017

cdumez commented Jan 3, 2017

cdumez commented Jan 6, 2017

foolip commented Jan 6, 2017

annevk commented Jan 6, 2017

cdumez commented Feb 8, 2017

annevk commented Feb 8, 2017

annevk commented Feb 8, 2017

foolip commented Feb 8, 2017

foolip commented Feb 8, 2017

cdumez commented Feb 24, 2017

cdumez commented Mar 5, 2017

cdumez commented Mar 5, 2017

annevk commented Mar 6, 2017

annevk commented Mar 6, 2017

annevk commented Mar 7, 2017

foolip commented Mar 7, 2017

cdumez commented Mar 7, 2017

initEvent should not require three parameters #387

initEvent should not require three parameters #387

Comments

achristensen07 commented Dec 20, 2016

achristensen07 commented Dec 20, 2016

annevk commented Dec 21, 2016

achristensen07 commented Dec 21, 2016

annevk commented Dec 22, 2016

achristensen07 commented Dec 23, 2016

cdumez commented Jan 3, 2017 • edited Loading

annevk commented Jan 3, 2017

smaug---- commented Jan 3, 2017

cdumez commented Jan 3, 2017

cdumez commented Jan 3, 2017

smaug---- commented Jan 3, 2017

cdumez commented Jan 3, 2017

cdumez commented Jan 6, 2017

foolip commented Jan 6, 2017

annevk commented Jan 6, 2017

cdumez commented Feb 8, 2017

annevk commented Feb 8, 2017

annevk commented Feb 8, 2017

foolip commented Feb 8, 2017

foolip commented Feb 8, 2017

cdumez commented Feb 24, 2017

cdumez commented Mar 5, 2017

cdumez commented Mar 5, 2017

annevk commented Mar 6, 2017

annevk commented Mar 6, 2017

annevk commented Mar 7, 2017

foolip commented Mar 7, 2017

cdumez commented Mar 7, 2017

cdumez commented Jan 3, 2017 •

edited

Loading