394873 - Customize Toolbar window disappears when add/removing toolbar button

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Omi Azad]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a7) Gecko/2007080209 GranParadiso/3.0a7
Build Identifier: version 3.0a1pre (2007090303)

When you want to customize your toolbar by add/remove button, the Customize Toolbar window forgets it's position and goes behind the main mail window. If the user don't notice where the button customization window is, then it'll be hard to find the window.

Reproducible: Always

Steps to Reproduce:
1. Go to View -> Toolbars -> Customize
2. Try dragging a button from Customize Toolbar window to the toolbar (for adding the button) or the reverse way, I mean drag a button from the toolbar to the Customize Window (for removing the button).
Actual Results:  
The Customize Toolbar window disappears by going behind the main window.

Expected Results:  
The window should not forget it's position and stay on top of the main window.

Comment 1

[Phil Ringnalda (:philor)]

Really a duplicate of bug 385714, but I have the feeling it may prove handy to have a Tbird bug about "we need something that works" as a fallback from that "this particular thing doesn't work."

Comment 2

[Scott MacGregor]

Phil, should we using a fake sheet like Firefox here?

Comment 3

[Phil Ringnalda (:philor)]

"Maybe." The fake sheet currently has some brutal Cocoa-problems in Fx (bug 361394, bug 383740 that's really a dupe, at least one or two more similar dupes in various Fx components), but since it's fairly rare for us to put it over content, and even rarer to put it over content that gets to redraw, we might not lose anything even if we do port Fx's current solution before they have to switch it to something else.

Comment 4

[Phil Ringnalda (:philor)]

And I'd forgotten about bug 350282 (must be inline to not have chrome draw over it, like, say, getting a new email) versus bug 351363 (must be a separate file to masquerade as customizeToolbar.xul so extension overlays work). Offhand, I'm not sure how to reconcile those two, short of collapsing the whole tabmail, filling the window with a background image of a paint-splattered dropcloth, and putting the sheet over that.

Comment 5

[Phil Ringnalda (:philor)]

Or, perhaps bug 395122 + bug 395123 :)

Comment 6

[Phil Ringnalda (:philor)]

Attachment: WIP

Spent too long chasing a couple of stupid, but well-hidden, typos, and now I've forgotten what still needs to be done with this, but some staring will probably remind me.

Comment 7

[Phil Ringnalda (:philor)]

Attachment: Fix v.1

Oh, that's what had to go: the "add a toolbar" button that I still need to wire up.

But other than that, I think that we want to take this now, even with interesting behavior like bug 395334 (trying to combine an editor, Venkman, and a sheet that insists on being on top of everything else is "fun" but in general, if you don't want the sheet over another app, close it, it'll be fixed soon enough) - it has the outstanding benefit of not enraging me the way the dialog currently does, and going forward has a much better chance of both looking and working right.

I'm not terribly happy with where I hid the CSS, but I didn't find a better candidate spot.

Comment 8

[Scott MacGregor]

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

this looks great Phil! Question, should the implementations of getMailToolbox be ifdef'ed as well since they are only called from ifdefed code?

Comment 9

[Phil Ringnalda (:philor)]

It's only called from ifdefed code because I'm one (or more) of a) forgetful, b) lazy, c) trying to increase my fixed bug count by needing trivial followups for every bug.

The inspiration for it (and one of my bugs along the way, since I started out reusing the same name function name, though MsgPrintEngine returns something different, and order of inclusion varies) was the getNavToolbox() that I scattered around the tree as a semi-API for print preview, which turned out to be doubly useful for browser.js, since it was already doing document.getElementById("nav-toolbox") half a dozen times. We're not quite so extreme (yet), but there's at least one other potential caller in every file, since setting up MailToolboxCustomizeDone always calls for getting the toolbox element.

Comment 10

[Phil Ringnalda (:philor)]

I really need more sleep, or more people who will call me on my foolishness cc'ed. Calling getMailToolbox() is only a benefit if it's cached from the first call, and if the first call is the only call for most of our users, it's no benefit at all.

However, having it not ifdefed is a good thing, since otherwise someone developing extensions on Linux and testing on Windows won't know that their hypothetical getMailToolbox() function in an overlay will clobber the one that's only visible on OS X.

mail/base/Makefile.in 1.12
mail/base/jar.mn 1.103
mail/components/addrbook/Makefile.in 1.6
mail/components/compose/Makefile.in 1.7
mail/base/content/messenger.xul 1.83
mail/base/content/messageWindow.xul 1.35
mail/base/content/mailCore.js 1.35
mail/base/content/customizeToolbarSheet.js 1.1
mail/base/content/customizeToolbarSheet.xul 1.1
mail/base/content/customizeToolbar.js 1.13
mail/base/content/customizeToolbar.xul 1.13
mail/base/content/msgMail3PaneWindow.js 1.136
mail/base/content/messageWindow.js 1.54
mail/components/addrbook/content/addressbook.xul 1.72
mail/components/addrbook/content/addressbook.js 1.37
mail/components/compose/content/messengercompose.xul 1.107
mail/components/compose/content/MsgComposeCommands.js 1.118
mail/themes/pinstripe/mail/primaryToolbar.css 1.12