Add window.event as a legacy compat requirement #407

miketaylr · 2017-02-09T04:25:41Z

@annevk is this sufficient? Or should I add srcElement everywhere that target is mentioned (feels a bit redundant after a while...)

I'll have a related wpt PR tomorrow at some point.

Preview | Diff

annevk

This looks good. I think I'd prefer adding all the Microsoft extensions at the same time, so we have only one bug report per browser (if needed). Landing them quickly one after another seems okay though.

annevk · 2017-02-09T10:24:54Z

dom.bs

@@ -570,6 +571,9 @@ algorithm below.
 <dt><code><var>event</var> . {{Event/target}}</code>
 <dd>Returns the object to which <var>event</var> is <a>dispatched</a>.

+ <dt><code><var>event</var> . {{Event/srcElement}}</code>
+ <dd>Returns <var>event</var>'s {{Event/target}} attribute value.


No need to add it here since developers can just use target.

annevk · 2017-02-09T10:25:18Z

dom.bs

@@ -642,6 +646,9 @@ attributes must return the values they were initialized to. When an
 <a>event</a> is created the attributes must be
 initialized to null.

+The <dfn attribute for=Event><code>srcElement</code></dfn>
+attribute must return the {{Event/target}} attribute value.


I'd prefer to define it together with target. So "The target and srcElement attributes must return X".

miketaylr · 2017-02-09T16:42:57Z

I think I'd prefer adding all the Microsoft extensions at the same time, so we have only one bug report per browser (if needed).

That cool. I'll use this PR to add in the other bits and update the title accordingly.

miketaylr · 2017-02-25T00:16:14Z

@annevk I guess HTML is already overloading returnValue on BeforeUnloadEvent, right? Do we need to do anything more to call that out?

https://html.spec.whatwg.org/multipage/browsers.html#the-beforeunloadevent-interface

annevk · 2017-02-25T15:11:14Z

I don't think so. @domenic would know.

The current text looks good to me, but it's also starting to show that we don't use internal slots to back the event attributes. As now we have to initialize two attributes rather than a single slot that backs both of them.

You're still planning on adding window.event, right?

domenic · 2017-02-25T17:13:24Z

No need to do anything else, yeah.

hmntptwl · 2017-02-25T19:42:06Z

Thats a perfect sort of thing

miketaylr · 2017-02-27T20:15:52Z

You're still planning on adding window.event, right?

Yeah, just doing commits + tests piecemeal. That's next.

domenic · 2017-03-09T21:30:39Z

dom.bs

+attribute must be initialized to true when an <a>event</a> is created.
+When the attribute is set to false it must set the <a>canceled flag</a> if the
+{{Event/cancelable}} attribute value is true and the
+<a>in passive listener flag</a> is unset.


Do implementations update returnValue if the event gets canceled by e.g. preventDefault()?

(this notification got swallowed by my inbox, will look into this tomorrow)

@domenic yeah, Safari, Chrome and Edge do:

https://miketaylr.com/bzla/canceled-returnValue.html

So we need to add a getter that returns if the canceled flag is set, and false otherwise.

miketaylr · 2017-10-26T21:31:36Z

@annevk @hayatoito could you check the window.event bits? e7cdac7

Thanks.

miketaylr · 2017-11-02T16:49:30Z

@hayatoito @annevk how does 449ddb7 look? Is it OK to leave inner invoke algorithm alone and define window.event like this?

annevk · 2017-11-03T10:18:40Z

dom.bs

+is the <var>event</var> passed to <a>inner invoke</a>. If <var>event</var>'s {{Event/target}} <a for=tree>root</a>
+is a <a for=/>shadow root</a>, the <a>current event</a> is undefined.
+
+<p class="note no-backref">Authors <span class=allow-2119>should</span> explicitly pass in the


Make this "Developers are strongly encouraged" instead of using "authors" and "should".

annevk · 2017-11-03T10:20:20Z

dom.bs

+<dfn attribute for=Window><code>event</code></dfn> attribute that always returns the <a>current event</a>
+during <a>dispatch</a>, and is otherwise undefined. The <dfn type=dfn id=concept-current-event>current event</dfn>
+is the <var>event</var> passed to <a>inner invoke</a>. If <var>event</var>'s {{Event/target}} <a for=tree>root</a>
+is a <a for=/>shadow root</a>, the <a>current event</a> is undefined.


