Open Bug 968065 Opened 8 years ago Updated 3 years ago

Should returning a value from a beforeunload listener (NOT handler!) set returnValue on the beforeunload event?

Categories

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

x86
macOS
defect

Tracking

()

REOPENED

People

(Reporter: bzbarsky, Unassigned)

References

(Blocks 1 open bug)

Details

See http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#unloading-documents step 8:

  If the returnValue attribute of the event object is not the empty string, or if
  the event was canceled, then the user agent should ask the user to confirm that
  they wish to unload the document.

We do the returnValue check, but not the canceled check.

Olli, do you know why we don't check for preventDefault here?
Flags: needinfo?(bugs)
Blocks: 968240
Huh.  So we do.  In that case, I have no idea what the jQuery issue here is about.  Rick?
Flags: needinfo?(waldron.rick)
And it's been that way for a good long while.  Certainly ever since the hg.1 days.
(In reply to Boris Zbarsky [:bz] from comment #2)
> Huh.  So we do.  In that case, I have no idea what the jQuery issue here is
> about.  Rick?

I've looked through the blame and it's not clear to me either. I'm going to ping the author(s) of that code to find out more.
Flags: needinfo?(waldron.rick)
I was asked to participate in this ticket from this comment – https://github.com/jquery/jquery/commit/9dd0b010174dbfa70142a995a875a316337e1913#commitcomment-5289380, i wasn't given any other information related to this issue, so i'm gonna play a Jedi warrior here, as i understand, you currently reviewing jQuery source code for Firefox related hacks prompted by this discussion – https://twitter.com/jeresig/status/429019936506142720 right? It's great, long overdue and all that, although i might be out of job soon :-)

I assume you referring to this line – https://gist.github.com/rwaldron/8720084#file-jquery-js-L4566, if so, then you fixing the wrong "bug", this issue is not related to preventDefault in any way, comment there was changed and now is incorrect – https://github.com/jquery/jquery/commit/e47dfc63090df1af806520fe829639b897309232#commitcomment-5292362, i guess that has lead to the this confusion, but i honestly don't understand from where preventDefault thing has came out, but anyway, orignal comment was "Even when returnValue equals to undefined Firefox will still show alert"

which simply mean that if you do this in FF – 
window.addEventListener( "beforeunload", function( event ) {
	event.returnValue = undefined;
});

you get a prompt window, which is strictly speaking is a correct behavior given that spec state – 

"If the returnValue attribute of the event object is not the empty string, or if the event was canceled, then the user agent should ask the user to confirm that they wish to unload the document."

undefined is not an empty string, so technically you're correct here, but this is at least illogical, because returnValue initially equals to undefined value, so setting it to undefined should not trigger a prompt window, any other browser agrees with that, but you are not, this is why that hack is in place there.
> i honestly don't understand from where preventDefault thing has came out

Well, the comment said "does not alert", so I assumed the goal was to alert, which is what happens when preventDefault is called or returnValue is set.  ;)

> because returnValue initially equals to undefined value,

No, it's initially "".  http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#dom-beforeunloadevent-returnvalue says:

  The returnValue attribute represents the message to show the user. When the event is
  created, the attribute must be set to the empty string. 

> any other browser agrees with that

I just tested some other browsers on http://jsfiddle.net/uu7aZ/1/ and http://jsfiddle.net/uu7aZ/ and here's what I see:

1)  Chrome dev has the same behavior we do.
2)  Safari and Opera seem to never show the alert, no matter what (and in Safari the
    attribute is a boolean, not a string!).
3)  IE seems to have undefined as the default value

Sounds like there's no bug on our end here, at least...  ;)
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WORKSFORME
It appears i have to apologies before everyone, i screw up here pretty badly here. 

Current comment is correct although not complete, the answer why *now* that code is there is in this link – http://jsfiddle.net/z5KZ6/

