Closed Bug 191578 Opened 22 years ago Closed 20 years ago

"Close Other Tabs" Needs a Confirmation Dialog

Categories

(SeaMonkey :: Tabbed Browser, defect)

Product:
SeaMonkey
Component:
Tabbed Browser
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla1.8alpha1

People

(Reporter: Peter, Assigned: jag+mozilla)

References

Details

(Keywords: dataloss, Whiteboard: See comment #72 - No context menu reordering discussions)

Attachments

(1 file, 7 obsolete files)

Final patch again, with one more variable rename (not edited directly this time... apologies... I don't work via patches too often)
5.11 KB, patch
neil
: review+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.3b) Gecko/20030131
Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.3b) Gecko/20030131

Bug 103354 made the mistake of removing the enormously useful ability to "Close
All Tabs". The reason was justified (it's too easy to accidentally select).

However, removing "Close Other Tabs" has *severly* limited the usefulness of
tabs! The whole point of tabs is to easily have many of them, and also to just
as easily remove then once they are no longer needed.

I agree that the previous implemetation was too risky - closing all other tabs
by accident was too likely. So what to do? 

I don't like the "modifier" idea in bug 191492 because it is undiscoverable and
I've never seen context menus change with modifier keys (in windows).

S U G G E S T E D   S O L U T I O N:
I suggest to reverse the "fix" for this bug and instead when someone
(accidentally) choses "Close other Tabs" there would be a confirmation dialog
(similar to when you "delete" stuff. It would be fairly unobtrusive because the
user could just hit <spacebar> or <ESC> to accept or deny the confirmation. That
would be sufficient safety while being discoverable and unobtrusive.

Reproducible: Always

Steps to Reproduce:
Keywords: mozilla1.3, nsbeta1, nsbranch, nsCatFood
Flags: blocking1.3b?
ben, would you mind rendering another UI decision?
The addition of a confirmation dialog was suggested and turned down in bug 103354.
Doesn't closing the window close all tabs?

We don't have a confirm on quit (and aren't going to get one, forget the bug
number); adding one to close all tabs seems silly... Suggest WONTFIX.
Whiteboard: wontfix?
The summary should probably be changed.

The functionality that Peter is requesting closes all but one tab.
That's why it's called "Close Other Tabs" *not* "Close All tabs"
Close other tabs has been removed. This bug is meaningless. Do not reopen or
file additional bugs to restore the option. If you want to make UI proposals
take it to the newsgroups. Creating bugs in bugzilla to challenge a UI decision
made by the module owner is abuse and it won't be tollerated. If you want to
challenge that decision or propose alternate solutions take it to the newsgroup.
Status: UNCONFIRMED → RESOLVED
Closed: 22 years ago
Flags: blocking1.3b? → blocking1.3b-
Resolution: --- → INVALID
verified invalid
Status: RESOLVED → VERIFIED
> ... take it to the newsgroups.

