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

Ignore beforeunload event return value #1001

Merged
merged 1 commit into from
Apr 8, 2016
Merged

Conversation

domenic
Copy link
Member

@domenic domenic commented Apr 5, 2016

Instead of using it as the message to show to the user, simply compare
it against the empty string or not. This matches Gecko, WebKit, and
Blink (but not yet EdgeHTML).

Closes #952.

@avidrissman, can you review?

Ideally we'd probably get rid of returnValue entirely (and the weird return behavior) and just use event.preventDefault(), but I assume that's not web-compatible and nobody wants to try. So instead I just added lots of notes about how the DOMStringiness of returnValue is a historical artifact.

@domenic domenic added the removal/deprecation Removing or deprecating a feature label Apr 5, 2016
@avidrissman
Copy link

I think this is cool; LGTM. I totally didn't know that doing event.preventDefault() also works. You're right, of course, that we can't just drop the string result deal. Oh well. I'll settle for this.

@@ -82377,13 +82377,14 @@ dictionary <dfn>PageTransitionEventInit</dfn> : <span>EventInit</span> {

<dl class="domintro">

<dt><var>event</var> . <code subdfn data-x="dom-BeforeUnloadEvent-returnValue">returnValue</code> [ = <var>value</var> ]</dt>
<dt><var>event</var> . <code subdfn data-x="dom-BeforeUnloadEvent-returnValue">returnValue</code> [ = " " ]</dt>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a little weird. Are you trying to say that the only valid value is a space?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to say that setting it to a generic value is meaningless, so here's an example of what you could do.

Maybe we should w-nodev this whole section and remove the domintro box since devs should be using e.preventDefault() anyway.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the latter solution more I think.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps with // historical annotation in IDL.

@domenic
Copy link
Member Author

domenic commented Apr 6, 2016

I switched things up a bit (sorry no fixup commit) to make it clearer that preventDefault() is the correct thing to do. I added some authoring guidance and wording about how the entire interface is legacy, instead of marking the whole thing w-nodev, since it seems weird to exclude an entire interface from the dev version.

One thing we could do is move the interface into https://html.spec.whatwg.org/#obsolete, but I wasn't sure, since it seems somewhat important to keep it near the rest of the unloading algorithms.

@avidrissman
Copy link

Still LGTM.

@annevk
Copy link
Member

annevk commented Apr 7, 2016

LGTM too. Leaving to @domenic to merge in case he changes his mind about moving the interface overnight.

Instead of using it as the message to show to the user, simply compare
it against the empty string or not. This matches Gecko, WebKit, and
Blink (but not yet EdgeHTML).

Closes #952.
@domenic domenic merged commit 01d9870 into master Apr 8, 2016
@annevk annevk deleted the no-beforeunloadstring branch April 8, 2016 02:22
@domenic domenic mentioned this pull request Jun 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
removal/deprecation Removing or deprecating a feature
Development

Successfully merging this pull request may close these issues.

3 participants