Cancel customize toolbar loses buttons

RESOLVED FIXED in Thunderbird2.0

Status

Thunderbird
Mail Window Front End
RESOLVED FIXED
14 years ago
10 years ago

People

(Reporter: Federico Tello Gentile, Assigned: philor)

Tracking

({fixed1.8.1})

Trunk
Thunderbird2.0
x86
All
fixed1.8.1

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

632 bytes, text/plain
Details
3.20 KB, patch
Scott MacGregor
: review+
Scott MacGregor
: approval-thunderbird2+
Details | Diff | Splinter Review
(Reporter)

Description

14 years ago
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.
(Reporter)

Updated

14 years ago
Summary: Cancel customize toolbar looses buttons → Cancel customize toolbar loses buttons
(Reporter)

Updated

14 years ago
Version: unspecified → Trunk

Comment 1

14 years ago
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?
(Reporter)

Comment 2

14 years ago
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

14 years ago
(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.
Severity: normal → minor
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 4

12 years ago
 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
(Assignee)

Comment 5

12 years ago
Created attachment 244790 [details] [diff] [review]
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.
Assignee: mscott → philringnalda
Status: NEW → ASSIGNED
Attachment #244790 - Flags: review?
(Assignee)

Updated

12 years ago
Attachment #244790 - Flags: review? → review?(mscott)

Comment 6

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

This seems like the right way to go for the branch. Thanks Phil.
Attachment #244790 - Flags: review?(mscott)
Attachment #244790 - Flags: review+
Attachment #244790 - Flags: approval-thunderbird2+
(Assignee)

Comment 7

12 years ago
Trunk: mail/base/content/customizeToolbar.js 1.8
Branch: mail/base/content/customizeToolbar.js 1.7.4.1
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED

Comment 8

12 years ago
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.
(Assignee)

Comment 9

12 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

12 years ago
Status: REOPENED → ASSIGNED
(Assignee)

Comment 10

12 years ago
Created attachment 245538 [details]
Test protocol

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

Comment 11

12 years ago
Created attachment 245541 [details] [diff] [review]
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.
Attachment #244790 - Attachment is obsolete: true
Attachment #245541 - Flags: review?(mscott)
Attachment #245541 - Flags: approval-thunderbird2?
(Assignee)

Updated

12 years ago
Severity: minor → normal
Keywords: fixed1.8.1
Target Milestone: --- → Thunderbird2.0

Comment 12

12 years ago
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.
Attachment #245541 - Flags: review?(mscott)
Attachment #245541 - Flags: review+
Attachment #245541 - Flags: approval-thunderbird2?
Attachment #245541 - Flags: approval-thunderbird2+
(Assignee)

Comment 13

12 years ago
Trunk: mail/base/content/customizeToolbar.js 1.10
Branch: mail/base/content/customizeToolbar.js 1.7.4.3
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago12 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
(Assignee)

Updated

12 years ago
Blocks: 346696
(Assignee)

Updated

10 years ago
Duplicate of this bug: 318997
You need to log in before you can comment on or make changes to this bug.