288079 - Cancel customize toolbar loses buttons

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Federico Tello Gentile]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.6) Gecko/20050223 Firefox/1.0.1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.6) Gecko/20050223 Firefox/1.0.1

When there are buttons in the menu bar, specifically to the left of the File
menu, selecting cancel in the customize toolbar dialog makes those bttons dissapear.

Reproducible: Always

Steps to Reproduce:
1. Customize the toolbar adding some buttons from the button bar in the menu
bar, next to the File menu. Optionally, some other buttons can be added to the
right of the Help menu.
2. Press OK so the customized toolbar is set.
3. Select Customize again, do nothing and select cancel

Actual Results:  
All buttons in the menubar are lost and the customize dialog does not close.

Expected Results:  
The same as when customizing without placing buttons in the menu bar, next to
the File menu.

This does not happen when I only add buttons to the right of the Help Menu.
Firefox does not have this problem, because the is not a Cancel button there.
I tested this in the 20050327 nightly build and it is still present.
It can be frustating to new users losing some buttons.

Comment 1

[Mike Cowperthwaite]

I'm not seeing this problem with TB 1.0+0325, Win2K.  Did you test this under 
Windows, or have you only seen this under Linux?

Comment 2

[Federico Tello Gentile]

Tested with the 27/03/2005 nightly on Windows XP with SP2, on a clean profile.
Can you see it on the 1.0 version? Be careful to add the buttons in the menu bar
no the toolbar.

Comment 3

[Mike Cowperthwaite]

(In reply to comment #2)
> Be careful to add the buttons in the menu bar no the toolbar.

Whoops, I missed that.  Yes, I see this; 1.0 and 1.0+0403, Win2K.

Additional symptom: when you click Cancel, and the buttons disappear, the 
Customize dialog remains visible -- and won't close with additional clicks on 
Cancel or on the CloseWindow widget.  You have to click OK to dismiss the 
dialog.

Comment 4

[Greg Thompson]

 tried to locate routine options for support & got tossed into this random email
list. ( Now I am completely confused ).  I just downloaded the latest version of
FireFox Browser onto Windows XP ...& `All I want to know is, 
 
             "what happened to my Toolbar buttons??"  
 
Before, I could R.Clik in the toolbar area & select (Autofill), to refresh/bring-up the active button/icon. Now there is a check mark, on the drop-down menu -[next to] "Autofill". It 'is' checked, but no icon/button is available `as before. Plus all standard buttons, ie: [Print] were gone. (I was able to go into CUSTOMIZE options & drag them to useable location, ...but still no Autofill).  Perhaps something was corrupted in the download & I need to remove the latest version installed?, ...or try "GoBack" on computer??                       `What say you ~ to this frustration??   /  Greg T

Comment 5

[Phil Ringnalda (:philor)]

Attachment: Fix v.1

So, what's happening is that the toolbar binding's setter for currentSet knows about the possibility of getting a wrapped firstPermanentChild, so when it's removing non-permanent children it looks at firstChild.firstChild for a toolbarpaletteitem, but it then calls insertItem with aBeforePermanent set to true, and insertItem doesn't know that it might need to insertBefore the wrapper for this.firstPermanentChild instead of this.firstPermanentChild itself.

That's a bug, possibly bug 350588 or possibly another one I'll file, but not a branch-fixable one, exactly, I don't think. However, unwrapping before we start setting currentSet back to previousset should be safe enough, and gets around the problem.

Comment 6

[Scott MacGregor]

Comment on attachment 244790 [details] [diff] [review]
Fix v.1

This seems like the right way to go for the branch. Thanks Phil.

Comment 7

[Phil Ringnalda (:philor)]

Trunk: mail/base/content/customizeToolbar.js 1.8
Branch: mail/base/content/customizeToolbar.js 1.7.4.1

Comment 8

[Mike Cowperthwaite]

V fixed with 2a1-1011, Win2K.  Thanks, Phil.

I notice that now, Cancel doesn't really seem to do anything different from OK;
followup in bug 317424.

Comment 9

[Phil Ringnalda (:philor)]

Note to self: a tiny bit more testing would be in order.

With this fix, it doesn't lose buttons, because it doesn't remove anything, because unwrapToolbarItems() removes the previousset attribute that onCancel was planning to use to set thing back to how they were.

Comment 10

[Phil Ringnalda (:philor)]

Attachment: Test protocol

You know you've gotten sufficiently anal about testing when your list of tests is too long to include in a comment.

Comment 11

[Phil Ringnalda (:philor)]

Attachment: Fix v.2

Several things:

- Undo my screwup, with an argument to unwrapToolbarItems to say whether or not to remove the previousset attribute, so unwrapToolbarItems can be both cleanupBeforeClosing and workaroundToolbarBindingBugs

- Remove the window.close in onUnload - not only does it appear to be the cause of bug 346696, it's scary: according to the docs, onunload is supposed to be called after the window goes away, so if it does anything, it would need to be something random and bad

- Remove rebuilding the palette and wrapping the toolbars in onCancel: we're about to close, so we don't need those "get ready to redisplay" things that came over from restoreDefaultSet

- Teach unwrapToolbarItems to be sure it got a firstChild from what it thought would be a wrapped item, so we can safely call it in onUnload even when onCancel didn't rewrap

- Unwrap before running through restoreDefaultSet, to avoid triggering the same binding troubles as in onCancel (done just wrong, you can wind up with it "restoring" the menubar to {spring}{throbber}{menuitems})

Two things I found in testing:

- restoreDefaultSet() asserts in the buildPalette call, about "not initialized: 'mRoot', file /home/philor/moz/mozilla/content/xul/templates/src/nsXULTreeBuilder.cpp, line 762," which appears to be the result of the Folder Location and Mail Views items, since it also asserts when you drag enough other items to the toolbar to make them change rows. I'll keep poking at them, though it seems relatively harmless.

- The only part of my tests that doesn't do what I expect is "restore default set" then "cancel," which I'd expect to cancel the restoring, rather than cancel anything done after the restoring. However, it's apparently been like that all along, without anyone filing on it that I can see.

Comment 12

[Scott MacGregor]

Comment on attachment 245541 [details] [diff] [review]
Fix v.2

Thanks for the detailed description Phil. I'm not too worried about the restore/cancel case as it's always been that way.

Comment 13

[Phil Ringnalda (:philor)]

Trunk: mail/base/content/customizeToolbar.js 1.10
Branch: mail/base/content/customizeToolbar.js 1.7.4.3