This will need to become an imperative definition where we manipulate the internal slot that the attribute reflects from the dispatch algorithm. Declarative definitions often lead to subtle edge cases becoming undefined over time.

annevk · 2017-11-03T10:20:42Z

dom.bs

@@ -587,7 +603,8 @@ return the value it was initialized to. When an
 <a>event</a> is created the attribute must be
 initialized to the empty string.

-The <dfn attribute for=Event><code>target</code></dfn> and
+The <dfn attribute for=Event><code>target</code></dfn>,
+<dfn attribute for=Event><code>srcElement</code></dfn> and


Oxford comma.

annevk · 2017-11-03T10:21:46Z

dom.bs

@@ -731,7 +755,7 @@ To <dfn export for=Event id=concept-event-initialize>initialize</dfn> an
 <a>canceled flag</a>.
 <li>Set the {{Event/isTrusted}} attribute
 to false.
- <li>Set the {{Event/target}} attribute to
+ <li>Set the {{Event/target}} and {{Event/srcElement}} attributes to


An alternative might be that we actually define Event's "target" as a concept and set that here and than have the target and srcElement attributes reflect that.

Filed a follow up issue for that: #570

miketaylr · 2018-02-14T17:29:28Z

Participation — @miketaylr has not signed up to participate

done.

"file:/home/travis/build/whatwg/dom/dom.spec.whatwg.org/index.html":501.381-501.480: error: Attribute “type” not allowed on element “dfn” at this point.

oops.

domenic · 2018-02-14T18:39:36Z

done.

I noticed you signed up as an individual. Do you no longer work for Mozilla (or another company in the field of web technologies)? If you still work for Mozilla, then you'll want to get in touch with @dbaron and get invited to https://github.com/mozilla-standards instead.

dbaron · 2018-02-14T19:49:38Z

@miketaylr is now part of @mozilla-standards

domenic · 2018-02-14T19:52:28Z

Awesome. He'll just need to make his affiliation public, then the checker will be satisfied.

(All that said, I imagine this PR will need to wait until @annevk is back from vacation.)

miketaylr · 2018-02-14T20:04:01Z

Awesome. He'll just need to make his affiliation public, then the checker will be satisfied.

Done, thanks.

(All that said, I imagine this PR will need to wait until @annevk is back from vacation.)

Yeah, no worries. There still a few things to address -- I'll manually ping for review when ready.

miketaylr · 2018-02-15T05:45:05Z

OK, window.event WPTs added to web-platform-tests/wpt#4790.

~~r? @annevk (when back from vacation)~~

edit: working on some feedback from tests, hang tight.

annevk

I decided to review anyway to allow for parallel addressing of comments. Hope that helps.

annevk · 2018-02-19T09:48:40Z

dom.bs

@@ -499,6 +505,15 @@ list of tuples, each of which consists of an <b>item</b> (an {{EventTarget}} obj
 object). A tuple is formatted as (<b>item</b>, <b>target</b>, <b>relatedTarget</b>). A <a
 for=Event>path</a> is initially the empty list.</p>

+For web compatibility, the <a>current global object</a> has an


All new/changed paragraphs should start with <p>.

This paragraph should be reworded and moved done I think. It makes more sense to me to define this extension to the Window object at the end of this section since it's also at the end of the IDL.

Furthermore, we shouldn't give the reason in the description of the feature. That can go into a note if it's important.

annevk · 2018-02-19T09:48:53Z

dom.bs

+
+<p class="note no-backref">Authors <span class=allow-2119>should</span> explicitly pass in the
+event object to event listener callbacks, rather than rely on a global <code>window.event</code>
+object.


I wouldn't use should here. Just say "strongly encouraged".

annevk · 2018-02-19T09:50:52Z

dom.bs

@@ -684,6 +700,13 @@ The <dfn attribute for=Event><code>bubbles</code></dfn> and
 <dfn attribute for=Event><code>cancelable</code></dfn> attributes
 must return the values they were initialized to.

+The <dfn attribute for=Event><code>returnValue</code></dfn>
+attribute must be initialized to true when an <a>event</a> is created.


I don't think this makes sense. You want to instead describe this as a getter and a setter. The default value is already evident from the default value of the canceled flag after all.

See https://url.spec.whatwg.org/#dom-url-protocol for an example of defining a getter and a setter.

annevk · 2018-02-19T09:51:30Z

dom.bs

