Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Define Event.srcElement and Event.returnValue #626

Merged
merged 1 commit into from Apr 9, 2018

Conversation

3 participants
@miketaylr
Copy link
Contributor

commented Mar 30, 2018

@cvrebert

This comment has been minimized.

Copy link
Member

commented Apr 2, 2018

Fixes #625. (References in titles don't create cross-references in GitHub.)

dom.bs Outdated
@@ -446,6 +447,7 @@ interface Event {

readonly attribute boolean bubbles;
readonly attribute boolean cancelable;
attribute boolean returnValue;

This comment has been minimized.

Copy link
@cvrebert

cvrebert Apr 2, 2018

Member

Also historical? We have preventDefault() and defaultPrevented nowadays.

dom.bs Outdated
@@ -675,6 +680,12 @@ 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.

<p>The <dfn attribute for=Event><code>returnValue</code></dfn> 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>

This comment has been minimized.

Copy link
@cvrebert

cvrebert Apr 2, 2018

Member

When the attribute is set to

Should be phrased as a setter instead?: "The returnValue attribute’s setter must [...]"

See cancelBubble for an example of the getter/setter boilerplate verbiage.

This comment has been minimized.

Copy link
@annevk

annevk Apr 3, 2018

Member

Yeah, I think phrasing this as a getter and setter (one paragraph each) (and not saying anything about initial state) would be best. The initial state doesn't really exist, since it just reflects the canceled flag.

@miketaylr miketaylr changed the title Issue #625. Define Event.srcElement and Event.returnValue for compat. Fixes #625. Define Event.srcElement and Event.returnValue for compat. Apr 5, 2018

@miketaylr

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2018

Fixes #625. (References in titles don't create cross-references in GitHub.)

(once the commit lands it should, but thanks!)

@miketaylr miketaylr force-pushed the miketaylr:issues/625/1 branch from 7da8e8e to 7883839 Apr 5, 2018

@miketaylr

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2018

@miketaylr miketaylr force-pushed the miketaylr:issues/625/1 branch from 7883839 to ec01bff Apr 5, 2018

dom.bs Outdated
@@ -675,6 +680,12 @@ 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.

<p>The <dfn attribute for=Event><code>returnValue</code></dfn> attribute's getter must return false
if the <a>canceled flag</a> is set, and true otherwise.

This comment has been minimized.

Copy link
@cvrebert

cvrebert Apr 6, 2018

Member

if the <a>canceled flag</a> is set

if <a>context object</a>'s <a>canceled flag</a> is set

dom.bs Outdated
<p>The <dfn attribute for=Event><code>returnValue</code></dfn> attribute's getter must return false
if the <a>canceled flag</a> is set, and true otherwise.

<p>The {{Event/returnValue}} attribute's setter must, if the given value is false, set the <a>canceled flag</a>

This comment has been minimized.

Copy link
@cvrebert

cvrebert Apr 6, 2018

Member

set context object's canceled flag

dom.bs Outdated
if the <a>canceled flag</a> is set, and true otherwise.

<p>The {{Event/returnValue}} attribute's setter must, if the given value is false, set the <a>canceled flag</a>
if the {{Event/cancelable}} attribute value is true and the <a>in passive listener flag</a> is unset.

This comment has been minimized.

Copy link
@cvrebert

cvrebert Apr 6, 2018

Member

context object's in passive listener flag

This comment has been minimized.

Copy link
@annevk

annevk Apr 6, 2018

Member

Also "the context object's {{Event/cancelable}} ..."

This comment has been minimized.

Copy link
@annevk

annevk Apr 6, 2018

Member

Although maybe it would be nicer to refactor this and make it share an algorithm with preventDefault(). I can do that before landing I suppose.

dom.bs Outdated
if the <a>canceled flag</a> is set, and true otherwise.

<p>The {{Event/returnValue}} attribute's setter must, if the given value is false, set the <a>canceled flag</a>
if the {{Event/cancelable}} attribute value is true and the <a>in passive listener flag</a> is unset.

This comment has been minimized.

Copy link
@cvrebert

cvrebert Apr 6, 2018

Member

[...] and do nothing otherwise.

@miketaylr

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2018

Changes made, thanks @annevk @cvrebert.

@annevk annevk force-pushed the miketaylr:issues/625/1 branch from a71ae13 to f0254b6 Apr 9, 2018

Define Event's srcElement and returnValue
Unfortunately the web depends on these. They are intentionally
defined in such a way to not offer capabilities beyond the
non-historical API for events.

Tests: web-platform-tests/wpt#10258.

Fixes #625 and fixes #627.

@annevk annevk force-pushed the miketaylr:issues/625/1 branch from f0254b6 to f6eb0e0 Apr 9, 2018

@annevk annevk changed the title Fixes #625. Define Event.srcElement and Event.returnValue for compat. Define Event.srcElement and Event.returnValue Apr 9, 2018

@annevk

This comment has been minimized.

Copy link
Member

commented Apr 9, 2018

I made a couple tweaks so this also fixes #627.

Do we have all the browser bugs that we need here?

I found https://bugs.chromium.org/p/chromium/issues/detail?id=277851 against Chrome, but it's not quite scoped to this (I left a comment).

For Firefox there's https://bugzilla.mozilla.org/show_bug.cgi?id=453968 for srcElement.

@cvrebert

This comment has been minimized.

Copy link
Member

commented Apr 9, 2018

The other relevant Chrome bug is https://bugs.chromium.org/p/chromium/issues/detail?id=692695
Looks like we need a WebKit bug for changing its returnValue to be no more powerful than preventDefault() ?
I couldn't find a Mozilla bug for implementing returnValue.

@annevk

This comment has been minimized.

Copy link
Member

commented Apr 9, 2018

Yeah, Edge and WebKit.

@annevk

This comment has been minimized.

Copy link
Member

commented Apr 9, 2018

(Travis is complaining due to validator/validator#634 btw.)

@annevk

This comment has been minimized.

Copy link
Member

commented Apr 9, 2018

@annevk annevk merged commit e36b369 into whatwg:master Apr 9, 2018

2 checks passed

Participation miketaylr participates on behalf of Mozilla Corporation
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

hubot pushed a commit to WebKit/webkit that referenced this pull request Apr 16, 2018

cdumez@apple.com
Change Event's returnValue so it doesn't expose a new primitive
https://bugs.webkit.org/show_bug.cgi?id=184415

Reviewed by Darin Adler.

LayoutTests/imported/w3c:

Import test coverage from web-platform-tests/wpt#10258.

* web-platform-tests/dom/events/AddEventListenerOptions-passive-expected.txt:
* web-platform-tests/dom/events/AddEventListenerOptions-passive.html:
* web-platform-tests/dom/events/Event-constructors.html:
* web-platform-tests/dom/events/Event-defaultPrevented-after-dispatch-expected.txt:
* web-platform-tests/dom/events/Event-defaultPrevented-after-dispatch.html:
* web-platform-tests/dom/events/Event-defaultPrevented-expected.txt:
* web-platform-tests/dom/events/Event-defaultPrevented.html:
* web-platform-tests/dom/events/Event-dispatch-click.html:
* web-platform-tests/dom/events/Event-dispatch-detached-click.html:
* web-platform-tests/dom/events/Event-dispatch-other-document.html:
* web-platform-tests/dom/events/Event-initEvent.html:
* web-platform-tests/dom/events/Event-returnValue-expected.txt: Added.
* web-platform-tests/dom/events/Event-returnValue.html: Added.
* web-platform-tests/dom/events/EventListener-handleEvent.html:
* web-platform-tests/dom/events/EventTarget-dispatchEvent-returnvalue-expected.txt:
* web-platform-tests/dom/events/EventTarget-dispatchEvent-returnvalue.html:
* web-platform-tests/dom/events/w3c-import.log:
* web-platform-tests/dom/interfaces-expected.txt:
* web-platform-tests/interfaces/dom.idl:

Source/WebCore:

Update Event.returnValue setter to match the latest DOM specification after:
- whatwg/dom#626

In particular, the returnValue setter is now a no-op if the new flag value
is true. If the input flag value is false, it only sets the 'canceled' flag
if the event is cancelable and the event’s in passive listener flag is unset.

Test: imported/w3c/web-platform-tests/dom/events/Event-returnValue.html

* dom/Event.cpp:
(WebCore::Event::setLegacyReturnValue):
(WebCore::Event::setCanceledFlagIfPossible):
(WebCore::Event::preventDefault):
* dom/Event.h:


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@230664 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Zirro added a commit to Zirro/jsdom that referenced this pull request Apr 16, 2018

Zirro added a commit to Zirro/jsdom that referenced this pull request Apr 23, 2018

Zirro added a commit to Zirro/jsdom that referenced this pull request Apr 25, 2018

domenic added a commit to jsdom/jsdom that referenced this pull request Apr 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.