"Close other tabs" should show the confirmation dialog if the number of tabs is greater than browser.sessionstore.max_tabs_undo

VERIFIED FIXED in Firefox 27

Status

()

defect
--
major
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: smaug, Assigned: jaws)

Tracking

(Blocks 1 bug, {dataloss, regression})

25 Branch
Firefox 27
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox24 unaffected, firefox25 unaffected, firefox26 unaffected, firefox27+ verified)

Details

Attachments

(1 attachment, 6 obsolete attachments)

about:config says I have browser.tabs.warnOnCloseOtherTabs set to default which is true.
and I'm 100% "Close other tabs" used to have confirmation dialog with a number
of to-be-closed tabs, since I used that to check how many tabs I have open.
I just lost around 150 tabs because there isn't the dialog anymore :/
Just reproduced with another profile.
Severity: normal → major
Note, I haven't figured out a way to restore all the closed tabs.
FF seem to restore only some tabs. Max 10 or so.
Would creating a sqlite database of closed tabs be feasible in this case?
See bug 887515. Apparently there's a menu item that you can use to restore all of the closed tabs (either the same as the existing menu but its behavior changed, or its a different menu option? I'm not sure).
Yes, you can click on "Undo Closed Tabs" (note the plurality) to undo the action. When only one tab has been closed it should read "Undo Closed Tab".
(In reply to Olli Pettay [:smaug] from comment #2)
> Note, I haven't figured out a way to restore all the closed tabs.
> FF seem to restore only some tabs. Max 10 or so.

This is a problem. Sorry for your loss of tabs. The dialog should be shown for more than our limit of tabs to undo-closing.
Blocks: 866880
OS: Linux → All
Hardware: x86_64 → All
Version: unspecified → 25 Branch
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Keywords: regression
Summary: "Close other tabs" doesn't have the confirmation dialog anymore → "Close other tabs" should show the confirmation dialog if the number of tabs is greater than browser.sessionstore.max_tabs_undo
Posted patch 896291.patch (obsolete) — Splinter Review
I'm not sure if we should show the prompt if the user has browser.tabs.warnOnClose set to false. I think it might be worth it since they won't have a way to restore those tabs otherwise, but they've also opted in to not being warned.
Attachment #779316 - Flags: review?(dao)
Blocks: 887515
No longer blocks: 866880
Review ping for Tim or Dao?
Flags: needinfo?(ttaubert)
Flags: needinfo?(dao)
Comment on attachment 779316 [details] [diff] [review]
896291.patch

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

::: browser/base/content/tabbrowser.xml
@@ +1566,5 @@
>            }
>  
>            if (tabsToClose <= 1 ||
> +              (aCloseTabs != this.closingTabsEnum.ALL &&
> +               tabsToClose <= Services.prefs.getIntPref("browser.sessionstore.max_tabs_undo")) ||

Shouldn't the middle of the condition be (modified for readability):

let maxUndo = Services.prefs.getIntPref("browser.sessionstore.max_tabs_undo");
let warnOther = Services.prefs.getBoolPref("browser.tabs.warnOnCloseOtherTabs");

(aCloseTabs != this.closingTabsEnum.ALL && (tabsToClose <= maxUndo || !warnOther))
Oh, wait. I think that's what you meant in your comment but you probably wanted to say 'warnOnCloseOtherTabs'. I think we should respect that setting, if not the pref is completely unused and could be removed.
Flags: needinfo?(ttaubert)
If we don't respect that setting and Olli has =< 10 tabs he'll be mad at us for closing these ;)
Hm, trying to remember what I was thinking back then is tough. Maybe I did mean warnOnCloseOtherTabs? I don't see a problem with keeping that pref around, it's probably what I should have used when writing this patch.

Should I upload a new patch that references the different pref now or is there anything else that I should change while I'm updating the patch?
Flags: needinfo?(dao)
So... this feels like the condition is a little more complicated now. We don't want to show the confirmation dialog if:

1) tabsToClose <= 1

If there's only one tab to close we'll never warn you about that.

2) aCloseTabs == this.closingTabsEnum.ALL && !browser.tabs.warnOnClose

If the whole window is going to be closed we never warn with warnOnClose=false.

3) aCloseTabs != this.closingTabsEnum.ALL &&
   (!browser.tabs.warnOnCloseOtherTabs ||
    tabsToClose <= browser.sessionstore.max_tabs_undo)

