Last Comment Bug 736738 - Make it easier for applications and extensions to overlay the customize toolbar window.
: Make it easier for applications and extensions to overlay the customize toolb...
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: Toolbars and Toolbar Customization (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Philip Chee
:
: :Gijs Kruitbosch
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-17 09:08 PDT by Philip Chee
Modified: 2012-03-30 13:08 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1.0 Proposed Fix. (3.42 KB, patch)
2012-03-17 09:14 PDT, Philip Chee
gavin.sharp: review+
Details | Diff | Splinter Review
Patch v1.1 fix nits. r=gavin (5.13 KB, patch)
2012-03-29 08:47 PDT, Philip Chee
philip.chee: review+
Details | Diff | Splinter Review

Description Philip Chee 2012-03-17 09:08:29 PDT
Thunderbird overlays the customizeToolbar window then uses some ugly javascript to shuffle the UI around. This patch adds some IDs to various elements so that Thunderbird can then use static XUL markup in the overlay to position their UI.

Secondly Thunderbird and Lightning have what they call "inline toolbars/toolboxes". The UX would really suck if users were allowed to add custom toolbars so Thunderbird uses some javascript to hide the Add New Toolbar button and other bits of the UI. They could stop users from adding custom toolbars by not having a toolbarset. However the customize code in a couple of places implicitly depends on the toolbarset existing. We can help Thunderbird and Lighting hide the Add New Toolbar button automatically if the toolbox doesn't have a toolbarset.
Comment 1 Philip Chee 2012-03-17 09:14:56 PDT
Created attachment 606875 [details] [diff] [review]
Patch v1.0 Proposed Fix.
Comment 2 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-03-28 10:08:03 PDT
Comment on attachment 606875 [details] [diff] [review]
Patch v1.0 Proposed Fix.

You should probably also add a sanity check for gToolbox.toolbarset in the persistCurrentSets's forEachCustomizableToolbar loop, just in case someone somehow manages to have toolbars with "customindex" set, but no toolbarset.

Making toolbox.xml's appendCustomToolbar fail a little more gracefully when this.toolbarset is null would also be good.
Comment 3 Philip Chee 2012-03-29 08:47:34 PDT
Created attachment 610561 [details] [diff] [review]
Patch v1.1 fix nits. r=gavin

Carrying forward r=gavin

> You should probably also add a sanity check for gToolbox.toolbarset in the
> persistCurrentSets's forEachCustomizableToolbar loop, just in case someone
> somehow manages to have toolbars with "customindex" set, but no toolbarset.
Fixed on checkin.

> Making toolbox.xml's appendCustomToolbar fail a little more gracefully when
> this.toolbarset is null would also be good.
Fixed on checkin.

Pushed to inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/a9dfce846606
Comment 4 Ed Morley [:emorley] 2012-03-30 13:08:00 PDT
https://hg.mozilla.org/mozilla-central/rev/a9dfce846606

Note You need to log in before you can comment on or make changes to this bug.