1050638 - Allow closing a tab directly while the (tab-modal) onbeforeunload dialog is visible

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Justin Dolske [:Dolske]]

In bug 578828 comment 65, I took a quick look at the state of unbeforeonload on Mobile... Safari does not support it at all. Mobile Chrome/Firefox only support it for navigation, but not for closing a tab. (Desktop Firefox, Chrome, and Safari all always show a prompt.)

For example, in mobile Chrome/Firefox, the following occurs:

0) Load some random page to fill a history entry
1) Load http://dolske.net/mozilla/tests/prompt/onbeforeunload.html
2) Click back
3) Enjoy a onbeforeunload dialog blocking the navigation

But if in step 2 you simply close the tab, no prompt is shown. The tab is immediately closed.

Seems like we could pick up the same behavior for desktop Firefox.

Alternatively, we could show the prompt on the *first* attempt to close the tab, but make tab-closing UI abort the prompt the *second* time. Currently you can't close a tab when the prompt is already showing. This would allow closing such a page by clicking the close button twice, or pressing Control/Command-W twice. This would be a more conservative change, but given that the major 3 mobile browser already ignore it (when closing a tab), I think we should just go for it.

Comment 1

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

> Alternatively, we could show the prompt on the *first* attempt to close the
> tab, but make tab-closing UI abort the prompt the *second* time. Currently
> you can't close a tab when the prompt is already showing. This would allow
> closing such a page by clicking the close button twice, or pressing
> Control/Command-W twice.

I believe the behavior in desktop Firefox is exactly as described here ever since bug 616853 landed. I just tested on desktop Firefox and that seems to be the case.

> This would be a more conservative change, but given
> that the major 3 mobile browser already ignore it (when closing a tab), I
> think we should just go for it.

I'm on board with either behavior.

Comment 2

[Justin Dolske [:Dolske]]

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #1)
> > Alternatively, we could show the prompt on the *first* attempt to close the
> > tab, but make tab-closing UI abort the prompt the *second* time. Currently
> > you can't close a tab when the prompt is already showing. This would allow
> > closing such a page by clicking the close button twice, or pressing
> > Control/Command-W twice.
> 
> I believe the behavior in desktop Firefox is exactly as described here ever
> since bug 616853 landed. I just tested on desktop Firefox and that seems to
> be the case.

It turns out this depends on how the onbeforeunload prompt is triggered...

* If it's triggered by navigating, clicking the tab-close button will immediately close the tab as expected.

* But if you trigger the prompt by clicking the tab-close button, further clicks of the tab-close button do nothing.

Comment 3

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Just re-tested on Firefox release and you're right! That's a regression; I wouldn't have landed bug 616853 if that were the case when I landed.

Comment 4

[:Gijs (he/him)]

Tim's right, this worked on 2014-04-11's nightly (the first with the modal onbeforeunload dialogs). Trying to get a regression window now...

Comment 5

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a5a720259d79&tochange=e6614d8d85f9

Comment 6

[:Gijs (he/him)]

I'm going to go out on a limb and suggest this was bug 1041788...

Comment 7

[:Gijs (he/him)]

Tim, any reason we can't instead ignore PermitUnload when it's pending, and to close the tab immediately? We should then catch the tab already being closed once permitunload returns (which will happen because the in-content dialog gets destroyed).

I wonder if we can create a test for this. It'll be tricky because of the event loop stuff, I suspect, but there's a test for bug 1046022 which we can use as a base.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