If "other tabs" or "tabs to the right" are closed we never warn with warnOnCloseOtherTabs=false. We also don't warn if we're closing less than max_tabs_undo tabs.

So basically, if (1 || 2 || 3) return true; Did I miss anything? The test lgtm.

(Please disregard comment #11, I forgot that we want to explicitly remove the warning dialog for users as long as we can restore all those tabs.)
Attachment #779316 - Flags: review?(ttaubert)
(In reply to Tim Taubert [:ttaubert] from comment #13)
> So... this feels like the condition is a little more complicated now. We
> don't want to show the confirmation dialog if:
> 
> 1) tabsToClose <= 1
> 
> If there's only one tab to close we'll never warn you about that.
> 
> 2) aCloseTabs == this.closingTabsEnum.ALL && !browser.tabs.warnOnClose
> 
> If the whole window is going to be closed we never warn with
> warnOnClose=false.
> 
> 3) aCloseTabs != this.closingTabsEnum.ALL &&
>    (!browser.tabs.warnOnCloseOtherTabs ||
>     tabsToClose <= browser.sessionstore.max_tabs_undo)
> 
> If "other tabs" or "tabs to the right" are closed we never warn with
> warnOnCloseOtherTabs=false. We also don't warn if we're closing less than
> max_tabs_undo tabs.
> 
> So basically, if (1 || 2 || 3) return true; Did I miss anything? The test
> lgtm.
> 
> (Please disregard comment #11, I forgot that we want to explicitly remove
> the warning dialog for users as long as we can restore all those tabs.)

This is almost correct.

For case 3, we need to disregard the warnOnCloseOtherTabs preference when tabsToClose > max_tabs_undo and always show the warning in this case since there is no undo.
Hmm. I'm not sure if we should do that, we certainly didn't do it before. On the one hand the user opted into not being warned about closing other tabs. OTOH, they might expect that to be revertible if done by mistake.

I see where you're coming from but we'd change the current behavior by doing that. And while there certainly are users that are lucky to be notified about an irreversible change, there will be users that are annoyed we don't respect their preferences as we did before.

IMO, the right thing with (warnOnCloseOther=false && tabsToClose > max_tabs_undo) would probably be to not warn and temporarily raise the max_tabs_undo limit, but that might not be totally trivial to implement.
(In reply to Jared Wein [:jaws] from comment #14)
> For case 3, we need to disregard the warnOnCloseOtherTabs preference when
> tabsToClose > max_tabs_undo and always show the warning in this case since
> there is no undo.

The whole point of the (hidden) warnOnCloseOtherTabs pref is to allow users to disable that warning.
Posted patch Patch v2 (obsolete) — Splinter Review
Attachment #779316 - Attachment is obsolete: true
Attachment #779316 - Flags: review?(dao)
Attachment #787143 - Flags: review?(dao)
Comment on attachment 787143 [details] [diff] [review]
Patch v2

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

The test unfortunately fails when run in single-mode on my machine:

TEST-UNEXPECTED-FAIL | browser_bug896291.js | All other tabs should be closed when the 'OK' option on the prompt is chosen - Got 4, expected 1
TEST-UNEXPECTED-FAIL | browser_bug896291.js | Found an unexpected tab at the end of test run: http://mochi.test:8888/
TEST-UNEXPECTED-FAIL | browser_bug896291.js | Found an unexpected tab at the end of test run: http://mochi.test:8888/
TEST-UNEXPECTED-FAIL | browser_bug896291.js | Found an unexpected tab at the end of test run: http://mochi.test:8888/

::: browser/base/content/tabbrowser.xml
@@ +1639,5 @@
>            }
>  
> +          let maxUndo =
> +            Services.prefs.getIntPref("browser.sessionstore.max_tabs_undo");
> +          let warnOnOtherTabs =

Nit: maybe warnOnCloseOtherTabs?

@@ +1641,5 @@
> +          let maxUndo =
> +            Services.prefs.getIntPref("browser.sessionstore.max_tabs_undo");
> +          let warnOnOtherTabs =
> +            Services.prefs.getBoolPref("browser.tabs.warnOnCloseOtherTabs");
> +          let warnOnWindow =

Nit: maybe warnOnCloseWindow?

@@ +1649,4 @@
>            if (tabsToClose <= 1 ||
> +              (aCloseTabs == closingAll && !warnOnWindow) ||
> +              (aCloseTabs != closingAll && (!warnOnOtherTabs ||
> +                                            tabsToClose <= maxUndo)))

Can you please add a comment that breaks this condition down and explains the rationale behind it? I just had to re-read my comment because even with your changes it's not quite readable enough IMO, especially if one hasn't touched that code for a while.
Attachment #787143 - Flags: review?(ttaubert)
Don't forget to update the preference

>
>-    if (reallyClose && !warnOnClose.value)
>+    if (reallyClose && !warnOnClose.value) {
>+      let pref = aCloseTabs == this.closingTabsEnum.ALL ?
>+                 "browser.tabs.warnOnClose" : "browser.tabs.warnOnCloseOtherTabs"
>       Services.prefs.setBoolPref(pref, false);
>+    }
>
Posted patch Patch (obsolete) — Splinter Review
The test was passing when I uploaded the patch. Bug 894048 landed in the interim, I've disabled the tab animation preference in the test now to get the test passing.

(In reply to onemen.one from comment #19)
> Don't forget to update the preference
> 
> >
> >-    if (reallyClose && !warnOnClose.value)
> >+    if (reallyClose && !warnOnClose.value) {
> >+      let pref = aCloseTabs == this.closingTabsEnum.ALL ?
> >+                 "browser.tabs.warnOnClose" : "browser.tabs.warnOnCloseOtherTabs"
> >       Services.prefs.setBoolPref(pref, false);
> >+    }
> >

This was fixed in bug 909662.
Attachment #787143 - Attachment is obsolete: true
Attachment #787143 - Flags: review?(dao)
Attachment #799701 - Flags: review?(ttaubert)
Comment on attachment 799701 [details] [diff] [review]
Patch

Felipe, can you review this since Tim is on PTO?
Attachment #799701 - Flags: review?(felipc)
(In reply to Jared Wein [:jaws] from comment #20)
> Created attachment 799701 [details] [diff] [review]
> Patch
> 
> The test was passing when I uploaded the patch. Bug 894048 landed in the
> interim, I've disabled the tab animation preference in the test now to get
> the test passing.
> 
> (In reply to onemen.one from comment #19)
> > Don't forget to update the preference
> > 
> > >
> > >-    if (reallyClose && !warnOnClose.value)
> > >+    if (reallyClose && !warnOnClose.value) {
> > >+      let pref = aCloseTabs == this.closingTabsEnum.ALL ?
> > >+                 "browser.tabs.warnOnClose" : "browser.tabs.warnOnCloseOtherTabs"
> > >       Services.prefs.setBoolPref(pref, false);
> > >+    }
> > >
> 
> This was fixed in bug 909662.

NO.
bug 909662 only deal with "browser.tabs.warnOnClose"
with this bug, user also can see the confirmation dialog when closing other tab
and when user un-check the box in the dialog you need to set "browser.tabs.warnOnCloseOtherTabs" to false not "browser.tabs.warnOnClose"
Comment on attachment 799701 [details] [diff] [review]
Patch

Ok, sorry for the misunderstanding. I see what you mean here.
Attachment #799701 - Flags: review?(ttaubert)
Attachment #799701 - Flags: review?(felipc)
Posted patch Patch v1.1 (obsolete) — Splinter Review
Now updating the preference correctly.
Attachment #799701 - Attachment is obsolete: true
Attachment #800907 - Flags: review?(felipc)
Comment on attachment 800907 [details] [diff] [review]
Patch v1.1

As Tim is back, sending it to him since he has already thought more about this change
Attachment #800907 - Flags: review?(felipc) → review?(ttaubert)
Comment on attachment 800907 [details] [diff] [review]
Patch v1.1

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

::: browser/base/content/tabbrowser.xml
@@ +1647,5 @@
> +          let warnOnCloseOtherTabs =
> +            Services.prefs.getBoolPref("browser.tabs.warnOnCloseOtherTabs");
> +          let warnOnCloseWindow =
> +            Services.prefs.getBoolPref("browser.tabs.warnOnClose");
> +          let closingAll = this.closingTabsEnum.ALL;

Looks like instead of having a shortcut to the const we could save (aCloseTabs == this.closingTabsEnum.ALL) to a variable (maybe isWindowClosing) and use that in all the conditions.

::: browser/base/content/test/browser_bug896291.js
@@ +75,5 @@
> +
> +  originalTab = gBrowser.selectedTab;
> +  gBrowser.selectedBrowser.loadURI("http://mochi.test:8888/");
> +
> +  maxTabsUndo = Services.prefs.getIntPref("browser.sessionstore.max_tabs_undo");

We need to clear this pref after the test finishes as well.
Attachment #800907 - Flags: review?(ttaubert) → review+
This should no longer track Firefox 25 since the patches that caused this regression were backed out in bug 914258. Moving tracking to Firefox 26.
Posted patch Patch v1.2 (obsolete) — Splinter Review
(In reply to Tim Taubert [:ttaubert] from comment #26)
> Comment on attachment 800907 [details] [diff] [review]
> Patch v1.1
> 
> Review of attachment 800907 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/tabbrowser.xml
> @@ +1647,5 @@
> > +          let warnOnCloseOtherTabs =
> > +            Services.prefs.getBoolPref("browser.tabs.warnOnCloseOtherTabs");
> > +          let warnOnCloseWindow =
> > +            Services.prefs.getBoolPref("browser.tabs.warnOnClose");
> > +          let closingAll = this.closingTabsEnum.ALL;
> 
> Looks like instead of having a shortcut to the const we could save
> (aCloseTabs == this.closingTabsEnum.ALL) to a variable (maybe
> isWindowClosing) and use that in all the conditions.

Agreed, change made.

> ::: browser/base/content/test/browser_bug896291.js
> @@ +75,5 @@
> > +
> > +  originalTab = gBrowser.selectedTab;
> > +  gBrowser.selectedBrowser.loadURI("http://mochi.test:8888/");
> > +
> > +  maxTabsUndo = Services.prefs.getIntPref("browser.sessionstore.max_tabs_undo");
> 
> We need to clear this pref after the test finishes as well.

This pref isn't changed by the test, so no need to reset it when the test finishes.
Attachment #800907 - Attachment is obsolete: true
Attachment #805639 - Flags: review+
Attachment #805639 - Attachment is patch: true
Attachment #805639 - Attachment mime type: text/x-patch → text/plain
https://hg.mozilla.org/integration/fx-team/rev/0da508ef253c
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Posted patch Patch v1.21 (obsolete) — Splinter Review
Fixed test, it was broken when run after a test that closed multiple tabs.
Attachment #805639 - Attachment is obsolete: true
Attachment #806273 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/2e69de47685e
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Backed out in https://hg.mozilla.org/integration/fx-team/rev/b27ea519eebe since something in that wad of checkin-needed caused browser_homeDrop.js to time out, and according to that log in comment 30, you've already done that once.
Whiteboard: [fixed-in-fx-team]
I have fixed the browser_homeDrop.js hang in bug 918069.

The patch in that bug and the patch that I'll land for this bug depend on bug 917887 which moves the test directory.
Depends on: 917887
Posted patch Patch v2Splinter Review
This patch avoids the issues that I was having with the mock object factory by using the WindowListener pattern. This will also be a safer patch to uplift to Aurora.

Pushed to tryserver: https://tbpl.mozilla.org/?tree=Try&rev=6283aede0c9c
Attachment #806273 - Attachment is obsolete: true
Attachment #813956 - Flags: review?(ttaubert)
Comment on attachment 813956 [details] [diff] [review]
Patch v2

The production code changes were already r+'d by Tim, so this patch only contains changes to the test code. I talked with Matt about this earlier, so he may be able to review it as well.
Attachment #813956 - Flags: review?(mnoorenberghe+bmo)
Comment on attachment 813956 [details] [diff] [review]
Patch v2

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

r=me on the test

::: browser/base/content/test/general/browser.ini
@@ +198,5 @@
>  [browser_bug839103.js]
>  [browser_bug880101.js]
>  [browser_bug882977.js]
>  [browser_bug887515.js]
> +[browser_bug896291.js]

Better test filename please

::: browser/base/content/test/general/browser_bug896291.js
@@ +7,5 @@
> +let maxTabsUndo;
> +let maxTabsUndoPlusOne;
> +
> +function verifyUndoMultipleClose() {
> +  let cancelRemoveAllTabsDialogListener = new WindowListener("chrome://global/content/commonDialog.xul", function (domwindow) {

Nit: s/domwindow/domWindow/g

@@ +12,5 @@
> +    ok(true, "dialog appeared in response to multiple tab close action");
> +    domwindow.document.documentElement.cancelDialog();
> +    Services.wm.removeListener(cancelRemoveAllTabsDialogListener);
> +
> +    let acceptRemoveAllTabsDialogListener = new WindowListener("chrome://global/content/commonDialog.xul", function (domwindow) {

Nit: name the anonymous functions (4x) with non-trivial bodies.

@@ +13,5 @@
> +    domwindow.document.documentElement.cancelDialog();
> +    Services.wm.removeListener(cancelRemoveAllTabsDialogListener);
> +
> +    let acceptRemoveAllTabsDialogListener = new WindowListener("chrome://global/content/commonDialog.xul", function (domwindow) {
> +      ok(true, "dialog appeared in response to multiple tab close action");

Nit: It would be a bit easier to read if the acceptRemoveAllTabsDialogListener sub-test was a separate top-level function.

@@ +19,5 @@
> +      Services.wm.removeListener(acceptRemoveAllTabsDialogListener);
> +
> +      waitForCondition(function () gBrowser.tabs.length == 1, function () {
> +        is(gBrowser.tabs.length, 1,
> +           "All other tabs should be closed when the 'OK' option on the prompt is chosen");

s/when/after/

@@ +25,5 @@
> +      }, "Waited too long for the other tabs to be closed.");
> +    });
> +
> +    Services.wm.addListener(acceptRemoveAllTabsDialogListener);
> +    waitForCondition(function () gBrowser.tabs.length == 1 + maxTabsUndoPlusOne, function () {

Is the waitForCondition really necessary here?

@@ +87,5 @@
> +    var self = this;
> +    domwindow.addEventListener("load", function() {
> +      domwindow.removeEventListener("load", arguments.callee, false);
> +
> +      ok(true, "domwindow.document.location.href: " + domwindow.document.location.href);

This seems mostly informational so it can be an info() instead.
Attachment #813956 - Flags: review?(ttaubert)
Attachment #813956 - Flags: review?(mnoorenberghe+bmo)
Attachment #813956 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/1dd4cd4890fc
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Keywords: verifyme
I've filed bug 931891 to back out the patches that caused this regression from Fx26 and Fx27. This is similar to what we did for bug 914258.
Removing tracking from Firefox 26 since the patches that caused this regression have been backed out of Beta 26 in bug 931891. The patches are also expected to be backed out of Fx27 along with the rest of this feature. The feature is planned to be ride the trains once bug 895436 is fixed.
I have reproduced the issue that Olli had: Pref "browser.tabs.warnOnCloseOtherTabs" set to true, opened more then 10 tabs, click on "Close other tabs" (no confirmation dialog was prompted), used "Undo closed tabs" (only 10 tabs are reopened).

With latest Aurora the confirmation dialog is prompted but using "Undo closed tab" I can only reopen a max of 10 tabs. Is this expected, or there is already a bug on this?

This bug is verified as fixed since this is about Close other tabs not displaying the confirmation dialog.
Status: RESOLVED → VERIFIED
Flags: needinfo?(jaws)
Keywords: verifyme
(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #42)
> I have reproduced the issue that Olli had: Pref
> "browser.tabs.warnOnCloseOtherTabs" set to true, opened more then 10 tabs,
> click on "Close other tabs" (no confirmation dialog was prompted), used
> "Undo closed tabs" (only 10 tabs are reopened).

I am assuming here you are talking about using a beta1 build? This feature will be backed out in beta2.

> With latest Aurora the confirmation dialog is prompted but using "Undo
> closed tab" I can only reopen a max of 10 tabs. Is this expected, or there
> is already a bug on this?

This is expected. We show the confirmation dialog when we will be unable to restore all of the closed tabs if the action proceeds any further. After accepting the dialog and closing the group of tabs, Undo Close Tabs will only restore as many as we can. This is not a regression from before, where the user could choose the "Undo Close Tab" menuitem 10 times but no more (this simply groups that operation in to one action instead of 10).
Flags: needinfo?(jaws)
You need to log in before you can comment on or make changes to this bug.