@@ -1357,6 +1380,9 @@ with <var>event</var>, <var>listeners</var>, and an optional
   <li><p>If <var>listener</var>'s <b>passive</b> is true, then set <var>event</var>'s
   <a>in passive listener flag</a>.

+   <li><p>If <var>event</var>'s {{Event/target}} <a for=tree>root</a> is not a
+   <a for=/>shadow root</a>, set the <a>current event</a> to <var>event</var>.


annevk · 2018-02-19T09:53:01Z

dom.bs

+For web compatibility, the <a>current global object</a> has an
+<dfn attribute for=Window><code>event</code></dfn> attribute that returns the
+<dfn export id=concept-current-event>current event</dfn>, which is initially
+set to undefined.


You also need to define what current event is associated with. Also, we should probably test that this quirk is not present in workers? And ensure the algorithms work in a worker context as well, since events don't just dispatch inside a window.

hsivonen · 2018-03-27T09:30:30Z

Is this PR in a state that I should try to write Gecko code for?

annevk · 2018-03-27T09:39:51Z

@hsivonen I had another look and while I thought I only had nits, I think one problem with window.event is that "If event's target's root is not a shadow root" is a runtime check, while this should be based on information known prior to dispatch. And perhaps we should also hide the event if any of its relatedTarget or touch target list contain shadow tree entries, but only while calling listeners in the shadow tree. (See also #585.)

returnValue should be safe to implement though and srcElement is already in Nightly.

smaug---- · 2018-03-28T12:26:02Z

while this should be based on information known prior to dispatch.

Yeah, this definitely. Should also make the implementation faster, at least in Gecko.

But why is the check based on target and not currentTarget? I would have expected window.event to be null/undefined whenever an event listener in shadow DOM tries to access it. I think that makes the feature easier to understand - it works only in light DOM, and scripts in shadow DOM can't use it.

annevk · 2018-06-04T10:19:38Z

I've now updated this PR to use the suggestion from @yuki3 to take the global from the listener's callback. So now it's evaluated on a per-listener basis rather than on a per-event-target basis. Apologies for the delay. I'll next update the tests and file implementation bugs.

annevk · 2018-06-04T11:56:23Z

Bugs:

If someone could give this and the tests a final review pass that'd much appreciated!

miketaylr · 2018-06-06T18:31:53Z

If someone could give this and the tests a final review pass that'd much appreciated!

Spec updates LGTM.

For whatwg/dom#334 and whatwg/dom#407.

Automatic update from web-platform-testsAdd tests for window.event For whatwg/dom#334 and whatwg/dom#407. -- wpt-commits: 659fe6eb4dea2550f4fb5dc14c2916aa1eaef1cc wpt-pr: 4790

…shadow tree https://bugs.webkit.org/show_bug.cgi?id=186266 Reviewed by Ryosuke Niwa. LayoutTests/imported/w3c: Re-sync dom/events web-platform-tests from 59d4a411a8e to gain test coverage. * web-platform-tests/dom/events/Event-returnValue-expected.txt: * web-platform-tests/dom/events/Event-returnValue.html: * web-platform-tests/dom/events/event-global-expected.txt: Added. * web-platform-tests/dom/events/event-global-extra.window.js: Added. (async_test.t.frame.onload.t.step_func_done): * web-platform-tests/dom/events/event-global.html: Added. * web-platform-tests/dom/events/event-global.worker-expected.txt: Added. * web-platform-tests/dom/events/event-global.worker.html: Added. * web-platform-tests/dom/events/event-global.worker.js: Added. * web-platform-tests/dom/events/relatedTarget.window.js: Added. (test): (test.t.input.oninput.t.step_func): * web-platform-tests/dom/events/resources/event-global-extra-frame.html: Added. * web-platform-tests/dom/events/resources/w3c-import.log: Added. * web-platform-tests/dom/events/w3c-import.log: Source/WebCore: Stop exposing window.event to Shadow DOM by not setting window.event if the event's target is a Node inside a shadow tree. This is as per the latest DOM specification: - whatwg/dom#407 This aligns our behavior with Blink as well: - https://bugs.chromium.org/p/chromium/issues/detail?id=779461 Tests: imported/w3c/web-platform-tests/dom/events/event-global.html imported/w3c/web-platform-tests/dom/events/event-global.worker.html * bindings/js/JSEventListener.cpp: (WebCore::JSEventListener::handleEvent): git-svn-id: http://svn.webkit.org/repository/webkit/trunk@233489 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Automatic update from web-platform-testsTest self.event in workers To compliment #10329. Needed for whatwg/dom#407. -- wpt-commits: 3543c82551c1c1c152885844b00b0af0d0e38864 wpt-pr: 10382 wpt-commits: 3543c82551c1c1c152885844b00b0af0d0e38864 wpt-pr: 10382 UltraBlame original commit: 3838ac80516e937da4487e484094587ad3483348

Automatic update from web-platform-testsAdd tests for window.event For whatwg/dom#334 and whatwg/dom#407. -- wpt-commits: 659fe6eb4dea2550f4fb5dc14c2916aa1eaef1cc wpt-pr: 4790 UltraBlame original commit: 2f6df5e484b27039d7c56fe74e8e9ad7eee437a8

Automatic update from web-platform-testsAdd tests for window.event For whatwg/dom#334 and whatwg/dom#407. -- wpt-commits: 659fe6eb4dea2550f4fb5dc14c2916aa1eaef1cc wpt-pr: 4790 UltraBlame original commit: f27248fd9a0c66d1e8b87085365f4de286491691

Automatic update from web-platform-testsAdd tests for window.event For whatwg/dom#334 and whatwg/dom#407. -- wpt-commits: 659fe6eb4dea2550f4fb5dc14c2916aa1eaef1cc wpt-pr: 4790 UltraBlame original commit: 2f6df5e484b27039d7c56fe74e8e9ad7eee437a8

Automatic update from web-platform-testsAdd tests for window.event For whatwg/dom#334 and whatwg/dom#407. -- wpt-commits: 659fe6eb4dea2550f4fb5dc14c2916aa1eaef1cc wpt-pr: 4790 UltraBlame original commit: f27248fd9a0c66d1e8b87085365f4de286491691

Automatic update from web-platform-testsAdd tests for window.event For whatwg/dom#334 and whatwg/dom#407. -- wpt-commits: 659fe6eb4dea2550f4fb5dc14c2916aa1eaef1cc wpt-pr: 4790 UltraBlame original commit: 2f6df5e484b27039d7c56fe74e8e9ad7eee437a8

Automatic update from web-platform-testsAdd tests for window.event For whatwg/dom#334 and whatwg/dom#407. -- wpt-commits: 659fe6eb4dea2550f4fb5dc14c2916aa1eaef1cc wpt-pr: 4790 UltraBlame original commit: f27248fd9a0c66d1e8b87085365f4de286491691

srcElement and returnValue were specified in [1] and have WPTs in [2]. cancelBubble is documented in [3] and has a WPT which passes in all browsers in [4]. [1] whatwg/dom#407 [2] web-platform-tests/wpt#10258 [3] https://developer.mozilla.org/en-US/docs/Web/API/Event/cancelBubble [4] https://wpt.fyi/results/dom/events/Event-cancelBubble.html Fixed: 692695 Change-Id: Ia7113a9996a0e272829a3e81bf751412444e9ba8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2610947 Reviewed-by: Mason Freed <masonfreed@chromium.org> Commit-Queue: Joey Arhar <jarhar@chromium.org> Cr-Commit-Position: refs/heads/master@{#840397}

srcElement and returnValue were specified in [1] and have WPTs in [2]. cancelBubble is documented in [3] and has a WPT which passes in all browsers in [4]. [1] whatwg/dom#407 [2] web-platform-tests/wpt#10258 [3] https://developer.mozilla.org/en-US/docs/Web/API/Event/cancelBubble [4] https://wpt.fyi/results/dom/events/Event-cancelBubble.html Fixed: 692695 Change-Id: Ia7113a9996a0e272829a3e81bf751412444e9ba8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2610947 Reviewed-by: Mason Freed <masonfreed@chromium.org> Commit-Queue: Joey Arhar <jarhar@chromium.org> Cr-Commit-Position: refs/heads/master@{#840397} GitOrigin-RevId: 3de46b6619a24531b2277b243ac39484a8ca5505

From what I can tell, the warning on MDN is a bit misleading, since this MSIE extension is still technically part of the standard*, although its use is discouraged. This API will likely never go away. See this issue** on MDN content. Note that this uses a different approach for getting the inner window. * https://dom.spec.whatwg.org/#interface-window-extensions ** mdn/content#21848 Spec PR: whatwg/dom#407 Spec discussion: whatwg/dom#334 Partially based on https://bugzilla.mozilla.org/show_bug.cgi?id=218415

From what I can tell, the warning on MDN is a bit misleading, since this MSIE extension is still technically part of the standard*, although its use is discouraged. This API will also likely never go away. See this issue** on MDN content. Note that this uses a different approach for getting the inner window. * https://dom.spec.whatwg.org/#interface-window-extensions ** mdn/content#21848 Spec PR: whatwg/dom#407 Spec discussion: whatwg/dom#334 Partially based on https://bugzilla.mozilla.org/show_bug.cgi?id=218415

This MSIE extension is still technically part of the standard*, although its use is discouraged. This API will also likely never go away based on some comments at this issue on MDN content**. Note that this uses a different approach for getting the inner window. * https://dom.spec.whatwg.org/#interface-window-extensions ** mdn/content#21848 Spec PR: whatwg/dom#407 Spec discussion: whatwg/dom#334 Partially based on https://bugzilla.mozilla.org/show_bug.cgi?id=218415

annevk reviewed Feb 9, 2017

View reviewed changes

miketaylr changed the title ~~Add event.srcElement as a legacy alias to event.target~~ Add window.event, event.srcElement, event.returnValue as a legacy compat requirements Feb 9, 2017

miketaylr force-pushed the issues/334/1 branch from 48b006f to 522acb5 Compare February 10, 2017 00:27

miketaylr mentioned this pull request Feb 10, 2017

Add tests for window.event web-platform-tests/wpt#4790

Merged

domenic reviewed Mar 9, 2017

View reviewed changes

miketaylr force-pushed the issues/334/1 branch from 0c8d9b9 to e7cdac7 Compare October 26, 2017 21:30

miketaylr force-pushed the issues/334/1 branch from e7cdac7 to 449ddb7 Compare November 2, 2017 16:47

miketaylr force-pushed the issues/334/1 branch from 449ddb7 to 5cc3243 Compare November 2, 2017 18:33

annevk reviewed Nov 3, 2017

View reviewed changes

miketaylr force-pushed the issues/334/1 branch from 5cc3243 to 393f447 Compare February 14, 2018 17:08

miketaylr mentioned this pull request Feb 14, 2018

Define a target concept that Event.target and Event.srcElement reflect #570

Closed

miketaylr force-pushed the issues/334/1 branch from 393f447 to 979c442 Compare February 14, 2018 21:08

annevk reviewed Feb 19, 2018

View reviewed changes

derive global from the listener's callback

acc8ede

annevk force-pushed the issues/334/1 branch from 01fc776 to acc8ede Compare June 4, 2018 10:18

annevk removed the do not merge yet Pull request must not be merged per rationale in comment label Jun 4, 2018

annevk pushed a commit to web-platform-tests/wpt that referenced this pull request Jun 7, 2018

Add tests for window.event

659fe6e

For whatwg/dom#334 and whatwg/dom#407.

annevk merged commit c69d7c0 into whatwg:master Jun 7, 2018

dhdavvie pushed a commit to dhdavvie/wpt that referenced this pull request Jun 7, 2018

Add tests for window.event

35c7f01

For whatwg/dom#334 and whatwg/dom#407.

annevk mentioned this pull request Apr 25, 2023

Does the currentEvent variable make a sense in Inner Invoke? #1191

Closed

Add window.event as a legacy compat requirement #407

Add window.event as a legacy compat requirement #407

Conversation

miketaylr commented Feb 9, 2017 • edited by pr-preview bot Loading

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miketaylr commented Feb 9, 2017

miketaylr commented Feb 25, 2017

annevk commented Feb 25, 2017

domenic commented Feb 25, 2017

hmntptwl commented Feb 25, 2017

miketaylr commented Feb 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miketaylr commented Oct 26, 2017

miketaylr commented Nov 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miketaylr commented Feb 14, 2018

domenic commented Feb 14, 2018

dbaron commented Feb 14, 2018

domenic commented Feb 14, 2018

miketaylr commented Feb 14, 2018

miketaylr commented Feb 15, 2018 • edited Loading

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hsivonen commented Mar 27, 2018

annevk commented Mar 27, 2018

smaug---- commented Mar 28, 2018

annevk commented Jun 4, 2018

annevk commented Jun 4, 2018

miketaylr commented Jun 6, 2018

miketaylr commented Feb 9, 2017 •

edited by pr-preview bot

Loading

miketaylr commented Feb 15, 2018 •

edited

Loading