you don't do this for addEventListener but only for window.onbeforeunload.
Oh yeah, Safari still does not show the alert if event is added to iframe, so in order for it to work in jsfiddle you have to view it directly – http://jsfiddle.net/z5KZ6/show
Hmm.  If I view http://jsfiddle.net/z5KZ6/show/ in Safari 6.1.1 on Mac and navigate away I get no alert.  Like I said, I can't get an alert in Safari no matter what I do.

I can confirm that I get an alert there in Chrome and IE.

That's really bizarre, because using addEventListener like that creates an EventListener, which will have its HandleEvent called (that calls the function in question) and that just returns a boolean, not a string.  So I have no idea why Chrome and IE are doing what they're doing....
Checked in Safari 7, 6.1, 5.1.7 they all show the alert – https://www.dropbox.com/s/6xzy9dvuttdbwqc/2014-02-06_1911.png

Since Opera 12.x will go away, firefox will be the only browser that does not do this.
That's quite confusing.  I wonder why you're seeing the alert in Safari but I'm not...

If you think the spec here needs to change, you should probably raise the spec issue.
Huh.  And now Safari is showing the alerts for me.  Weird.  :(

But OK, it sounds like either the other browsers need to change their behavior or the spec needs to change.
> I wonder why you're seeing the alert in Safari but I'm not...
This whole thing is, actually, never was embarrassed like this in my life

> If you think the spec here needs to change, you should probably raise the spec issue.
I know, i know i remember your phrase from back when - "If the spec says to do something, you do it. If it says to not do something, you don't do it". It's just a de facto implementation, it certainly wouldn't hurt if you would change your mind, but we can live without it. It's one of the things that jQuery is dealing with and i assume there always will be something like that. 

I guess my job perspectives are not so gloomy after all :-)
Just for future reference, given this testcase:

  var value = /* Put some value here */;
  window.addEventListener("beforeunload", function(e) {
    return value;
  });

I see an alert in Safari/Chrome for true, false, 0, 1, "", document.all.  I do not see an alert for null or undefined.

I also tried this:

  window.addEventListener("beforeunload", { handleEvent: function(e) {
    return value;
  } });

and see the same behavior.  So speccing this will in fact need to use a type other than EventListener for beforeunload somehow (since the return value of that is a boolean that is supposed to call preventDefault if false).  In at least Chrome returning false here does _not_ preventDefault the event...
Anne, you'll love this one.  ;)

Reopening and resummarizing to cover the actual issue here.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Summary: preventDefault of beforeunload event should show beforeunload prompt → Should returning a value from a beforeunload listener (NOT handler!) set returnValue on the beforeunload event?
> since the return value of that is a boolean that is supposed to call preventDefault if
> false

Er, no, that's for event handlers.  For event listeners the return value is supposed to be ignored.
https://github.com/whatwg/dom/pull/407/files defines returnValue as a boolean. Is it the same returnValue being discussed here? Should this bug be considered the bug to implement the boolean returnValue from that spec PR?

I take it that assigning false to returnValue should call preventDefault() considering that what the spec PR says matches what the spec says about preventDefault() but our preventDefault() runs quite a bit more code. Correct?
Flags: needinfo?(bugs)
It's a different returnValue. This is about https://html.spec.whatwg.org/multipage/browsing-the-web.html#beforeunloadevent.

It seems Chrome's bug has been fixed meanwhile. Not sure what remains here given that.
(In reply to Anne (:annevk) from comment #19)
> It's a different returnValue. This is about
> https://html.spec.whatwg.org/multipage/browsing-the-web.
> html#beforeunloadevent.

How do the two interact considering that they appear to be in the same inheritance hierarchy?
They each do their own thing as defined and don't interact directly. This was discussed in https://github.com/whatwg/dom/issues/334#issuecomment-281404452.
(Anne answered to ni?)
Flags: needinfo?(bugs)
Priority: -- → P3
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.