333734 - Protect against opening too many tabs at once

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, enhancement, P3)

Description

[Håkan Waara]

I just accidently middle-clicked the "Latest headlines" bookmark. All I can say is ouch! 39 tabs were simultaneously loading, and my whole computer hung for a good while.

bsmedberg had a good suggestion:  we should protect against (accidently) opening too many tabs at once.

This would be similar to how we warn before opening a window with existing tabs.

So if, for example, the user tries to open 5 or more tabs at once, we would put up a dialog, asking if this is really appropriate.

Comments, ideas?

Comment 1

[:Gavin Sharp [email: [e-mail]]]

*** Bug 285543 has been marked as a duplicate of this bug. ***

Comment 2

[Joe Hughes]

The other option would be to make opening a gajillion tabs at the same time work well (probably by loading them in batches to avoid hammering the system).  I heard that someone had taken a look at this in the past, anyone have a bug # handy?

Comment 3

[Håkan Waara]

(In reply to comment #2)
> The other option would be to make opening a gajillion tabs at the same time
> work well (probably by loading them in batches to avoid hammering the system). 
> I heard that someone had taken a look at this in the past, anyone have a bug #
> handy?

That doesn't really help if the action was a mistake from the beginning...

Comment 4

[zug_treno]

Duplicate of bug 253806?

(In reply to comment #2)
> I heard that someone had taken a look at this in the past, anyone have a bug #
> handy?

Maybe bug 326735?

Comment 5

[:Gavin Sharp [email: [e-mail]]]

*** Bug 253806 has been marked as a duplicate of this bug. ***

Comment 6

[Mike Connor [:mconnor]]

Hmm, so without some sort of dialog, I doubt that we could properly interrupt the addition of multiple tabs in an obvious way.  Five is way too few though, I'd put the number at more than 15, so its really going to hammer the system real good.

Its trivial to implement, really, but not especially a feature, so blocking for radar, b1 for review.

Comment 7

[Mike Connor [:mconnor]]

load-balancing tabbrowser stuff to sspitzer

Comment 8

[Mike Connor [:mconnor]]

gah.  s/.org/.com/!

Comment 9

[(not reading, please use [e-mail] instead)]

I remember that gemal's linky would prompt you before opening "too many" tabs, and that tbird would prompt you before opening "too many" messages, with "Opening <x> messages, may be slow.  Continue? [ok] [cancel]".

so if we do a prompt, it could be like the "warn against closing multiple tabs" prompt, with a check box that matches a pref with UI.

we have:

"You are about to close <x> open tabs.  Are you sure you want to continue?
[x] warn me when I attempt to close multiple tabs
[close tabs] [cancel]"

we could add the other half:

"You are about to open <x> tabs.  Are you sure you want to continue?
[x] warn me when I attempt to open multiple tabs
[open tabs] [cancel]"

and then add pref UI to the tabs pane.

Comment 10

[(not reading, please use [e-mail] instead)]

Attachment: a patch, based on existing code (and UI) to warn on closing tabs

Comment 11

[(not reading, please use [e-mail] instead)]

Attachment: screen shot of pref UI

Comment 12

[(not reading, please use [e-mail] instead)]

Attachment: screen shot of prompt UI

Comment 13

[(not reading, please use [e-mail] instead)]

> I'd put the number at more than 15

I chose 15, following mconnor's comment, but made it a (hidden) pref in all.js.  

it's hidden as there is no UI to change that value.

Since there is a UI change, I'll request a review from beltzner.

Comment 14

[(not reading, please use [e-mail] instead)]

Comment on attachment 224393 [details] [diff] [review]
a patch, based on existing code (and UI) to warn on closing tabs

seeking sr= from ben, too.

Comment 15

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 224393 [details] [diff] [review]
a patch, based on existing code (and UI) to warn on closing tabs

You get to enjoy my nit-machine. 

>Index: browser/components/bookmarks/content/bookmarks.js
>+        var promptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"].getService(Components.interfaces.nsIPromptService);

This line is really long. Are other lines in this file this long? An alternative is:

var promptService = 
    Components.classes["@mozilla.org/embedcomp/prompt-service;1"].
    getService(Components.interfaces.nsIPromptService);

>+        //default to true: if it were false, we wouldn't get this far
>+        var warnOnOpen = { value:true };

nit: 

{ value: true };

(space between : and t)

>+        var buttonPressed = promptService.confirmEx(window,
>+            BookmarksUtils.getLocaleString('tabs.openWarningTitle'),

nit: single quotes here, but double quotes elsewhere in file. 

>+            BookmarksUtils.getLocaleString('tabs.openWarningPromptMe'),

ditto

>Index: browser/components/preferences/tabs.xul
>       <preference id="browser.tabs.warnOnClose"        name="browser.tabs.warnOnClose"        type="bool"/>
>+      <preference id="browser.tabs.warnOnOpen"         name="browser.tabs.warnOnOpen"        type="bool"/>

nit: note the space alignment issue. I sorta wish I'd never lined these up this way.

>Index: browser/locales/en-US/chrome/browser/preferences/tabs.dtd
>+<!ENTITY warnOnOpen.label                    "Warn when opening multiple tabs">
>+<!ENTITY warnOnOpen.accesskey                "o">

For the preferences UI, this is sort of vague. Multiple = 2, but you don't warn (by default) when only 2 tabs are to be opened. Being more specific is probably a good idea here, especially since the sense is slightly different from "warn on close multiple tabs" which will always prompt, regardless of number of tabs. Now, providing the exact number could be considered too much detail (second opinion?) so if that's the case a compromise might be "Warn when opening very many tabs" or "a lot of tabs" (flip side: too vague, but not sure people will care about the detail)

I'd like to see this one particular thing resolved in some way (I'll kick the first person who suggests including a text box in prefs to control the .warnOnOpen pref ;-) before marking sr=. The rest looks OK to me nits accounted for.

Comment 16

[(not reading, please use [e-mail] instead)]

in addition to addressing the nits in my patch, I can fix those same nits in the existing "warn on close code" code in http://lxr.mozilla.org/mozilla1.8/source/toolkit/content/widgets/tabbrowser.xml#1187

I see your point about the warning.  

Thinking about it, the wording should imply that this warning has nothing to do with having lots of tab open, it has to do with opening up many tabs at the same time from bookmarks UI.  Does that impact how we want to word this?

additionally, if I choose a bookmarks folder as my homepage, and the folder has > 15 bookmarks, it will not warn me.  This seems good to me, but it is something I hadn't considered.

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

Maybe

  [x] Warn me when I open more than [ 5] tabs

?

Comment 18

[Håkan Waara]

(In reply to comment #17)
> Maybe
> 
>   [x] Warn me when I open more than [ 5] tabs

I don't see a point in exposing a textfield to set this pref, since the value is arbitrary anyway...  I think something deliberately more fuzzy like "Warn when opening many tabs at once" is okay.  

We're supposed to figure out what "many tabs" is - not push that decision to the user. :-)

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

Yeah, I suppose that's a good point (and that I should stop commenting on bugs at 3 in the morning). The tradeoffs here are:

 - too vague can lead to user expectations not being met
 - too specific can lead to users wanting to control the exact number

Ben's idea in comment 15 seems the most sensible, some wording options:

 [ ] Warn me before opening a lot of tabs at once
 [ ] Ask me before opening many tabs at once
 [ ] Warn me before opening enough tabs that I'd regret it

Comment 20

[(not reading, please use [e-mail] instead)]

for comparison, here are the strings I'm using in my newest patch (I'll attach it).  It might help to see the "open" ones alongside the "close" ones.

<!ENTITY warnOnClose.label                   "Warn when closing multiple tabs">

tabs.closeWarningTitle=Confirm close
tabs.closeWarning=This Browser window has %S tabs open. Do you want to close it and all its tabs?
tabs.closeButton=Close all tabs
tabs.closeWarningPromptMe=Warn me when closing multiple tabs


<!ENTITY warnOnOpen.label                    "Warn when opening a lot of tabs at
 once">

tabs.openWarningTitle=Confirm open
tabs.openWarningMultiple=You are about to open %S tabs. Are you sure you want to
 open these tabs?
tabs.openButtonMultiple=Open tabs
tabs.openWarningPromptMe=Warn me when I attempt to open a lot of tabs at once

Comment 21

[(not reading, please use [e-mail] instead)]

Attachment: new patch, addressing ben's comments and the new wording

Comment 22

[(not reading, please use [e-mail] instead)]

Comment on attachment 224787 [details] [diff] [review]
new patch, addressing ben's comments and the new wording

note to ben, I also made some similar nit fixes to tabbrowser.xml (that you asked me to make to bookmarks.js)

Comment 23

[(not reading, please use [e-mail] instead)]

Attachment: prompt UI (new wording)

Comment 24

[(not reading, please use [e-mail] instead)]

Attachment: options UI (new wording)

Comment 25

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 224787 [details] [diff] [review]
new patch, addressing ben's comments and the new wording

r=[e-mail] for this.

Comment 26

[(not reading, please use [e-mail] instead)]

note, I need to port this functionality back to the trunk to work with places and the other bookmarks changes on the trunk.

Comment 27

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #23)
> Created an attachment (id=224788) [edit]
> prompt UI (new wording)

I should have caught this before. Either make the warning a warning (not a question) or chance the "OK"/"Cancel" to a "Yes"/"No". Personally, I prefer making the warning a statement that explains why the action is deserving of a warning like:

"You are about to open %s tabs at once. This might slow your system down as the pages are loading."

Comment 28

[Håkan Waara]

(In reply to comment #27)
> I should have caught this before. Either make the warning a warning (not a
> question) or chance the "OK"/"Cancel" to a "Yes"/"No". [...]

Please do not change button titles from the very descriptive "Open tabs"/"Cancel" to a "Yes"/"No" variant though, that'd be one step backwards.

Comment 29

[Mike Beltzner [:beltzner, not reading bugmail]]

Gah. Yes. Of course. I meant:

 .-------------------------------------------------------------------.
 | You have asked to open %s tabs at once. This might slow down your |
 | system while the pages are loading.                               |
 |                                                                   |
 |   [x] Warn me when I attempt to open lots of tabs at once         |
 |                                                                   |
 |                 ( Open Tabs ) ( Cancel )                          |
 '-------------------------------------------------------------------'

or, explicitly:

+tabs.openWarningMultiple=You have asked to open %S tabs at once. This might slow down your system while the pages are loading.

Comment 30

[(not reading, please use [e-mail] instead)]

beltzner, good point about adding an explanation as to why we are prompting.  I'll switch to your version of the warning and attach a final patch (and some new screen shots.)

I'm not a wordsmith, so if you know of any (lurking in bugzilla), can you cc them on this bug and let them take a crack at re-wording the warning before the l10n freeze?

Comment 31

[Mike Beltzner [:beltzner, not reading bugmail]]

I'm usually better than I have been in this particular bug, but I'll get deb to take a peek at it. Although string changes are easy to do after the fact.

cc'ing Axel since this'll need some l10n support.

Comment 32

[(not reading, please use [e-mail] instead)]

Attachment: prompt UI (newer wording from beltzner)

Comment 33

[(not reading, please use [e-mail] instead)]

Attachment: the patch, ported to places (for the trunk)

Comment 34

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 224966 [details] [diff] [review]
the patch, ported to places (for the trunk)

As usual, lots of nits:

>Index: browser/components/places/content/controller.js
>   openLinksInTabs: function PC_openLinksInTabs() {
>+    var PREF = 
>+        Components.classes["@mozilla.org/preferences-service;1"].
>+        getService(Components.interfaces.nsIPrefBranch);

lower case "pref"


>+    const kWarnOnOpenPref = "browser.tabs.warnOnOpen";
>+    if (PREF.getBoolPref(kWarnOnOpenPref))
>+    {

Style in this file is:

if (pref.getBoolPref(kWarnOnOpenPref)) {

r=[e-mail] with nits fixed.

Comment 35

[(not reading, please use [e-mail] instead)]

I've fixed the coding style changes that ben pointed and checked in on the trunk.

I also fixed this problem with my patch that I failed to catch before on the trunk:

<            pref.setBoolPref(kWarnOnOpen, false);
---
>            pref.setBoolPref(kWarnOnOpenPref, false);

and the branch:

<            PREF.setBoolPref(kWarnOnOpen, false);
---
>            PREF.setBoolPref(kWarnOnOpenPref, false);

yikes!  I've fixed that mistake on both branch and trunk.

Comment 36

[Henrik Gemal]

(In reply to comment #33)
> Created an attachment (id=224966) [edit]
> the patch, ported to places (for the trunk)
> 

"You have asked to open %S tabs at once. This might slow down your system while the pages are loading."

should we really mix "tabs" and "pages"?

Comment 37

[Peter van der Woude [:Peter6]]

this caused Bug 340966

Comment 38

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #36)
> should we really mix "tabs" and "pages"?

Yes. Pages load in tabs. Opening the tabs themselves isn't slow, it's the action of loading pages within those tabs.

Comment 39

[(not reading, please use [e-mail] instead)]

> this caused Bug 340966

I'll look into that QI failure on middle click (on the trunk)

Comment 40

[(not reading, please use [e-mail] instead)]

I have a fix in hand for the QI alert, but it pointed out a scenario I have not yet protected against (at least on trunk, I need to look into the branch.)

if you multiple select many individual bookmarks and then attempt to open them in tabs, we should warn you (heeding the pref values, of course).

Comment 41

[mmortal03]

Just FYI, I regularly open folders of bookmarks into tabs with 25 or more inside.  The cutoff value IS configurable, right?  Is this option on by default?

Comment 42

[(not reading, please use [e-mail] instead)]

> The cutoff value IS configurable, right?  

yes.  for details on the hidden pref that control the cutoff, see #340937

> Is this option on by default?

yes.

Comment 43

[dolphinling]

People here may be interested in bug 341046, which I just filed.

FWIW, this would be nothing but an annoyance to me, and I immediately turned it off. 

Comment 44

[Grey Nicholson (they/them)]

"You have asked to open" seems a bit too passive on the part of the user - they *told/ordered/demanded* that Firefox open the tabs. I think "You are about to open" is a better choice of words; for a start it matches "You are about to close".

Comment 45

[Henrik Gemal]

(In reply to comment #44)
> "You have asked to open" seems a bit too passive on the part of the user - they
> *told/ordered/demanded* that Firefox open the tabs. I think "You are about to
> open" is a better choice of words; for a start it matches "You are about to
> close".
> 

good point. perhaps you should open a new bug since this is fixed

Comment 46

[Grey Nicholson (they/them)]

(In reply to comment #45)
> (In reply to comment #44)
> good point. perhaps you should open a new bug since this is fixed

Filed bug 342930.

Comment 47

[Christian Ascheberg]

The shown number of tabs that are about to be opened seems to be wrong.
When I middle-click click a folder that has subfolders, these subfolders are counted as pages. So it happens that I am warned that 16 tabs will be opened although there are only 10 or so pages.

Comment 48

[Adam Guthrie]

[e-mail], feel free to file a follow-up bug on that issue and make it depend on this one, but commenting in this bug will get you nowhere.

Comment 49

[(not reading, please use [e-mail] instead)]

> The shown number of tabs that are about to be opened seems to be wrong.
> When I middle-click click a folder that has subfolders, these subfolders are
> counted as pages. So it happens that I am warned that 16 tabs will be opened
> although there are only 10 or so pages.

good catch, thank you.  I've logged bug #344040 on this.