sorry, and fine :(

Last comment here: Removing "Close Other Tabs" has made tabs 50% less useful. :(
Summary: "Close All Tabs" Needs a Confirmation Dialog → "Close Other Tabs" Needs a Confirmation Dialog
There is a discussion to fix this aweful removal of "Close Other Tabs" here:

news://news.mozilla.org:119/3e3ae2ba$0$227$626a54ce@news.free.fr
The fitting Title: how to get "close other tabs" back !!!
Please reopen this bug (or is another more suitable?) -- somehow "Close other
tabs" is back in 1.4b (and rc3 in fact).

The placement of "close this tab" right next to "close other tabs" makes Mozilla
tabbed browsing almost useless to me. Irregardless of where the command is
placed (as I can see some might find it useful), it seems obvious from the hot
debate on the subject (bug 103354 etc) that there should be a preference --
hidden perhaps -- to remove the command from its present location.
*** Bug 210424 has been marked as a duplicate of this bug. ***
Reopening since close-other-tabs is now back in the browser.

A soltion in mind is to implement a confirmation very similar to what happens
when you close a window full of tabs.
Status: VERIFIED → UNCONFIRMED
Resolution: INVALID → ---
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: wontfix?
andrews, have you requested approval of your patch?

PS. Do the Assignee and QA guys still exist?
Keywords: mozilla1.3 → dataloss
nope, i am unsure of how to.
would be delighted if someone could tell me how. do i just set the '?' flag on
review?
no idea about QA/Assignee.
Yes, i believe you need to select the "?" next to "review" and enter your e-mail
under "Requestee" in the attached patch. Just try it... (we'll both learn) ;)
you request review from a qualified reviewer, not yourself. i'd suggest
super-review?jaggernaut
(OT) Timeless, why "sr"? Isn't there supposed to be an "r" first?

Do you know if jaggernaut and pmac are still active in this project?
Attachment #128576 - Flags: superreview?(jaggernaut)
Attachment #128576 - Flags: approval1.5b?
Comment on attachment 128576 [details] [diff] [review]
patch v1 should do the trick, adds a confirmation similar to what when you close a window full of tabs

please don't request approval to land a patch which hasn't been through the
proper review and super-review. thanks.
Attachment #128576 - Flags: approval1.5b?
Comment on attachment 128576 [details] [diff] [review]
patch v1 should do the trick, adds a confirmation similar to what when you close a window full of tabs

I think the button text is too long. Would "Close other tabs" do? That still
seems a bit to long...

I'm not sure I like this logic in tabbrowser. What would you think of having it
in navigator.xul somewhere, registered as a callback (or event + listener?)
with tabbrowser?
The dialoge should be similar to the dialoge for closing a window with multiple
tabs (although perhaps not *quite* as long). How about:

+- Confirm Close Other Tabs ---------------------------+
|                                                      |
| You are about to close [4] other tabs. Proceed?      |
|                                                      |
|  [x] Warn me about closing other tabs in the future. |
|                                                      |
|         [ Close Other Tabs ]    [ Cancel ]           |
+------------------------------------------------------+

We can fine-tune the wording (if needed) in a future bug. Let's just get this
fix in quickly, please.
Once again, I accidentally closed all my tabs and was left with only the one
I meant to close (arghh!!).  So I guess I'm in favor of this enhancement 
request.  However, a simple "undo" would do just as well!
jag: i thought you had a patch. What's the status?

re #19: perhaps put the code that same place the "close windows with thabs
warning" is in. (disclaimer: i'm no programmer)
Peter: I didn't have a patch. I like the "Close Other Tabs" text better. And
yeah, we should look into how much code we can share for this. I won't have time
anytime soon, CCing someone who might.
Comment on attachment 128576 [details] [diff] [review]
patch v1 should do the trick, adds a confirmation similar to what when you close a window full of tabs

I can't see any obvious improvement if the pref, caption and title, etc will be
different, nor does it appear to be easy to move the code into navigator.js
Code reuse opportunity!  Shouldn't this bug be fixed using the code that fixed
Bug 108973 (Multi-tabbed windows should confirm close)?  
Code reuse is a good thing, so if at all possible yes (just make sure you don't
run into internationalization issues).

This also affects Firebird.

I also wonder why "Close Tab" appears after "Close other Tabs" on the list on
Firebird. It makes it heard to perform that action.
For me personally this a very annoying "feature" because it's right next to the
opposite function. I would really prefer to see this configurable or at least
have a separator in between.... they are way to close and the text is way to
similar for that. I really do like the modifier idea! "Shift" would be perfect.
Modifiers for context menus *are* a common thing under windows
A separator would do as a quick workaround, but it wouldn't prevent *all*
mistaken selections of "Close Other Tabs". The idea of the shift modifier is as
one commenter mentioned "undiscoverable" - and it too fails to address the issue
of accidental mis-selection.

Finally, there is no valid argument against consistency. If "Close All Tabs"
requires a confirmation dialogue (and it does: that's why there is one) then so
does "Close Other Tabs".

I can't understand why the team are dragging their heels over this, it's simply
a no-brainer. It's not as if it's hard to do - the submitter even provided a
patch. This fix *should* be in 1.5.
I was suprised (but pleased) to see this bug alive again.

> This also affects Firebird.

AFAIK, it was decided NOT to implement a warning when closing all tabs (bug
189888) - so I would reasonably suspect that the same logic would apply here,
and the developers would also decide to WONTFIX any bug that suggested a warning
for closing other tabs...  (Then again, a previous bug on this exact subject was
also marked WONTFIX, and now it being reconsidered, so who knows.)

> I really do like the modifier idea!

See bug 191492 - it's not incompatible with this bug, but it is something
different.  (This bug is only about a confirmation dialog.)

> I can't understand why the team are dragging their heels over this

They're not - they just don't have the time to implement it right now.  (See
comment 23 which indicates to me that they're not against it in principle.)
> AFAIK, it was decided NOT to implement a warning when closing all tabs (bug
> 189888)

Aehm... but there *is* a warning dialog when closing a browser with all tabs
Same should happen when closing the "other tabs" for the same reasons!

> See bug 191492 - it's not incompatible with this bug, but it is something
> different.  (This bug is only about a confirmation dialog.)

A modifier key would still be the best way IMO - but a dialog would be good
enough ...and we have a patch sitting here! Why not apply it until someone finds
the time to implement the modifier approach?

The current situation is just unacceptable
> there *is* a warning dialog when closing a browser with all tabs

Only with Mozilla, not with Firebird - which is what my comment 29 was in
reference to.

> we have a patch sitting here! Why not apply it

Because it doesn't have the proper reviews yet.  Once it's received approval
(after it's been tweaked/modified appropriately) then it can be checked in. 
Complaining isn't going to help.  As with any of the bugs in this open source
project, you can either wait for the volunteers to make the changes, or make the
changes yourself.  Simply saying that it's awful and needs to be fixed isn't
going to help move things along:
http://bugzilla.mozilla.org/page.cgi?id=etiquette.html
I'd have to agree w/ Torsten.  The confirmation should be there (it is in
Mozilla).  If it's not in Firebird, it's a UI design issue that is missing; if
the confirmation was taken out, something else needs to be done because I still
find myself missing the Close Tab button occasionally, even though this bug was
reported a long time ago :-P
One suggestion I have is we remove "Close Other Tabs" altogether and implement
bug 102132 "Ability to move content areas from tabs into windows (Link/delink tabs)"
Then people could just delink the tab(s) they want to keep and close the rest by
closing the main window.
Not sure if this will satisfy the users of this "Close Other Tabs" but I'd love
to see this link/delink feature anyway! If this would be a consensus... that
would be great!
The de-link tabs to windows is an inadequate solution. It requires more mouse
clicks and more mouse distance to get to the state as the close other tabs does.
Close other tabs must stay (hopefully). Any further discussion is OT here.
Please take it to a newsgroup.
...as inadequate and inconsistent is the *current* situation

Looking at all comments on this bug it seems that a dialog (with a checkbox
for those that don't wanna see it ever again) is the only possible consensus.

> Close other tabs must stay (hopefully). Any further discussion is OT here.

(Please mind your words) If a dialog is still no option we should really take
this to a list. Otherwise which actions are necessary to get the patch applied?
I was also thinking from a UI perspective that if the link/delink feature is
added, that will be yet another thing on the context menu. We obviously must
make some concessions when it comes to the UI and prioritize, or the UI will
look like my bedroom did when I was in junior high (messy). The "Close other
tabs" feature is a danger, while a confirmation dialog is a hassle for people,
they will disable it, then the feature will do more good. In my opinion, when a
feature could cause dataloss, it should be a bit harder to perform. The extra
two steps might save someone's form entries.
Please stop SPAMming this bug!  (My apologies to anybody who is actually on-topic.)

Link/delink is something completely different and has nothing to do with this
bug.  If you're interested in THAT feature then see bug 102132. This bug is ONLY
about a confirmation dialog on closing other tabs.
This is a defect. Dataloss/critical.
Severity: enhancement → critical
Flags: blocking1.6b?
Flags: blocking1.6?
Flags: blocking1.4.2?
Wouldn't hold the releases for this feature request. Minusing.
Flags: blocking1.6b?
Flags: blocking1.6b-
Flags: blocking1.6?
Flags: blocking1.6-
Flags: blocking1.4.2?
Flags: blocking1.4.2-
Arrrrrrgh, I accidentally misclicked again and had 10 tabs open, loading in the
background. Now I have to go find the links again, click them all again, and
load the pages. What a hassle!!! A confirmation dialog can be stupid, but and
UNDO feature hidden under the Edit menu would be quite welcome right now! There
is an undo for when I type into this bug comment box, why isn't there an undo
for close other tabs? 
You can look in your history to see what pages you were on. If you are doing a
form, then that is a different story, as you will have lost the form data.
*** Bug 227345 has been marked as a duplicate of this bug. ***
This is not a feature request. It is a bug.
Flags: blocking1.7a?
as anyone looked at this patch, or played with it/tested it?
Flags: blocking1.7b?
Flags: blocking1.7a?
Flags: blocking1.7a-
Not going to hold the release for this.
Flags: blocking1.7b?
Flags: blocking1.7b-
Flags: blocking1.6b-
Flags: blocking1.6-
Flags: blocking1.4.2-
Flags: blocking1.3b-
*** Bug 237033 has been marked as a duplicate of this bug. ***
Again 10 pages that I had to restore from the history!
Pleeeeeease! If this "feature" is really so controversial
please move it down in the context menu ...like it is in
Firefox. That way it's less likely to be hit accidently.

This bug was openend more than a year(!) ago. There even
is a patch available. And still it's being postponed
over and over again! I would help but there *is* a patch
already available and I am no committer here.

Is there any technical reason not to apply the patch?

I don't wanna count the time I've lost over the time
because of this so called "feature".
yes, Yes, YES. Fix the damn bug already. What is the matter with you people, are
you deaf or just ignorant? Or maybe you are Microsoft-planted confederates
intended to bring the mozilla project into disrepute? It must be one of these
because I can conveive of no rational explanation why such a damaging usability
bug, and one that is so easy to fix, should have been left unattended for so long.

(In reply to comment #48)
> Again 10 pages that I had to restore from the history!
> Pleeeeeease! If this "feature" is really so controversial
> please move it down in the context menu ...like it is in
> Firefox. That way it's less likely to be hit accidently.
> 
> This bug was openend more than a year(!) ago. There even
> is a patch available. And still it's being postponed
> over and over again! I would help but there *is* a patch
> already available and I am no committer here.
> 
> Is there any technical reason not to apply the patch?
> 
> I don't wanna count the time I've lost over the time
> because of this so called "feature".
Ralph, your comment does absolutely nothing to help the development of this
feature/bug.  In fact, your comment will discourage any developer from fixing
this anytime soon.  

Please read http://bugzilla.mozilla.org/page.cgi?id=etiquette.html before
commenting.
What can be done except providing a patch and to wait?
We would love to help but "soon" becomes a little relative
having the lifetime of the bug in mind anyway ;)

...but you are right - let's focus on fixing it!

Has anyone had a look at the patch yet?
I fear it's to old to just apply it but did anyone try so far?

Peter?
Ralph: Why don't YOU fix it? Pick up that patch and finish it. Why don't you
take a look at the number of open bugs. There are a lot higher priority bugs
than this ones, bugs that cause things like crashes and such. Comparing us to
Microsoft, a company is satisfied with cosmetic fixes but nothing to their
underlying code, is uncalled for. The developers here are volunteer, and they
give their free time to bring you a good browser product. Give them some respect.
Flags: blocking1.8a?
Flags: blocking1.7?
(In reply to comment #52)
> Comparing us to Microsoft, a company is satisfied with cosmetic fixes
> but nothing to their underlying code, is uncalled for.

This bug was opened more than 12 months ago and the patch was created almost six
months after that. I don't see how time is an issue here since there has been a
lot given to the developers already. Taking shots at Microsoft isn't
constructive. I seem to recall a number of "cosmetic fixes" (name changes)
recently with the browser ;)

Part of the root problem with "Close other tabs" from a UI design standpoint
rather than a practical stanpoint, is it is not "simple." It is not a simple
action on a single UI object, it is a an action on the result of boolean logic
on a set of UI objects. This is the sort of thing that makes for inelegant
interfaces.

How about replacing this feature with an action to convert a given tab to it's
own window? Then the User can simply close the old window with the many tabs.
I'd even go so far as to say no menu control is needed, simply the ability to
drag and drop the tab out the old window. Wouldn't that be so much more elegant?
See comment #33 and some others up around there. Delinking tabs is bug 102132
and I suggested that but if I recall correctly there were complaints about it.
Comment on attachment 128576 [details] [diff] [review]
patch v1 should do the trick, adds a confirmation similar to what when you close a window full of tabs

In comment 19, Jag disapproved of this patch, but did not minus it. Minus-ing
the bug would help people understand the status of this bug.

In comment 23, Jag said he did not have time for this bug. If that is still
true, it would be great to change the ownership of this bug to the default
owner or nobody.
Attachment #128576 - Flags: review?(jag)
Comment on attachment 128576 [details] [diff] [review]
patch v1 should do the trick, adds a confirmation similar to what when you close a window full of tabs

Minusing patch, obsoleting it (I doubt it still applies cleanly).
Attachment #128576 - Attachment is obsolete: true
Attachment #128576 - Flags: superreview?(jag)
Attachment #128576 - Flags: superreview-
Attachment #128576 - Flags: review?(jag)
Would someone apply if it would apply cleanly to the current source?
Anyone out there who is willing to port this patch to the new version?

OT: I personally moved to firefox and will not look back. Sorry, guys I am also
doing a lot of OS work ...but it's just hilarious how this bug is treated.
> Would someone apply if it would apply cleanly to the current source?
> Anyone out there who is willing to port this patch to the new version?

Even if it applies cleanly, it might have other issues and/or not get noticed by
reviewers (or they might not have time).
Please add a confirmation to 'Close Other Tabs'.

For a second time I have mistakenly slipped from 'Close Tab' to 'Close Other
Tabs', and lost all of the tabs I wanted to keep (some of which had
half-completed forms in them).

I really think a confirmation dialogue would fit in. One appears when you
attempt to close a browser window that has several tabs.

Please add a 'Close Other Tabs' confirmation dialogue, before I lose more
important browser sessions.
no patch. feature request. past the string freeze. not gonna block 1.7 for this.
Flags: blocking1.7? → blocking1.7-
May I simply suggest to reorder the menu options to :
'Close Tab' | 'New Tab' | 'Close Other Tabs' | 'Bookmark ...' | ...  (bug # 242664)
Its eventual fix should a trivial one and accidents are then less likely to happen.

I didn't see an open bug for Firefox (which shares this problem), but I agree
with the suggestion in #62. However, since Firefox (inexplicably) has a
different order to the right click options for each tab, I would suggest they be
ordered as:

New Tab | Reload Tab - Close Tab - Reload All Tabs | Close Other Tabs

The real key here is that "Close Tab" and "Close Other Tabs" NEED to have at
LEAST one menu item between them to prevent accidental clicks. This is also true
of "New Tab" and "Close Tab"... the above scenario is the only configuration I
can think of that would facilitate both.
I've located what appears to be the necessary instances to move around the order
of the right-click tab menu items. If I were to rearrange them in the order
mentioned, would anyone be willing to take a look at this? I'm talking about
Firefox mainly, but Seamonkey appears to be virtually the same as far as changes go.

Also, in relation to how the proposed re-ordering might not be seen as
intuitive... I concede that it may not be the most intuitive layout... but this
needs to be addressed. If reordering the items is the only way to "solve" this
problem in the time being, I say we go ahead with it. Having a menu option that
is directly next to another option, when the two have EXACT OPPOSITE
functionality, is just not a good idea.  Only other "fix" I can think of would
be to increase the gap currently between "Close Tab" and "Close Other Tabs",
thus moving "Close Tab" farther away from all the other options.
This code has been forked, so it has to be modified in
http://lxr.mozilla.org/seamonkey/source/toolkit/content/widgets/tabbrowser.xml
also (for Firefox) and
http://lxr.mozilla.org/seamonkey/source/toolkit/locale/widgets/tabbrowser.properties.


mozilla.org@adb.enki.net:
I believe your patch is in the same file as this would be fixed in
(http://lxr.mozilla.org/mozilla/source/xpfe/global/resources/content/bindings/tabbrowser.xml#60?).
Please combine your patch with the patch above and attach your patch using the
ordering and naming below. I think we should also give "Reload All Tabs"
confirmation dialog since that is a destructive activity too. You can just
follow the same methods as in the patch above. Also make sure you do the changes
also in the forked code within the toolkit directory I mentioned above.

Please replace:

+tabs.closeWarningTitle=Confirm other tab close
+tabs.closeWarning=You are about to close %S other Navigator tab(s). Are you
sure you want to continue?
+tabs.closeButton=Close all other tabs
+tabs.closeWarningPromptMe=Warn me when using close other tabs

With:

+tabs.closeWarningTitle=Confirm Closing Other Tabs
+tabs.closeWarning=You are about to close %S other tab(s). Are you sure you want
to continue?
+tabs.closeButton=Close other tabs
+tabs.closeWarningPromptMe=Warn me when I attempt to close other tabs



+----------------+
|New Tab         |
+----------------+
|Reload Tab      |
|Close Tab       |
+----------------+
|Close Other Tabs|
|Reload All Tabs |
+----------------+

Close tab should not be the first option, and the reason is that it is prone to
mouse slippage problems. Bug 191826's consistancy argument forgets that in
Gnome, Close is not at the bottom. I agree there is too much mouse move
necessary to get to "Close Tab". Doing it this way would keep them together (but
seperated still), make it so "Close other tabs" is not at the end of the menu
(people might get confused about it being in the same position as the old "Close
Tab" position), and Close Tab and "Reload Tab" are right next to each other and
both destructive operations.

I looked at bug 103354, and its renamed to '"close other tabs" is in a dangerous
position on tab context menu' and they marked it WONTFIX. We can fix it here and
then just mark bug 103354 fixed when it checks in.
Tim: I started writing my comment before yours came, and I got a mid-air
collision :-) . I'll reply to your comment now :-)

This should be fixed in the same patch for both Firefox and Seamonkey for
consistancy. See my comment #65 about the fork and where to find the Firefox and
Seamonkey code.

They cannot be seperated by more than a seperator as mentioned
http://bugzilla.mozilla.org/show_bug.cgi?id=103354#c131 unless you can get jag
to change his mind.

Notice that my ordering is similiar to yours but moves the least used items to
the bottom.

>  way to "solve" this problem in the time being

I don't think it can really be solved. It might be deterred by a reordering.
There is already a patch in this bug. It just needs to be dusted off. See my
comment #65 for information on what needs to be done. We'll nail them both in
one patch since they apply to the same file.
Hah... mid air collision.

I like the new ordering, except my vote would be for:

+----------------+
|New Tab         |
+----------------+
|Reload Tab      |
|Close Tab       |
+----------------+
|Reload All Tabs |
|Close Other Tabs|
+----------------+

I think it is very important that we do NOT put Close Tab and Close Other Tabs
right on top of one another. This allows for mouse slippage (and thus is the
same problem we have now). They are the complete opposite of one another, and as
such should not be put in a position where one can easily be clicked  went the
other was intended. Also, ordering it like this keeps the "Reload ___ - Close
____" group ordering in tact for both groups, which I think makes a little more
sense.

Thoughts? I'll try to take a look at this tonight.

Thanks.
One more thought...

How about this for the new ordering:

+----------------+
|New Tab         |
+----------------+
|Reload All Tabs |
|Close Other Tabs|
+----------------+
|Reload Tab      |
|Close Tab       |
+----------------+

Two advantages here:

1) It keeps "Close Tab" in the current spot (at least in Firefox), and therefore
takes care of any "learned behavior" problems that might otherwise arise.

2) The browser suite already has the "Reload" option at the bottom of the list,
so this wouldn't be as HUGE a change for them.

Though, looking at the layout now of the menu in the browser suite, it really
does baffle me how the ordering ended up this way... doesn't seem intuitive at
all. Firefox's layout, though it's missing the "Bookmark This Group Of Tabs"
option, seems MUCH more intuitive, even in it's current layout.

Just to be clear here, we are now talking about completely reordering the menu
for BOTH products?

Thanks again.
People - the recent discussion is off topic for this bug.  This bug is ONLY
about getting a confirmation dialog, NOT about the order of the menu items. 
Bugs that discussed the menu item order included bug 191826, bug 194341, and bug
215385.

If you want to debate the order of the menu items, please either reopen one of
those bugs (for Seamonkey or Firefox specifically) or create a new one for that
purpose.
I strongly disagree with both of the proposed revisions.  The "new tab"
button to the left works very nicely for doing that action.

Intuitively if I click on a particular tab, I am expecting to focus action on 
that particular tab, usually closing it.  I never use the other options except
for the hated accidental clicking of the "close other tabs" which still bites
me again and again.  ARRGH!

Please keep "close tab" at the top and move "close other tabs" as far away from
it as possible.  That's my 2 cents.  Thanks.
Jason:
> People - the recent discussion is off topic for this bug.

This is inaccurate. SOME of the recent discussion has been off topic. Much of
what I said pertained to fixing this particular bug, such as the mention about
forking, etc.

Tim:
The ordering discussion could be better off in another bug. Piggybacking that on
the a patch for this could open a pandora's box. I'm going to reopen bug 215385
and you can attach a patch there. 
Scratch that. There are too many bugs for reordering the context menu and let's
move the discussion to Mozillazine:
http://forums.mozillazine.org/viewtopic.php?p=512622#512622

See comment #65 for implementation details. Ignore things about reordering.
Whiteboard: See comment #72 - No context menu reordering discussions
This is being put on here for others to play with. Evidently some changes are
also required under the "toolkit" dir for the project overall, but for simply
the browser suite, this should be all that's needed.
Please mirror the changes to Firefox since we should kill two birds with one
stone. toolkit, browser, etc is used by Firefox. This will add those directories:
cvs co mozilla/browser mozilla/toolkit mozilla/chrome mozilla/mail

http://daihard.home.comcast.net/firefox/build.html

Although I'd use a .mozconfig file with the following:

mk_add_options MOZ_OBJDIR="@TOPSRCDIR@/../obj-firefox"
. $topsrcdir/browser/config/mozconfig
ac_add_options --disable-tests

If you use OBJDIRs for your Seamonkey and Firefox build, you can build them both
in the same tree.


set the MOZCONFIG environ variable to the location of that .mozconfig file
(don't overwrite your Seamonkey one)

Make sure if you have built Mozilla in the tree without an object directory
(object files will be inside the tree and the tree will contain a
mozilla/dist/bin, to clean it. I recommend using objdir for seamonkey too
(MOZ_OBJDIR="@TOPSRCDIR@/../obj-seamonkey")
I've gone ahead and made netdragon's suggested changes. I've compiled both the
suite and firefox on redhat 9, and the patch seems to work fine. Can anyone
test this via a windows build? I cannot.  I don't see how there could POSSIBLY
be any problems though.
Attachment #147783 - Attachment is obsolete: true
Attachment #147813 - Flags: review?(jag)
Submitted for review... we'll see.
If I recall correctly, they didn't want these confirmation dialogs in Firefox.
Since Firefox is a separate product I suggest you open a new bug on them and let
the owner decide whether to take the firefox part of the patch. I'll look at the
Mozilla-the-Suite part of it.
Per the request of jag, I've opened a new bug for this that is Firefox
specific... PLEASE... ANYONE who is concerned about this same issue with
Firefox, lend some help on the new bug. Be sure to at least vote for it so we
can hopefully get this taken care of in both products.

New bug for Firefox:

http://bugzilla.mozilla.org/show_bug.cgi?id=242844
Comment on attachment 147813 [details] [diff] [review]
Updated version of yesterday's patch, this one applies to both Firefox and the Suite.

>Index: xpfe/global/resources/content/bindings/tabbrowser.xml
>===================================================================

>+            if (numtabs > 1) {
>+              var promptPref = "browser.tabs.warnOnCloseOther";

Please use |const promptPref| here. Also, call it |closeOtherTabsPref| or
something a little more descriptive.

>+              var shouldPrompt = this.mPrefs.getBoolPref(promptPref);
>+              var reallyClose = true;
>+
>+              if (shouldPrompt) {
>+                var promptService =
>+                  Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
>+                    .getService(Components.interfaces.nsIPromptService);

This should be formatted to be in line with the rest of the file:

		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 warnOnClose = {value:true};

  var warnOnClose = { value: true };

Space is cheap ;-)

>+                var bundle = this.mStringBundle;
>+                var remTabs = numtabs - 1; //number of tabs to be removed
>+
>+                var buttonPressed = promptService.confirmEx(window, 
>+                  bundle.getString('tabs.closeWarningTitle'), 

...

Same here. Indent the "wrapped" argument lines to line up with the first
argument:

		var buttonPressed = promptService.confirmEx(window, 
							   
bundle.getString('tabs.closeWarningTitle'), 
							   
bundle.getFormattedString("tabs.closeWarning", [remTabs]),
							   
(promptService.BUTTON_TITLE_IS_STRING * promptService.BUTTON_POS_0 +
							    
promptService.BUTTON_TITLE_CANCEL * promptService.BUTTON_POS_1),
							   
bundle.getString('tabs.closeButton'),
							    null, null,
							   
bundle.getString('tabs.closeWarningPromptMe'),
							    warnOnClose);


I know this looks kinda ugly with the wrapping, but we have these "rules" where
you should stick to a file's code formatting style, and this seems to be the
style most used.

>+                var reallyClose = (buttonPressed == 0);

You've already declared |reallyClose| up above, just remove the |var| here.


>+              //don't set the pref unless they press OK and it's false
>+              if (reallyClose && !warnOnClose.value)
>+                this.mPrefs.setBoolPref(promptPref, false);

This should go inside the |if (shouldPrompt)| block, no need to test
warnOnClose.value unless it could've changed.

sr=jag with those changes, pending any r= comments.

Asking r= from Neil

>Index: toolkit/content/widgets/tabbrowser.xml
>Index: toolkit/locale/widgets/tabbrowser.properties

These two files will have to be dealt with under the Firefox product.
Attachment #147813 - Flags: superreview+
Attachment #147813 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #147813 - Flags: review?(jag)
Tim: Good work. The only way running it on Windows would be a problem would be
if Windows chokes on some flaw that Linux didn't choke on. I generally test on
both systems, and the most common thing is simply a compiling issue. Once it
builds, you are usually OK. I usually write XPCOM code, but you don't really
need to test on a lot of systems with XUL unless you are looking for visual
glitches. Since this is XUL code, then any major problems (besides incorrectly
arranged GUI) items will most likely be mirrored on all systems.

You probably will want to split up the patch between the Firefox code and
Seamonkey code since there are two bugs now. That'll get each part checked in
without having to wait for the other part to be approved. Basically, you can
just diff each set of files independently.

Is there ever a point that "Reload All Tabs" could cause dataloss? I just tested
it with Mozilla and Firefox and it looks like that when you hit reload, data is
put back into the forms. Therefore, it seems to me like we don't need a
confirmation on that like I formally thought.
All the Firefox stuff (not sure if I needed to include "all.js" in that patch
as well... I did) was moved into a seperate patch for the new bug. I've made
all the changes suggested, though I was a little confused on the last fix you
mentioned:

"This should go inside the |if (shouldPrompt)| block, no need to test
warnOnClose.value unless it could've changed."

Admittedly I don't have the most in depth understanding of what's going on with
all this (as what I'm doing is mostly porting the original patch to work with
the current tree). The change I made was to remove the somewhat redundant
"if(reallyClose && !warnOnClose.value)" and move the newly shortened
conditional "if (!warnOnClose.value")" into the already existing
"if(reallyClose)" block. Is this acceptable? Or is there more streamlining that
need be done?

Thanks in advance. I will set this patch to you again for review of the
changes. Hoepfully that is what you intended? If not my apologies.
Attachment #147813 - Attachment is obsolete: true
Attachment #147858 - Flags: review?(jag)
What jag is saying is this:

+                reallyClose = (buttonPressed == 0);

Put the if (reallyclose) thing after that because that's the only place it
changes. Otherwise, it'll be an extra instruction that has to be executed even
if the reallyClose couldn't change because shouldPrompt was false.
netdragon:  I see. I guess I just don't get the advantage of the seperate
"if(reallyClose && !warnOnClose.value)" conditional inside the shouldPrompt
block, after the "reallyClose = (buttonPressed == 0);" line.

I suppose I can put that in... but isn't the same thing accomplished by placing
just the "if (!warnOnClose.value)" conditional at the end of the
"if(reallyClose)" conditional block? You already have a seperate instance where
you're testing "if(reallyClose)", and by putting "if(!warnOnClose.value)" inside
that block, the code path is essentially "if(reallyClose &&
!warnOnClose.value)", which is what the original patch had.

Again, if jag wants it the other way, I'll be happy to make the change. Either
way, I saw one small indenting error that I'm going to go ahead and correct. If
you could give me some feedback on this I would really appreciate it, and maybe
I can do the indentation along with whatever you suggest in one fell swoop.
Also, would you have any idea who to request review from on the Firebird version
of this? I have the patch over there, but not sure of the next step.

Thanks in advance.
Well, I don't know about JS, but in C++, this conditional: "if(reallyClose &&
!warnOnClose.value)" would have the !warnOnClose.value test never happen if
reallyClose isn't true... That's called short-circuiting.

But I see the real issue jag is talking about now... You created warnOnClose
within the shouldPrompt block, but you use it outside the block where it wasn't
defined. warnOnClose therefore could never have been initialized, leading to
something like a "variable not initialized" error if shouldPrompt was false. You
want reallyClose OUTSIDE the shouldPrompt block because if they turned off the
pref, and you didn't prompt them, then it should close all the tabs. It wouldn't
do this if you stuck it within the shouldPrompt block because the reallyClose
part is where they are all closed.

Therefore, leave reallyClose where it is, and move the warnOnClose into the
shouldPrompt block. Like this:


+                 reallyClose = (buttonPressed == 0);
+                 //don't set the pref unless they press OK and it's false
+		  if (!warnOnClose.value)
+		    this.mPrefs.setBoolPref(closeOtherTabsPref, false);
+              }
+
+              if (reallyClose) {
+                if (aTab.localName != "tab")
+                  aTab = this.mCurrentTab;
+                else
+                  this.mTabContainer.selectedItem = aTab;
+
+                var childNodes = this.mTabContainer.childNodes;
+
+                for (var i = childNodes.length - 1; i >= 0; --i) {
+                  if (childNodes[i] != aTab)
+                    this.removeTab(childNodes[i]);
+                }
+             }
In answer to your other question about the Firefox bug, I suggest get this done
on Seamonkey, then you can basically just use the same code that was checked in
here for the Firefox patch. As it hasn't been fully approved yet, you wouldn't
want to have to constantly change both patches in parallel. Besides, there might
be a little politics (I hate politics) we have to deal with on the Firefox side
since they are against confirmation dialogs or something.

I guess it was thought that people don't usually pay attention to confirmation
dialogs. I wouldn't agree with this in some cases, but not others. I pay
attention to the important ones, and I'd use this feature so little I'd want
that confirmation dialog there for my protection.
I'll set this again for review. Fingers crossed everyone.
Attachment #147858 - Attachment is obsolete: true
Attachment #147858 - Flags: review?(jag)
Attachment #147924 - Flags: review?(jag)
Comment on attachment 147924 [details] [diff] [review]
Final version (taking into account jag and dragon's changes)

netdragon is right. I thought I had explained why I wanted the conditional
setBoolPref inside the shouldPrompt block, I guess I removed a little too from
my comments.

>+              var shouldPrompt = this.mPrefs.getBoolPref(closeOtherTabsPref);
>+              var reallyClose = true;
>+
>+              if (shouldPrompt) {
>+                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 warnOnClose = { value:true };

.....

>+                reallyClose = (buttonPressed == 0);
>+                //don't set the pref unless they press OK and it's false
>+                if (!warnOnClose.value)
>+                  this.mPrefs.setBoolPref(closeOtherTabsPref, false);
>+              }

I think you forgot to add back the reallyClose check. If you hit cancel
(buttonPressed == 1 or 2), this bool pref shouldn't get updated.

sr=jag with that change made.
Attachment #147924 - Flags: review?(jag) → superreview+
Final version (for real this time ;))

Marking for super review.
Attachment #147924 - Attachment is obsolete: true
Attachment #147965 - Flags: superreview?(jag)
Comment on attachment 147965 [details] [diff] [review]
Added missing conditional argument per jag

Hmmm, missed this the other times around, but numtabs should really be numTabs.
No need for a new patch, whoever checks this in just needs to change that real
quick before checking in.

sr=jag
Attachment #147965 - Flags: superreview?(jag) → superreview+
jag and netdragon... a problem just occured to me.

If this gets checked in now, that means that it would be on the 1.8a release
right? Not the 1.7 final? The problem here is that, if this doesn't get checked
into the 1.7 branch, then that means that both Firefox 0.9 and 1.0 won't have
this fix available (since they would rely on the all.js in 1.8a, assuming that's
where this would go were it checked in soon), and there still is no ETA for a
1.1 Firefox, and whether it would even be based on 1.8 has yet to be announced.

Since rc2 of 1.7 still hasn't been released, could we PLEASE try to get this put
in both the branch and trunk? This is a BIG dataloss issue for a lot of people,
and it's going on 10 months since the original patch was submitted. Personally,
I use Firefox, but I fixed the original patch for Mozilla Suite too since I knew
that it would take this getting fixed first before the Firefox guys would even
consider adding it to theirs.

As you can see from the patch, there is no risk of breaking anything with this
patch. I've been using my build with it on Linux for the past 3-4 days with no
consequence. I'm temporarily setting this to possible 1.7 bloker to hopefully
get some notice.

ANY help getting this in to 1.7 would be GREATLY APPRECIATED.

Thanks in advance.
Flags: blocking1.8a?
Flags: blocking1.7?
Flags: blocking1.7-
Tim: the fastest way to get this on the branch is to first get it on the trunk.
 The patch still needs review.
Flags: blocking1.8a?
Can anyone with check-in ability please help me out with this? I'm trying to get
this onto the trunk ASAP with the hope that it might get picked up for the 1.7
Branch.

Note to whoever checks-in, please correct the three instances of "newtab" to be
"newTab" as specified by jag. I would submit a fixed patch, but I imagine that
would require another review round.

Any help would be greatly appreciated.
Attachment #147813 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #147965 - Flags: review?(neil.parkwaycc.co.uk)
(In reply to comment #92)
> Note to whoever checks-in, please correct the three instances of "newtab" to be

You still need to get a r+.

> "newTab" as specified by jag. I would submit a fixed patch, but I imagine that
> would require another review round.

newTab/numTabs !?
You are not required to provide a new patch;
but it can't hurt while we have time;
no new r/sr will be needed if you only change thoses.
Sorry jag, yeah, I meant "numTabs"... little under the weather and still tired
at the time of writing.

Anyhow, that change to numTabs has been made here. Hopefully this can get
looked at soon.  Thanks again.
Attachment #147965 - Attachment is obsolete: true
Attachment #147965 - Attachment is obsolete: false
Attachment #147965 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #148198 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 148198 [details] [diff] [review]
Final patch, with numtabs changed to numTabs

sr=jag
Attachment #148198 - Flags: superreview+
Comment on attachment 148198 [details] [diff] [review]
Final patch, with numtabs changed to numTabs

>@@ -850,15 +850,51 @@
Been editing this patch? There are only 50 lines from the new version.

>+            var numTabs = this.mTabContainer.childNodes.length;
>+
>+            if (numTabs > 1) {
I'm not convinced that this is necessary... Close Other Tabs should be disabled
and/or hidden if only one tab is open. In fact I would prefer this check to be
> 2 because I don't like the string "1 tab(s)" :-P

>+                var remTabs = numTabs - 1; //number of tabs to be removed
I would have called this tabsToClose or something more meaningful.
This is just like the last one, except I've changed remTabs to tabsToClose as
was suggested and created the patch the correct way via "cvs -u diff", sorry
about that.

As for the other argument of removing the "Close Other Tabs" option completely
when there is only one tab remaining... I'm not sure I'm following. Both the
suite and Firefox already grey out that option when there is only a single tab
making it unselectable. If the suggestion was to completely remove the option
(but by that rationale, shouldn't Reload All Tabs also be removed at that
point) when there is only one tab instead of greying it out, I just think that
perhaps that is a bit beyond the scope of fixing this bug with a popup.

Also, as for not showing the popup when there are only two tabs, ie -
increasing  the line "if (numTabs > 1) {" to "> 2", I don't think I follow that
either. The point of this was to prevent accidental closing of unintended
windows by clicking on "Close Other Tabs" by mistake. If you have two tabs
open, it is still entirely possible to close the wrong tab by doing this.
Whether you're only closing one tab accidently or 30 tabs accidently, depending
on what was in those other tabs, the damage can be just as bad.

Any thoughts?
Attachment #148218 - Flags: superreview?(neil.parkwaycc.co.uk)
Neil:

I generally don't like achilles heels, and if their is a regression causing it
not to be greyed out during some UI overhaul, its nice to have an additional
check so we don't ask them for no reason, which might confuse them. I don't see
why two checks hurt, its only a couple more lines of code.

wrt the tab(s) part, I agree that this is perhaps something that could be done
better, though its getting pretty picky. :-)

We can't change (numTabs > 1) for the reason Tim said. 

The best way might be to do something like this:

tabs.closeWarning=You are about to close %S other tab%S. Are you sure you want
to continue?

and something like:

var plurality = "";
if (tabsToClose > 1)
   plurality = "s";
bundle.getFormattedString("tabs.closeWarning", [tabsToClose], plurality)


If its 1,  you'll see "1 other tab", otherwise you'll see "n other tabs"
(In reply to comment #98)
> The best way might be to do something like this:
> 
> tabs.closeWarning=You are about to close %S other tab%S. Are you sure you want
> to continue?

That is not localizable. Pluralizing "tab" in other languages may involve
pluralizing multiple words (e.g. "other").

As icky as "1 other tab(s)" looks, I think it's the best we can do right now. I
do think we should warn for the "1" case, since this was about accidentily
selecting "close other tabs" when you wanted "close this tab".

Yes, the numTabs test is redundant, we could remove it, but it doesn't hurt to
be there and it makes it clear in the code that we'll only show the warning when
two or more tabs are open. Though, a comment asserting this function will only
get called in that case would be fine too.

Neil, it's your call.
Comment on attachment 148218 [details] [diff] [review]
Final patch again, with one more variable rename (not edited directly this time... apologies... I don't work via patches too often)

Thanks for the comments, this is fine now.
Attachment #148218 - Flags: review+
Attachment #148218 - Flags: superreview?(neil.parkwaycc.co.uk) → approval1.7?
Flags: blocking1.8a?
Flags: blocking1.8a-
Flags: blocking1.7?
Flags: blocking1.7-
I'm hoping this gets checked in soon, but if not, I don't think that at least
the blocking1.8a flag should have been removed.

This is a DATALOSS issue that has been open for over a year.

Please reevaluate.
Comment on attachment 148198 [details] [diff] [review]
Final patch, with numtabs changed to numTabs

Removing obsolete r= request.
Attachment #148198 - Flags: review?(neil.parkwaycc.co.uk)
Fix checked in to the trunk.
Status: NEW → RESOLVED
Closed: 22 years ago → 20 years ago
Resolution: --- → FIXED
Attachment #148218 - Flags: approval1.7?
Attachment #147965 - Attachment is obsolete: true
Attachment #148198 - Attachment is obsolete: true
Target Milestone: --- → mozilla1.8alpha
Well, I couldn't work out which e-mail address belonged to the person who added
the fix, nor whether it was one or many people who added this fix, so I'll post
my message here: thank you. Unlike democracy, Bugzilla and the Mozilla team seem
to work. I just got the overnight build and close other tabs now seems to behave
cautiously, with the much-requested warning dialogue. Thank you.
Right, its a meritocracy with a bit of politics :-)

I tested the latest trunk build thoroughly on Linux, and it works with no
problems. I even tested disabling it, and then re-enabling it through
about:config. Good work Tim. If there are any problems in the future, feel free
to reopen, but I doubt there will be any problems.

VERIFIED FIXED
Status: RESOLVED → VERIFIED
Bob:  Tim Meader provided the new fix with modifications of an original patch
submitted by andrews (comment #12)
I think this has a VERY good reason to be checked in. Not getting this into the
1.7 branch heavily precludes it from getting into Firefox 1.0 easily. The
analogous bug for this in Firefox has already been marked 1.0+, so I would think
it would make sense to apply this before 1.7 is released. It's been in heavy use
by me personally, as well as netdragon from what I can tell. I have come across
zero issues stemming from this patch. And merely looking at the code therein, I
can't imagine anything that could adversely affect anything at all. At WORST the
patch simply wouldn't pop up a dialog, but it does do that, it works fine.
Flags: blocking1.7- → blocking1.7?
Tim: Only drivers can change a blocking status from an agreed upon resolution. 
Asa already set it to 1.7- and it cannot be touched by anybody else.  Resetting
to official decision.  Please don't touch it again.
Flags: blocking1.7? → blocking1.7-
Apologies, I was not aware that the "-" flag was not resettable. However, I'd
spoken via email with Asa previously, and he mentioned that the first step in
possibly getting this checked into the 1.7 branch (for the purpose of easier
inclusion in Firefox 0.9 and 1.0) would be to get it checked into the trunk.
That has been accomplished. I emailed Asa early Friday asking what next step I
might take, and received no response, hence I did the next logical thing.

Can anyone tell me what other step I would take then BESIDES setting 1.7?   ?
To be honest, I'm not sure what the protocol is at this point.  I think it's
simply up to somebody from drivers to become aware of the issue and make the
change (if they see fit).  Unfortunately, other than Asa I don't know off the
top of my head who the other drivers members are - and don't have time, at work
here, to hunt down that information...
blocking1.7- doesn't mean that a patch can`t get checked into the 1.7 branch.
blocking means that a 1.7 can't be released without this "bugfix" and it's
correct that this is not blocking a 1.7 Release.

If you want to nominate a patch for the branch, you must request a 1.7 aproval
for the patch itself. It's possible that this is already to late because of the
l10n impact and I think that we have already a l10n freeze for 1.7.

BTW: Never touch flags if you don't know exaclty what they mean !
Just send an email to drivers@mozilla.org with the URL of the bug and basically
what you said in comment #107
This isn't necessary for 1.7. Firefox doesn't need this to be in 1.7 to fix it
for Firefox 1.0. SeaMonkey users can get this with 1.8 when it comes out.
Thank you to everyone that replied. I have a much better understanding of the
protocol now. I'm not intentionally trying to step on anyone's toes here... for
that I'm sorry, just trying to help out the other users who've been running
across the same problem.

Asa's last post explained everything that I was wondering.
*** Bug 261669 has been marked as a duplicate of this bug. ***
*** Bug 298035 has been marked as a duplicate of this bug. ***
Product: Core → SeaMonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: