1135303 - "Close Tabs to the Right" should not warn when closing multiple tabs

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect, P2)

Description

[Mats Palmgren (inactive)]

STR
0. create a fresh profile
1. open Preferences and uncheck "Warn me when closing multiple tabs"
2. restart
3. open a few tabs so that there are 3 or more open
4. click "Close Tabs to the Right" on the context menu of the left-most tab

ACTUAL RESULTS
A warning dialog opens:
"You are about to close 2 tabs...Continue?" [Cancel][Close Tabs]

EXPECTED RESULTS
No warning, just close the tabs, because that's what an unchecked
"Warn me when closing multiple tabs" preference means.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

The "Warn me when closing multiple tabs" checkbox is currently hooked up to .warnOnClose. I don't see a checkbox there that is hooked up to .warnOnCloseOtherTabs.

warnOnCloseOtherTabs is the pref that is checked when closing multiple tabs but not the whole window. MXR shows that it is defined and read, but never set.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

This may have been broken some time between bug 866880, bug 896291, bug 887515, and bug 931891.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

http://hg.mozilla.org/mozilla-central/rev/4e1073ed5389 regressed this. It looks like the fix is to remove the `aCloseTabs == this.closingTabsEnum.ALL` check from the condition at http://hg.mozilla.org/mozilla-central/annotate/5f1009731a97/browser/base/content/tabbrowser.xml#l1939

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

As well as lines 1933 and 1934 need to be fixed too to show the checkbox.

Comment 6

[Dão Gottwald [:dao]]

This is an old regression but I guess we now get more Chrome users migrating to Firefox and using "Close Tabs to the Right". Might be the right time to pick this up again.

Comment 7

[Mark Horstman]

Or in my case because the "Tab Mix Plus" extension doesn't work with FF v57, and TMP doesn't have this issue.

Comment 8

[rob.miller.gh]

Also applies to 'close other tabs' context menu entry.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1135303 - Honor the browser.tabs.warnOnCloseOtherTabs for closing tabs to the right.

Review commit: https://reviewboard.mozilla.org/r/206354/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/206354/

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 8935459 [details]
Bug 1135303 - Honor the browser.tabs.warnOnCloseOtherTabs for closing tabs to the right.

https://reviewboard.mozilla.org/r/206354/#review212936

::: browser/base/content/test/tabs/browser_closeOtherTabs.js:30
(Diff revision 1)
> +  })
> +});
> +
> +function addTabs(numberOfTabsToAdd) {
> +  for (let i = 0; i < numberOfTabsToAdd; i++) {
> +    BrowserTestUtils.addTab(gBrowser, "about:blank", { skipAnimation: true });

This can be simplified to gBrowser.addTab(); BrowserTestUtils does nothing useful here, and the animation hopefully doesn't make a difference?

::: browser/base/content/test/tabs/browser_closeOtherTabs.js:37
(Diff revision 1)
> +}
> +
> +add_task(async function test_closeOtherTabs() {
> +  is(gBrowser.tabs.length, 1, "There should only be one tab to start the test");
> +  ok(Services.prefs.getBoolPref(pref), "warnOnCloseOtherTabs should be set to true by default");
> +  expectedPrompt = true;

nit: add blank line above

::: browser/base/content/test/tabs/browser_closeOtherTabs.js:41
(Diff revision 1)
> +  ok(Services.prefs.getBoolPref(pref), "warnOnCloseOtherTabs should be set to true by default");
> +  expectedPrompt = true;
> +  dialogReturnValue = BUTTON_CANCEL;
> +  warnOnCloseValue = true;
> +  addTabs(3);
> +

nit: remove blank line

::: browser/base/content/test/tabs/browser_closeOtherTabs.js:55
(Diff revision 1)
> +  addTabs(3);
> +  warnOnCloseValue = false;
> +  gBrowser.removeAllTabsBut(gBrowser.tabs[0]);
> +  // For reasons I don't understand, removeAllTabsBut seems to return
> +  // before the tabs have been removed whereas I don't see that same
> +  // behavior with removeTabsToTheEndFrom.

removeAllTabsBut uses {animate: true}, whereas removeTabsToTheEndFrom doesn't (which seems wrong).

::: browser/base/content/test/tabs/browser_closeOtherTabs.js:69
(Diff revision 1)
> +  addTabs(3);
> +  gBrowser.removeAllTabsBut(gBrowser.tabs[0]);
> +  await TestUtils.waitForCondition(() => gBrowser.tabs.length == 1, "Tab length should be 1");
> +  is(gBrowser.tabs.length, 1, "Only one tab should remain");
> +
> +  Services.prefs.clearUserPref(pref);

please use registerCleanupFunction for this, or use SpecialPowers.pushPrefEnv

Comment 11

[Dão Gottwald [:dao]]

I also wonder if we should just not prompt for close to right / close other tabs, and remove browser.tabs.warnOnCloseOtherTabs...

Comment 12

[rob.miller.gh]

I would be happy with removing the warning all together (I do want and use the functionality).  The prompt warning does not seem necessary since the user can just use history -> recently closed tabs if they did not mean to close.

Comment 13

[Emma Mason]

Poking this bug in response to this post on Twitter - https://twitter.com/jar349/status/1089610465775374336

What is the status here? Would be nice to get this landed for folks.

Comment 14

[rob.miller.gh]

The twitter post is correct, it still warns when closing multiple tabs to the right.

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Dão Gottwald [::dao] from comment #11)

I also wonder if we should just not prompt for close to right / close other
tabs, and remove browser.tabs.warnOnCloseOtherTabs...

Yeah, I think this is the better route.

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1135303 - Remove the warning when closing other tabs or tabs to the right. r?dao

Note that the warning message for closing a window with multiple tabs is still present, and that text in English reads 'Warn me when I attempt to close multiple tabs' for the checkbox. This checkbox was only shown when closing the window, so its context isn't changing but I can see how someone might interpret that preference to protect against closing multiple but not all tabs (it never did). I tried to think of ways to tweak this checkbox text to be less ambiguous but in the end I couldn't think of anything better so left it as-is.

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1135303 - Don't warn when closing multiple tabs if sessionstore can undo close the requested amount of tabs. r?dao

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f5b403254529
Don't warn when closing multiple tabs if sessionstore can undo close the requested amount of tabs. r=Gijs,mikedeboer

Comment 19

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/f5b403254529

Comment 20

[Sajedul Islam]

I have reproduced this bug with Nightly 38.0a1 (2015-02-20) on Windows 10 , 64 Bit !

This bug's fix is Verified with latest Nightly 67.0a1 !

Build ID : - 20190223041557
User Agent : - Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:67.0) Gecko/20100101 Firefox/67.0

[bugday-20190220]

Comment 21

[Maria Berlinger [:mberlinger], Services QA]

Updating the flags based on comment 20.