Implement Event's returnValue

RESOLVED FIXED in Firefox 63

Status

()

P3
normal
RESOLVED FIXED
a year ago
9 days ago

People

(Reporter: annevk, Assigned: alchen)

Tracking

({dev-doc-complete, good-first-bug, site-compat})

unspecified
mozilla63
dev-doc-complete, good-first-bug, site-compat
Points:
---
Dependency tree / graph
Bug Flags:
webcompat +

Firefox Tracking Flags

(firefox63 fixed)

Details

(Whiteboard: [webcompat:p3])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

a year ago
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.
Flags: webcompat+
Whiteboard: [webcompat:p3]

Comment 1

9 months ago
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
Keywords: good-first-bug
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.
(Assignee)

Comment 6

7 months ago
Add retrunValue into Event's interface
(Assignee)

Comment 8

7 months ago
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?
(Assignee)

Updated

7 months ago
Attachment #9003481 - Flags: feedback?(bugs) → review?(bugs)
(Assignee)

Comment 10

7 months ago
(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+
(Assignee)

Updated

7 months ago
Attachment #9003481 - Flags: review?(bugs)
(Assignee)

Updated

7 months ago
Keywords: checkin-needed
Attachment #9003481 - Attachment description: Bug 1452569 - Implement Event's returnValue → Bug 1452569 - Implement Event's returnValue. r=smaug

Comment 12

7 months ago
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
(Assignee)

Comment 15

7 months ago
Add retrunValue into Event's interface
(Assignee)

Comment 16

7 months ago
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)

Updated

7 months ago
Assignee: nobody → alchen

Comment 18

7 months ago
Pushed by ebalazs@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/707675409147
Implement Event's returnValue. r=smaug
Keywords: checkin-needed

Comment 19

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/707675409147
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox63: --- → fixed
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.
Keywords: dev-doc-needed → dev-doc-complete

Updated

5 months ago
Depends on: 1502736
Depends on: 1478102
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.