Closed
Bug 1050638
Opened 10 years ago
Closed 10 years ago
Allow closing a tab directly while the (tab-modal) onbeforeunload dialog is visible
Categories
(Firefox :: Tabbed Browser, defect)
Firefox
Tabbed Browser
Tracking
()
People
(Reporter: Dolske, Assigned: Gijs)
References
(Blocks 1 open bug)
Details
(Keywords: regression)
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
ttaubert
:
review+
lmandel
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
> 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.
Reporter | ||
Comment 2•10 years ago
|
||
(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•10 years ago
|
||
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.
Assignee | ||
Updated•10 years ago
|
Keywords: regressionwindow-wanted
Assignee | ||
Comment 4•10 years ago
|
||
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...
Keywords: regression
Assignee | ||
Comment 5•10 years ago
|
||
Keywords: regressionwindow-wanted
Assignee | ||
Comment 6•10 years ago
|
||
I'm going to go out on a limb and suggest this was bug 1041788...
Blocks: 1041788
Assignee | ||
Comment 7•10 years ago
|
||
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.
Flags: qe-verify+
Flags: needinfo?(ttaubert)
Flags: in-testsuite?
Flags: firefox-backlog+
Comment 8•10 years ago
|
||
(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.
Flags: needinfo?(ttaubert)
Assignee | ||
Comment 9•10 years ago
|
||
(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...
Assignee | ||
Comment 10•10 years ago
|
||
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?
Status: NEW → ASSIGNED
Iteration: --- → 35.3
Points: --- → 5
Component: General → Tabbed Browser
Flags: needinfo?(mmucci)
Product: Toolkit → Firefox
Version: unspecified → Trunk
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8501449 -
Flags: review?(ttaubert)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → gijskruitbosch+bugs
Assignee | ||
Comment 13•10 years ago
|
||
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
Attachment #8501450 -
Flags: review?(ttaubert)
Assignee | ||
Updated•10 years ago
|
Attachment #8501449 -
Attachment is obsolete: true
Attachment #8501449 -
Flags: review?(ttaubert)
Comment 14•10 years ago
|
||
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?
Attachment #8501450 -
Flags: review?(ttaubert) → feedback+
Assignee | ||
Comment 15•10 years ago
|
||
(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.
Assignee | ||
Comment 16•10 years ago
|
||
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.
Attachment #8501715 -
Flags: review?(ttaubert)
Assignee | ||
Updated•10 years ago
|
Attachment #8501450 -
Attachment is obsolete: true
Assignee | ||
Comment 17•10 years ago
|
||
Comment 18•10 years ago
|
||
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.
Attachment #8501715 -
Flags: review?(ttaubert)
Assignee | ||
Comment 19•10 years ago
|
||
Use tab.closing instead...
Attachment #8502593 -
Flags: review?(ttaubert)
Assignee | ||
Updated•10 years ago
|
Attachment #8501715 -
Attachment is obsolete: true
Comment 20•10 years ago
|
||
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?
Attachment #8502593 -
Flags: review?(ttaubert) → review+
Updated•10 years ago
|
QA Contact: cornel.ionce
Assignee | ||
Comment 21•10 years ago
|
||
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
https://hg.mozilla.org/mozilla-central/rev/a0ee07b214da
https://hg.mozilla.org/mozilla-central/rev/1a6016480e7b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Comment 23•10 years ago
|
||
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.
Status: RESOLVED → VERIFIED
Assignee | ||
Comment 24•10 years ago
|
||
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
Attachment #8502593 -
Flags: approval-mozilla-beta?
Assignee | ||
Updated•10 years ago
|
status-firefox32:
--- → wontfix
status-firefox33:
--- → wontfix
status-firefox34:
--- → affected
status-firefox35:
--- → verified
Comment 25•10 years ago
|
||
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+
Attachment #8502593 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 26•10 years ago
|
||
Comment 27•10 years ago
|
||
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).
Assignee | ||
Updated•10 years ago
|
Flags: in-testsuite? → in-testsuite+
Updated•10 years ago
|
Summary: Suppress onbeforeunload dialogs when closing a tab. → Allow closing a tab directly while the (tab-modal) onbeforeunload dialog is visible
Comment 28•10 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•