(In reply to :Gijs Kruitbosch from comment #7)
> Tim, any reason we can't instead ignore PermitUnload when it's pending, and
> to close the tab immediately? We should then catch the tab already being
> closed once permitunload returns (which will happen because the in-content
> dialog gets destroyed).

I do like that idea. It might close the tab when hitting the close button twice as in bug 1041788 without really giving a chance to react to the dialog but that seems a really rare occurence and not as bad as not being able to close the tab.

> I wonder if we can create a test for this. It'll be tricky because of the
> event loop stuff, I suspect, but there's a test for bug 1046022 which we can
> use as a base.

Should be doable with setTimeout(gBrowser.removeTab) and some event loop spinning.

Comment 9

[:Gijs (he/him)]

(In reply to Tim Taubert [:ttaubert] (limited availability, work week) from comment #8)
> > I wonder if we can create a test for this. It'll be tricky because of the
> > event loop stuff, I suspect, but there's a test for bug 1046022 which we can
> > use as a base.
> 
> Should be doable with setTimeout(gBrowser.removeTab) and some event loop
> spinning.

Doable, yes, but not quite that way, because the timeout event gets added to the event loop that's there when you call it, and then we spin up the other one for the dialog, so it just stays queued all the time...

Comment 10

[:Gijs (he/him)]

Giving this points according to how much time I sunk into this before I picked it up already... but I have a patch, just working on a test at the moment.

Marco, can you add this?

Comment 11

[Marco Mucci [:MarcoM]]

Added to IT 35.3

Comment 12

[:Gijs (he/him)]

Attachment: should be able to close tab with onbeforeunload warning if clicking close a second time,

remote:   https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=571b13a1ae62

Comment 13

[:Gijs (he/him)]

Attachment: should be able to close tab with onbeforeunload warning if clicking close a second time,

Now with test files included. Note the getTabForBrowser change which nukes a Deprecated.jsm warning in the process, which happened to be annoying me because it muddled up my test output...
New try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a9a6fad16597

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8501450 [details] [diff] [review]
should be able to close tab with onbeforeunload warning if clicking close a second time,

Review of attachment 8501450 [details] [diff] [review]:
-----------------------------------------------------------------

The test doesn't fail if I remove the line that's bailing out _beginRemoveTab()?

::: browser/base/content/tabbrowser.xml
@@ +1964,5 @@
>                  delete aTab._pendingPermitUnload;
> +                // If we were closed during the unload, we return false now so
> +                // we don't (try to) close the same tab again. Of course, we
> +                // also stop if the unload was cancelled by the user:
> +                if (aTab._closedDuringPermitUnload || !permitUnload) {

Couldn't we just check |aTab.closing| instead of introducing a new property? That is set below.

::: browser/base/content/test/general/browser_double_close_tab.js
@@ +1,1 @@
> +const TEST_PAGE = "http://mochi.test:8888/browser/browser/base/content/test/general/file_double_close_tab.html";

I forgot, do tests not have headers anymore now that we changed the license?

Should add "use strict".

@@ +1,2 @@
> +const TEST_PAGE = "http://mochi.test:8888/browser/browser/base/content/test/general/file_double_close_tab.html";
> +var testTab;

nit: let

@@ +1,5 @@
> +const TEST_PAGE = "http://mochi.test:8888/browser/browser/base/content/test/general/file_double_close_tab.html";
> +var testTab;
> +
> +function waitForDialog(callback) {
> +  return new Promise((resolve, reject) => {

That's a fancy function. Returns a promise that never resolves and takes a callback ;)

@@ +13,5 @@
> +  });
> +}
> +
> +function waitForDialogDestroyed(node, callback) {
> +  return new Promise((resolve, reject) => {

Same?

Comment 15

[:Gijs (he/him)]

(In reply to Tim Taubert [:ttaubert] from comment #14)
> I forgot, do tests not have headers anymore now that we changed the license?

https://bugzilla.mozilla.org/show_bug.cgi?id=1073556#c0 says they don't need them.

Comment 16

[:Gijs (he/him)]

Attachment: should be able to close tab with onbeforeunload warning if clicking close a second time,

This fails without timing out if the patch isn't applied, and works with the patch applied, AFAICT. As discussed on IRC, aTab.closing isn't an option. New try push in a sec.

Comment 17

[:Gijs (he/him)]

 https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c1bfbf6093e2

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8501715 [details] [diff] [review]
should be able to close tab with onbeforeunload warning if clicking close a second time,

Review of attachment 8501715 [details] [diff] [review]:
-----------------------------------------------------------------

As per conversation on IRC:

::: browser/base/content/tabbrowser.xml
@@ +1952,5 @@
>  
>              var browser = this.getBrowserForTab(aTab);
> +            if (aTab._pendingPermitUnload && !aTabWillBeMoved) {
> +              aTab._closedDuringPermitUnload = true;
> +            } else if (!aTabWillBeMoved) {

Instead of introducing another property I think we should do:

if (!aTab._pendingPermitUnload && !aTabWillBeMoved) {

So that the second .removeTab() call wouldn't try to call .permitUnload().

@@ +1964,5 @@
>                  delete aTab._pendingPermitUnload;
> +                // If we were closed during the unload, we return false now so
> +                // we don't (try to) close the same tab again. Of course, we
> +                // also stop if the unload was cancelled by the user:
> +                if (aTab._closedDuringPermitUnload || !permitUnload) {

if (aTab.closing || !permitUnload) {
  return false;
}

So the first .removeTab() call would just bail out if another call removed the tab in the meantime.

Comment 19

[:Gijs (he/him)]

Attachment: should be able to close tab with onbeforeunload warning if clicking close a second time,

Use tab.closing instead...

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8502593 [details] [diff] [review]
should be able to close tab with onbeforeunload warning if clicking close a second time,

Review of attachment 8502593 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the test fixed.

::: browser/base/content/tabbrowser.xml
@@ +1959,5 @@
>                  // call before permitUnload() even returned.
>                  aTab._pendingPermitUnload = true;
>                  let permitUnload = ds.contentViewer.permitUnload();
>                  delete aTab._pendingPermitUnload;
> +                // If we were closed during the unload, we return false now so

Nit: it's technically not the unload but finding out whether we can unload. "onbeforeunload" :)

::: browser/base/content/test/general/browser_double_close_tab.js
@@ +1,1 @@
> +"use strict"

nit: "use strict";

@@ +42,5 @@
> +  // in an event queue. So when we spin a new event queue for a modal dialog...
> +  // everything gets messed up and the promise's .then callbacks never get
> +  // called, despite resolve() being called just fine.
> +  let dialogNode = yield new Promise(resolveOuter => {
> +    waitForDialog(function(dialogNode) {

nit: waitForDialog(dialogNode => {

@@ +44,5 @@
> +  // called, despite resolve() being called just fine.
> +  let dialogNode = yield new Promise(resolveOuter => {
> +    waitForDialog(function(dialogNode) {
> +      waitForDialogDestroyed(dialogNode, () => {
> +        let doCompletion = () => setTimeout(resolveOuter, 10);

That |10| here feels wrong... Why exactly is that here? Shouldn't we wait for either just the next tick or some special event/notification?

Comment 21

[:Gijs (he/him)]

One day, bugzilla will send review comments in the flag email and I will stop missing the fact that it wasn't a blanket r+. Now you get two commits for the price of one:

remote:   https://hg.mozilla.org/integration/fx-team/rev/a0ee07b214da
remote:   https://hg.mozilla.org/integration/fx-team/rev/1a6016480e7b

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a0ee07b214da
https://hg.mozilla.org/mozilla-central/rev/1a6016480e7b

Comment 23

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Reproduced the original issue on 35 Nightly from October 2nd (BuildID=20141002093155), on Win 7 x64. Clicking the "x" button (or using Ctrl+W) displayed the dialog, but then the "x" tab button and Ctrl+W would not work anymore.

The issue no longer reproduces in the latest Firefox 35 Nightly (BuildID=20141013030202), on Win 7 x64, Mac OS X 10.9.5, and Ubuntu 13.04 x64. Clicking the "x" button (or using Ctrl+W) displays the dialog, and now the "x" tab button and Ctrl+W work as expected even when the dialog is displayed, closing the tab (and window when closing the only tab). Closing the window and back navigation still work fine while the dialog displays.

While testing I found a scenario where the dialog shows twice, and the "x" tab button (or Ctrl+W) no longer work (dialog is not displayed, and tab/window is not closed). Since that issue is not directly related to this, behavior has been around ever since Firefox 4, and is tracked separately (https://bugzilla.mozilla.org/show_bug.cgi?id=305085#c12), I'm closing this issue as Verified.

Comment 24

[:Gijs (he/him)]

Comment on attachment 8502593 [details] [diff] [review]
should be able to close tab with onbeforeunload warning if clicking close a second time,

Approval Request Comment
[Feature/regressing bug #]: bug 1041788
[User impact if declined]: can't close tabs with onbeforeunload dialogs
[Describe test coverage new/current, TBPL]: has automated test
[Risks and why]: more tab closing regressions? But this is early beta, and it's been verified by QE already. The thing that caused this was uplifted, and I think the current situation is something we should also be addressing soon.
[String/UUID change made/needed]: nope

Comment 25

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8502593 [details] [diff] [review]
should be able to close tab with onbeforeunload warning if clicking close a second time,

(In reply to :Gijs Kruitbosch from comment #24)
> [Risks and why]: more tab closing regressions? But this is early beta, and
> it's been verified by QE already. The thing that caused this was uplifted,
> and I think the current situation is something we should also be addressing
> soon.

I agree. Let's get this into beta2. Beta+

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/98fc091c4706

Comment 27

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Also verified the fix on Windows 7 x64, Mac OS X 10.9.5, Ubuntu 12.04 x86, using Firefox 34 Beta 10 (BuildID=20141117202603), with same results as on Aurora 35 (comment 23).

Comment 28

[Jesse Ruderman]

Filed bug 1123986, for the original intent of this bug: getting rid of the dialog in some cases

Filed bug 1123987, for making this improvement to the dialog more obvious