Closed Bug 1452569 Opened 6 years ago Closed 6 years ago

Implement Event's returnValue

Categories

(Core :: DOM: Core & HTML, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: annevk, Assigned: alchen)

References

Details

(Keywords: dev-doc-complete, good-first-bug, site-compat, Whiteboard: [webcompat:p3])

Attachments

(1 file, 1 obsolete file)

Since we're adding Event's srcElement and Window's event, we should also add Event's returnValue, which is the last of (formerly) proprietary event extensions.

Standard: https://github.com/whatwg/dom/pull/626.

Note that the standard has made Event's returnValue less powerful than it currently is in Chrome, Edge, and Safari so that it's not a more powerful API than defaultPrevented and preventDefault(). It's hoped to be web compatible, but not yet field tested.
Blocks: 473360
Blocks: 271528
Blocks: 1076790
Flags: webcompat+
Whiteboard: [webcompat:p3]
MDN doesn't yet acknowledge the spec addition or this bug, and so will need updating:
https://developer.mozilla.org/en-US/docs/Web/API/Event/returnValue
(In reply to Chris Rebert from comment #1)
> MDN doesn't yet acknowledge the spec addition or this bug, and so will need
> updating:
> https://developer.mozilla.org/en-US/docs/Web/API/Event/returnValue

You can use the dev-doc-needed keyword for that, thanks!
Keywords: dev-doc-needed
Quick implementation notes:
 * This should be easy to implement by using the pre-existing implementations for preventDefault() and defaultPrevented.
 * _canceled_ seems to be called mDefaultPrevented: https://searchfox.org/mozilla-central/source/widget/BasicEvents.h#83
 * The main oddity seems to be that our preventDefault() tracks whether it was called from chrome or content. returnValue should probably be [NeedsCallerType], too, in WebIDL.
FWIW, the reason for defaultPrevented checking caller type is that web pages shouldn't see if only browser chrome has called preventDefaul()
Yeah, but the question is, do we trust chrome code never to set returnValue if we add returnValue? Seems safe to assume that someone might set it from chrome code if we add it.
Add retrunValue into Event's interface
Comment on attachment 9003481 [details]
Bug 1452569 - Implement Event's returnValue. r=smaug

Hi Oli,

Could you take a look?
Attachment #9003481 - Flags: feedback?(bugs)
Sure, could you just put it to my review queue and I'm more likely to see the patch :)

Don't you need to update some .ini files, like https://searchfox.org/mozilla-central/source/testing/web-platform/meta/dom/events/Event-returnValue.html.ini, if the tests are now passing?
Attachment #9003481 - Flags: feedback?(bugs) → review?(bugs)
(In reply to Olli Pettay [:smaug] from comment #9)
> Sure, could you just put it to my review queue and I'm more likely to see
> the patch :)
> 
> Don't you need to update some .ini files, like
> https://searchfox.org/mozilla-central/source/testing/web-platform/meta/dom/
> events/Event-returnValue.html.ini, if the tests are now passing?

Thanks. I've revised the patch and request a review.
Comment on attachment 9003481 [details]
Bug 1452569 - Implement Event's returnValue. r=smaug

Olli Pettay [:smaug] has approved the revision.
Attachment #9003481 - Flags: review+
Attachment #9003481 - Flags: review?(bugs)
Keywords: checkin-needed
Attachment #9003481 - Attachment description: Bug 1452569 - Implement Event's returnValue → Bug 1452569 - Implement Event's returnValue. r=smaug
Pushed by ebalazs@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8427667b7c9a
Implement Event's returnValue. r=smaug
Keywords: checkin-needed
Backed out for wpt failures on /dom/interfaces.html

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-searchStr=Linux%20x64%20opt%20Web%20platform%20tests%20with%20e10s%20test-linux64%2Fopt-web-platform-tests-e10s-2%20W-e10s(wpt2)&fromchange=8427667b7c9aef4aa8ece1f27254ac9332c6a62c&tochange=315e96b7fc527ecf3b85c8a634663bf6bb19fd00&filter-resultStatus=testfailed&filter-resultStatus=success&filter-resultStatus=runnable&selectedJob=196043310

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=196043310&repo=autoland&lineNumber=2485

Backout link: https://hg.mozilla.org/integration/autoland/rev/315e96b7fc527ecf3b85c8a634663bf6bb19fd00

[task 2018-08-27T08:49:30.559Z] 08:49:30     INFO - TEST-START | /dom/interfaces.html?exclude=Node
[task 2018-08-27T08:49:31.485Z] 08:49:31     INFO - 
[task 2018-08-27T08:49:31.486Z] 08:49:31     INFO - TEST-PASS | /dom/interfaces.html?exclude=Node | Event interface: constant AT_TARGET on interface object 
[task 2018-08-27T08:49:31.486Z] 08:49:31     INFO - TEST-PASS | /dom/interfaces.html?exclude=Node | Event interface: constant AT_TARGET on interface prototype object 
[task 2018-08-27T08:49:31.486Z] 08:49:31     INFO - TEST-PASS | /dom/interfaces.html?exclude=Node | Event interface: constant BUBBLING_PHASE on interface object 
[task 2018-08-27T08:49:31.487Z] 08:49:31     INFO - TEST-PASS | /dom/interfaces.html?exclude=Node | Event interface: constant BUBBLING_PHASE on interface prototype object 
[task 2018-08-27T08:49:31.487Z] 08:49:31     INFO - TEST-PASS | /dom/interfaces.html?exclude=Node | Event interface: attribute eventPhase 
[task 2018-08-27T08:49:31.487Z] 08:49:31     INFO - TEST-PASS | /dom/interfaces.html?exclude=Node | Event interface: operation stopPropagation() 
[task 2018-08-27T08:49:31.487Z] 08:49:31     INFO - TEST-PASS | /dom/interfaces.html?exclude=Node | Event interface: attribute cancelBubble 
[task 2018-08-27T08:49:31.488Z] 08:49:31     INFO - TEST-PASS | /dom/interfaces.html?exclude=Node | Event interface: operation stopImmediatePropagation() 
[task 2018-08-27T08:49:31.488Z] 08:49:31     INFO - TEST-PASS | /dom/interfaces.html?exclude=Node | Event interface: attribute bubbles 
[task 2018-08-27T08:49:31.488Z] 08:49:31     INFO - TEST-PASS | /dom/interfaces.html?exclude=Node | Event interface: attribute cancelable 
[task 2018-08-27T08:49:31.489Z] 08:49:31     INFO - TEST-UNEXPECTED-PASS | /dom/interfaces.html?exclude=Node | Event interface: attribute returnValue - expected FAIL
[task 2018-08-27T08:49:31.489Z] 08:49:31     INFO - TEST-INFO | expected FAIL
[task 2018-08-27T08:49:31.494Z] 08:49:31     INFO - 
[task 2018-08-27T08:49:31.495Z] 08:49:31     INFO - TEST-PASS | /dom/interfaces.html?exclude=Node | Event interface: document.createEvent("Event") must inherit property "NONE" with the proper type 
[task 2018-08-27T08:49:31.496Z] 08:49:31     INFO - TEST-PASS | /dom/interfaces.html?exclude=Node | Event interface: document.createEvent("Event") must inherit property "CAPTURING_PHASE" with the proper type 
[task 2018-08-27T08:49:31.496Z] 08:49:31     INFO - TEST-PASS | /dom/interfaces.html?exclude=Node | Event interface: document.createEvent("Event") must inherit property "AT_TARGET" with the proper type 
[task 2018-08-27T08:49:31.497Z] 08:49:31     INFO - TEST-PASS | /dom/interfaces.html?exclude=Node | Event interface: document.createEvent("Event") must inherit property "BUBBLING_PHASE" with the proper type 
[task 2018-08-27T08:49:31.497Z] 08:49:31     INFO - TEST-PASS | /dom/interfaces.html?exclude=Node | Event interface: document.createEvent("Event") must inherit property "eventPhase" with the proper type 
[task 2018-08-27T08:49:31.498Z] 08:49:31     INFO - TEST-PASS | /dom/interfaces.html?exclude=Node | Event interface: document.createEvent("Event") must inherit property "stopPropagation()" with the proper type 
[task 2018-08-27T08:49:31.498Z] 08:49:31     INFO - TEST-PASS | /dom/interfaces.html?exclude=Node | Event interface: document.createEvent("Event") must inherit property "cancelBubble" with the proper type 
[task 2018-08-27T08:49:31.499Z] 08:49:31     INFO - TEST-PASS | /dom/interfaces.html?exclude=Node | Event interface: document.createEvent("Event") must inherit property "stopImmediatePropagation()" with the proper type 
[task 2018-08-27T08:49:31.499Z] 08:49:31     INFO - TEST-PASS | /dom/interfaces.html?exclude=Node | Event interface: document.createEvent("Event") must inherit property "bubbles" with the proper type 
[task 2018-08-27T08:49:31.500Z] 08:49:31     INFO - TEST-PASS | /dom/interfaces.html?exclude=Node | Event interface: document.createEvent("Event") must inherit property "cancelable" with the proper type 
[task 2018-08-27T08:49:31.500Z] 08:49:31     INFO - TEST-UNEXPECTED-PASS | /dom/interfaces.html?exclude=Node | Event interface: document.createEvent("Event") must inherit property "returnValue" with the proper type - expected FAIL
[task 2018-08-27T08:49:31.500Z] 08:49:31     INFO - TEST-INFO | expected FAIL
Flags: needinfo?(alchen)
r+ if you fix those TEST-UNEXPECTED-PASSes by removing relevant lines from .ini
Add retrunValue into Event's interface
Comment on attachment 9004209 [details]
Bug 1452569 - Implement Event's returnValue. r=smaug

Should update the patch in the original page.
Attachment #9004209 - Attachment is obsolete: true
Flags: needinfo?(alchen)
Assignee: nobody → alchen
Pushed by ebalazs@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/707675409147
Implement Event's returnValue. r=smaug
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/707675409147
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Looking at the spec, there's a comment on the IDL at https://dom.spec.whatwg.org/#interface-event that says "historical" next to returnValue.

My question: what does that mean? Is it deprecated? Or is it just old? :)

I ask because I need to update the docs, and they currently call this property both deprecated and non-standard. It's no longer non-standard, but I need to sort out whether or not to call it deprecated. :)
In the context of the DOM spec, historical more or less means "this is a legacy API that is needed for compat reasons". I think the following comment for HTMLCollection captures the essense pretty well:

> HTMLCollection is a historical artifact we cannot rid the web of. While developers are of course welcome to keep using it, new API standard designers ought not to use it (use sequence<T> in IDL instead). 

Personally, I wouldn't call it deprecated.
Thanks! That's exactly what I thought, but it was worth being sure.

Documentation updated:

https://developer.mozilla.org/en-US/docs/Web/API/Event/returnValue
https://developer.mozilla.org/en-US/docs/Web/API/Event

BCD PR submitted: https://github.com/mdn/browser-compat-data/pull/2756

And listed on Firefox 63 for developers.
Depends on: 1502736
Component: DOM → DOM: Core & HTML
No longer depends on: 1478102
Regressions: 1478102
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: