407725 - Toolbar customisation pointlessly removes, clones and adopts nodes

Status: RESOLVED FIXED

(Toolkit :: Toolbars and Toolbar Customization, defect)

Description

[[e-mail]]

* The toolbarpalette is invisible, but the toolbox removes it anyway.
  (This means that customised nodes may not exist in the document.)
* The toolbox uses clones of the palette items when it could just move then.
* createWrapper sometimes uses the wrong document.
* wrapPaletteItem should be passed an imported node to avoid adopting it.
* wrapToolbarItem should insert the wrapper as well as removing the node.

Comment 1

[[e-mail]]

Attachment: WIP

This seems to work, at least on Windows. Of course I'm somewhat limited because I'm using attachment 292316 [details] [diff] [review] but I've removed all the null-checks to be sure ;-)

Apparently toolbar customisation works differently on the Mac?

Comment 2

[Robert Kaiser]

Attachment: WIP, unbitrotted

This is the same patch unbitrotted to my best knowledge. Note that with my first tries on testing this, I saw some problems in Minefield, but when I wanted to start debugging this, it suddenly started working as expected.
Only when I "Restore Default Set" from the customization palette, I get empty items in the palette and an exception on closing the palette, leading to menubar and the "customize" entry staying disabled (not the other toolbars though):
Error: uncaught exception: [Exception... "Component returned failure code: 0x80004003 (NS_ERROR_INVALID_POINTER) [nsIDOMXULElement.replaceChild]"  nsresult: "0x80004003 (NS_ERROR_INVALID_POINTER)"  location: "JS frame :: chrome://global/content/customizeToolbar.js :: unwrapToolbarItems :: line 183"  data: no]

This might be caused by the restore counting on the old mechanics, though, I'm not sure.

Comment 3

[[e-mail]]

Attachment: Fix for restore default set

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 302343 [details] [diff] [review]
Fix for restore default set

I don't understand:
-    var title = stringBundle.getString(aItem.id + "Title");
+    var title = stringBundle.getString(aItem.localName.slice(7) + "Title");

And I think:
     // Attempt to locate an item with a matching id within palette.
-    var paletteItem = this.parentNode.palette.firstChild;
-    while (paletteItem) {
-    var paletteId = paletteItem.id;
-    if (paletteId == aId) {
-      newItem = paletteItem.cloneNode(true);
-      break;
-    }
-    paletteItem = paletteItem.nextSibling;
-    }
+    newItem = document.getElementById(aId);

will break firefox, because we have children of elements in the palette that have the same IDs as elements that used to be children of the palette, and we are specifically relying on the fact that that code only checks children.

I'm also a bit concerned that keeping the palette in the document will have unintended consequences.

r- because of the second issue, but that should be easily fixable. I haven't had a chance to test this yet.

Comment 5

[Robert Kaiser]

Gavin, is it feasible to still try fixing this in toolkit, or do you think it's not the right thing to do? If it's worth going this way, is it something that can be considered for 1.9.1?

Comment 6

[:Gavin Sharp [email: [e-mail]]]

I think it's definitely the right thing to do, we just need to make sure it doesn't cause bustage for Firefox or other consumers. We could certainly do it for 1.9.1 - with my comments in comment 4 addressed I think it might be good to go?

Comment 7

[[e-mail]]

(In reply to comment #4)
>(From update of attachment 302343 [details] [diff] [review])
>I don't understand:
>>-    var title = stringBundle.getString(aItem.id + "Title");
>>+    var title = stringBundle.getString(aItem.localName.slice(7) + "Title");
Actually I don't understand how the old code worked, unless somehow the cloned spacer/separator/springs always had the unique id rather than the random one.

> And I think:
>>     // Attempt to locate an item with a matching id within palette.
>>-    var paletteItem = this.parentNode.palette.firstChild;
>>-    while (paletteItem) {
>>-    var paletteId = paletteItem.id;
>>-    if (paletteId == aId) {
>>-      newItem = paletteItem.cloneNode(true);
>>-      break;
>>-    }
>>-    paletteItem = paletteItem.nextSibling;
>>-    }
>>+    newItem = document.getElementById(aId);
>will break firefox, because we have children of elements in the palette that
>have the same IDs as elements that used to be children of the palette, and we
>are specifically relying on the fact that that code only checks children.
Ah, but I'm not cloning them any more, so you'll only ever have the one node (per document, that is - the customise dialog still has its own clones).

Comment 8

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #7)
> >will break firefox, because we have children of elements in the palette that
> >have the same IDs as elements that used to be children of the palette, and we
> >are specifically relying on the fact that that code only checks children.

> Ah, but I'm not cloning them any more, so you'll only ever have the one node
> (per document, that is - the customise dialog still has its own clones).

I don't see how that's relevant - the problem is not that there are multiple nodes with the same ID, the problem is that one of the toolbar's children has elements whose IDs are the IDs of a previous version's top-level elements. For example, in Firefox 2 we had "back-button" and "forward-button" elements that could be customized, but in Firefox 3 we have only a "unified-back-forward-button", which has child elements with IDs "back-button" and "forward-button".

On upgrade, the code will look for the previous version's elements (since that's what's stored in localStore.rdf), and we're currently relying on the fact that it won't find them and just fail to add them to the toolbar. With this change, as far as I can tell we'll rip out the child element from it's parent and insert it into the toolbar.

That code will also find elements that are outside of the palette, which might be unexpected by current users.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #7)
> (In reply to comment #4)
> >(From update of attachment 302343 [details] [diff] [review])
> >I don't understand:
> >>-    var title = stringBundle.getString(aItem.id + "Title");
> >>+    var title = stringBundle.getString(aItem.localName.slice(7) + "Title");
> Actually I don't understand how the old code worked, unless somehow the cloned
> spacer/separator/springs always had the unique id rather than the random one.

Looking at this again, I don't even know why this code exists. What is the title used for anyways?

Comment 10

[[e-mail]]

(In reply to comment #9)
>(In reply to comment #7)
>>(In reply to comment #4)
>>>(From update of attachment 302343 [details] [diff] [review] [details])
>>>I don't understand:
>>>>-    var title = stringBundle.getString(aItem.id + "Title");
>>>>+    var title = stringBundle.getString(aItem.localName.slice(7) + "Title");
>>Actually I don't understand how the old code worked, unless somehow the cloned
>>spacer/separator/springs always had the unique id rather than the random one.
>Looking at this again, I don't even know why this code exists. What is the
>title used for anyways?
I think it's an extra label to be shown in the palette for items that don't normally have one.

Comment 11

[[e-mail]]

(In reply to comment #4)
>(From update of attachment 302343 [details] [diff] [review])
>And I think:
>>     // Attempt to locate an item with a matching id within palette.
>>-    var paletteItem = this.parentNode.palette.firstChild;
>>-    while (paletteItem) {
>>-    var paletteId = paletteItem.id;
>>-    if (paletteId == aId) {
>>-      newItem = paletteItem.cloneNode(true);
>>-      break;
>>-    }
>>-    paletteItem = paletteItem.nextSibling;
>>-    }
>>+    newItem = document.getElementById(aId);
>will break firefox, because we have children of elements in the palette that
>have the same IDs as elements that used to be children of the palette, and we
>are specifically relying on the fact that that code only checks children.
Sorry for the misunderstanding last time.

I think I need to check that the element's grandparent is the toolbox - I'd prefer that to checking that the parent is the palette, if possible.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #11)
> I think I need to check that the element's grandparent is the toolbox - I'd
> prefer that to checking that the parent is the palette, if possible.

That wouldn't address the main problem, because in this case the elements' grandparent *is* the toolbox, but we still don't want them found. It would address the (minor) second concern, though (getting elements from elsewhere in the document).

I'm not sure why you want to avoid checking the parent - what's wrong with that?

Comment 13

[[e-mail]]

(In reply to comment #12)
>That wouldn't address the main problem, because in this case the elements'
>grandparent *is* the toolbox, but we still don't want them found. It would
>address the (minor) second concern, though (getting elements from elsewhere in
>the document).
No, the back button's parent is the unified button, whose parent is the toolbar, whose parent is the toolbox.

>I'm not sure why you want to avoid checking the parent - what's wrong with that?
I think this code might get called to move items between toolbars. I also thought there was another reason, which I've since forgotten.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #10)
> I think it's an extra label to be shown in the palette for items that don't
> normally have one.

I couldn't get it to appear when hovering over any of the "special" items, on Windows or Mac. Perhaps it should be removed?

(In reply to comment #13)
Ah, I see. Yes, that could work. So with that check this patch is probably OK, but it needs to be unbitrotted.

Comment 15

[[e-mail]]

(In reply to comment #14)
> (In reply to comment #10)
> > I think it's an extra label to be shown in the palette for items that don't
> > normally have one.
> I couldn't get it to appear when hovering over any of the "special" items, on
> Windows or Mac.
Not hovering, but when customising, the spacer/spring/separator are labelled.

> (In reply to comment #13)
> Ah, I see. Yes, that could work. So with that check this patch is probably OK,
> but it needs to be unbitrotted.
Yes, by backing out a patch to a bug that this bug incidentally fixes :-)

Comment 16

[[e-mail]]

Attachment: Addressed review comments

I haven't actually tested this yet, but it's not wildly different than before.

Comment 17

[Robert Kaiser]

I just tested this with Firefox, and it works fine - with one exception: When I add a new toolbar, drag some items into it, and then reset to default, those items are gone from the palette and the UI, needing a Firefox restart to get them back in the palette.
Everything else looks good in testing!

Comment 18

[[e-mail]]

Attachment: Additional fix for KaiRo's bug

* Emptied out custom toolbars before removing them from the DOM.

Comment 19

[[e-mail]]

Attachment: Fix regression from comment 8

My fix for comment 8 broke springs/spaces/separators...

Comment 20

[Robert Kaiser]

Thanks a lot, the current patch really looks good in all my Firefox customize testing!

Comment 21

[[e-mail]]

Comment on attachment 338158 [details] [diff] [review]
Fix regression from comment 8

>-            // Hold on to the palette but remove it from the document.
>             toolbox.palette = node;       
>-            toolbox.removeChild(node);
If you don't want this change then that's not a problem, we can work around it in our derived binding; just remind me to remove it before check in.

Comment 22

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #21)
> (From update of attachment 338158 [details] [diff] [review])
> >-            // Hold on to the palette but remove it from the document.
> >             toolbox.palette = node;       
> >-            toolbox.removeChild(node);
> If you don't want this change then that's not a problem, we can work around it
> in our derived binding; just remind me to remove it before check in.

Right, I don't think we can take this change. Undoing it requires undoing your changes to insertItem in toolbar.xml as well though, doesn't it? getElementById won't find the element in the palette if it's not in the document. Can you post an updated patch that works?

Comment 23

[[e-mail]]

Attachment: Do things the hard way

Untested... I was too busy losing a chess game (fortunately the team still won)

Comment 24

[Robert Kaiser]

Comment on attachment 341027 [details] [diff] [review]
Do things the hard way

>diff -r 432d71ba1387 toolkit/content/widgets/toolbar.xml
>--- a/toolkit/content/widgets/toolbar.xml	Sat Sep 20 00:20:08 2008 +0200
>+++ b/toolkit/content/widgets/toolbar.xml	Tue Sep 30 00:22:47 2008 +0100
>+              if (!newItem || !newItem.parentNode ||
>+                  newItem.parentNode.parentNode != this.parentNode)
>+                return false;

The "newItem.parentNode.parentNode != this.parentNode" part of this check results in no items appearing in the toolbar now - if I remove it, the items appear.

Additionally, I suddenly have back, forward and the history dropdown appearing as separate buttons in the FF palette, and only the history dropdown being in the default set if I restore that, while without the patch those three are treated as one single element. Not sure if that's related with my removal of the check mentioned in comment #1 or if it's something else in your patch.

Unfortunately, we're only 29 hours from the 1.9.1 feature freeze (b1 freeze) right now, I hope you can still get this fixed, reviewed and checked in by then...

Comment 25

[Robert Kaiser]

As this is nearing completion and SeaMonkey really wants this in toolkit for the 1.9.1-based 2.0 release, trying to get this on a 1.9.1 radar. We're trying to make today's freeze, but could miss it slightly. I hope we still have a chance of getting this into 1.9.1 despite that as from what I understand, Neil thinks about forking toolbar customization if this doesn't make it, which I'd like to avoid for the sake of maintainability and extension compatibility.

Comment 26

[[e-mail]]

Attachment: Hopefully fixed patch

Based on KaiRo's comments I realised that my match function failed when the palette wasn't in the document. Hopefully this version works :-)

Comment 27

[Robert Kaiser]

Comment on attachment 341171 [details] [diff] [review]
Hopefully fixed patch

This patch works fine for me in FF in the basic tests with moving around stuff, adding toolbars, and restoring defaults.

Comment 28

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 341171 [details] [diff] [review]
Hopefully fixed patch

>diff -r 432d71ba1387 toolkit/content/customizeToolbar.js

>-    var title = stringBundle.getString(aItem.id + "Title");
>+    var title = stringBundle.getString(aItem.localName.slice(7) + "Title");

Please add a comment explaining the slice, or use "toolbar".length.

>diff -r 432d71ba1387 toolkit/content/widgets/toolbar.xml

>-            while (this.lastChild) {
>-              if (this.lastChild == this.lastPermanentChild ||
>-                  (this.lastChild.localName == "toolbarpaletteitem" &&
>-                  this.lastChild.firstChild == this.lastPermanentChild))
>-                break;
>-              this.removeChild(this.lastChild);
>-            }
>+            while (this.lastChild != this.lastPermanentChild)
>+              palette.appendChild(this.lastChild);

Hmm, I hope no one else is calling this setter while the items are wrapped... It's a shame that we ever supported that.

>+              // Attempt to locate an item with a matching id within palette.
>+              newItem = document.getElementById(aId);
>+              if (!newItem || !newItem.parentNode ||

Is it really possible for getElementById to return something without a parentNode?

>+                  newItem.parentNode.parentNode != this.parentNode) {
>+                newItem = this.parentNode.palette
>+                              .getElementsByAttribute("id", aId).item(0);
>+                if (!newItem || newItem.parentNode != this.parentNode.palette)
>                   return false;

Might not hurt to explain why you need to fall back to getElementsByAttribute (palette's not be in the document by default).

I'm slightly concerned that some extension is relying on all toolbar items always being in the palette somehow, but I suppose that is pretty unlikely. The win from not keeping two elements in memory for active toolbar items and avoiding clones during startup should more than offset that risk, I think.

It would be really nice to get some tests for this code. There were some toolbar tests in bug 354048, though they mostly focused on the currentSet setter.

Comment 29

[:Gavin Sharp [email: [e-mail]]]

I'm sorry and rather embarrassed that this review took so long to complete, despite Kairo's prodding. I'll try harder to not be a bottleneck in the future.

Comment 30

[[e-mail]]

Attachment: Final patch for push

Requesting approval for this potentially Ts-improving cruft-removal patch.

Comment 31

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 350222 [details] [diff] [review]
Final patch for push

a191=beltzner, yay Ts wins

Comment 32

[[e-mail]]

Pushed changeset fdd5e4e34241 to mozilla-central.

Comment 33

[Ria Klaassen (not reading all bugmail)]

Don't know what exactly happened, but this bug could have been the cause that the Custom Buttons 2 extension stopped function properly. Most of the buttons disappeared from the toolbar and the customize window. They don't exist anymore in the buttonsoverlay.xul and buttonsoverlay.bak files. Not sure if this bug is really the cause but the bug appeared right after this fix for the first time.
It happens after every two or three restarts and it is a kind of little disaster if you didn't backup these files (I think most people don't backup them).

Comment 34

[:Gavin Sharp [email: [e-mail]]]

Ria, could you file a followup please, and CC me?

Comment 35

[alta88]

there's an additional problem in that if you drag a flexible space eg onto a toolbar, Done, customize and drag it off, Done, then customize again there will be a duplicate in the palette.  only for the special items it seems.

needless to say, this did a number on the totaltoolbar extension..

Comment 36

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #35)
> there's an additional problem in that if you drag a flexible space eg onto a
> toolbar, Done, customize and drag it off, Done, then customize again there will
> be a duplicate in the palette.  only for the special items it seems.

Could you please file a new bug, and CC me? That's almost always better than commenting in a closed bug :)

Comment 37

[Shawn Wilsher :sdwilsh]

I backed this out because browser/base/content/test/browser_customize.js has been failing intermittently:
http://hg.mozilla.org/mozilla-central/rev/de15a638ac3c

Comment 38

[Frank Wein [:mcsmurf]]

The error was:
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/browser_customize.js | Timed out

Comment 39

[Philip Chee]

>> there's an additional problem in that if you drag a flexible space eg onto a
>> toolbar, Done, customize and drag it off, Done, then customize again there will
>> be a duplicate in the palette.  only for the special items it seems.

Perhaps the wrapper type gets lost somewhere or the check for wrapper type isn't working in the onDrop method.

> Could you please file a new bug, and CC me? That's almost always better than
> commenting in a closed bug :)

And make that bug block this too.

Comment 40

[Robert Kaiser]

Neil, can you get this back into mozilla-central together with the simple fix for bug 467272? I think we want this to go 1.9.1 again after some baking on trunk.

Comment 41

[[e-mail]]

Pushed changeset 2f727f142360 to mozilla-central.

This should include the fix to bug 467272.

Comment 42

[[e-mail]]

Attachment: [checked in trunk and 1.9.1] Patch as pushed including 467272 fix

Comment 43

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 351378 [details] [diff] [review]
[checked in trunk and 1.9.1] Patch as pushed including 467272 fix

a=191 on the as-pushed patch for good measure

Comment 44

[Mark Banner (:standard8)]

Comment on attachment 351378 [details] [diff] [review]
[checked in trunk and 1.9.1] Patch as pushed including 467272 fix

I've landed this on Neil's behalf: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/48bbf4936793

Comment 45

[:Gavin Sharp [email: [e-mail]]]

CustomButtons 2 was fixed in bug 467045, and I think alta88 has the TotalToolbar bustage covered (though would appreciate confirmation - here or in the forum!), but I'm sure there are other addons that are going to be affected by this change, so it's a shame that we didn't get it in for Beta 2.

I did a naive search on the addons MXR for ".palette", and found the following list of addons that looked like they referred to the browser's palette directly:

https://addons.mozilla.org/en-US/firefox/addon/6363
https://addons.mozilla.org/en-US/firefox/addon/28
https://addons.mozilla.org/en-US/firefox/addon/5202
https://addons.mozilla.org/en-US/firefox/addon/5700
https://addons.mozilla.org/en-US/firefox/addon/1865
https://addons.mozilla.org/en-US/firefox/addon/4028
https://addons.mozilla.org/en-US/firefox/addon/6545
https://addons.mozilla.org/en-US/firefox/addon/1027
https://addons.mozilla.org/en-US/firefox/addon/3495
https://addons.mozilla.org/en-US/firefox/addon/1272
https://addons.mozilla.org/en-US/firefox/addon/5941
https://addons.mozilla.org/en-US/firefox/addon/2313
https://addons.mozilla.org/en-US/firefox/addon/5743
https://addons.mozilla.org/en-US/firefox/addon/3615
https://addons.mozilla.org/en-US/firefox/addon/1833

Is there an easy way to reach out to the authors of these addons specifically? This isn't guaranteed to affect all of them, but it would be nice to give them a heads up and offer to help with any issues they might have because of this patch.

Comment 46

[Christopher Blizzard (:blizzard)]

Rey, is this something you can do?

Comment 47

[:Gavin Sharp [email: [e-mail]]]

I added a description of the changes in this bug on MDC:
https://developer.mozilla.org/En/Updating_extensions_for_Firefox_3.1#Customizable_toolbars

I can help out with any problems people might have updating their extensions.

Comment 48

[alta88]

the toolbar portion of TT is no problem, but the new design will necessitate a 'total' rethink of how customize statusbar is done (nodes there can't actually be removed so the old design worked).

but since the old design for toolbars was pretty silly i'm not complaining, and it's actually good to see effort in code renovation.

Comment 49

[Philip Chee]

https://addons.mozilla.org/en-US/firefox/addon/2313
Lightning. Who are the lightning devs?

https://addons.mozilla.org/en-US/firefox/addon/5743
xSearchbarT2 0.3 is mine. No need to cc me as I am already cc'ed on this and related bugs.

Comment 50

[Tony Mechelynck [:tonymec]]

(In reply to comment #49)
> https://addons.mozilla.org/en-US/firefox/addon/2313
> Lightning. Who are the lightning devs?

Lightning is the non-standalone form of the Calendar product. For bugmail, you might try to CC [e-mail], or for Lightning-but-not-Sunbird, [e-mail] -- or else, you might try browsing http://www.mozilla.org/owners.html and scrolling down to Calendar, to find who are the flesh-and-blood people in charge.

Comment 51

[Philipp Kewisch [:Fallen][📱📆]]

Thanks for the pointer, filed calendar bug 469307.

Comment 52

[Tony Mechelynck [:tonymec]]

Well, does anything need to be done for DOMi, ChatZilla, Venkman, or any other "built-in extensions" distributed together with some Mozilla standalone products?

Comment 53

[Philip Chee]

Adding Twanno (Duplicate Tab), Wladimir Palant (Adblock Plus), Mossop (Toolbar Thinger), Chris Neale (Status Buttons) to the CC list.

Comment 54

[Philip Chee]

> Well, does anything need to be done for DOMi, ChatZilla, Venkman, or any other
> "built-in extensions" distributed together with some Mozilla standalone
> products?

Highly unlikely. Gavin would have grepped MXR and found any dependencies by now.

Comment 55

[Robert Kaiser]

(In reply to comment #52)
> Well, does anything need to be done for [...]?

Nothing needs to be done unless the extension 1) uses customizable toolbars (none of the ones you cited do) and 2) are "messing" with the internals of the customizable toolbars mechanism, which most apps and extensions don't need and want to do, unless they are specifically extending/changing the mechanisms of customizations.

Comment 56

[Wladimir Palant]

Thanks for letting me know. This doesn't seem to have caused any issues in Adblock Plus. Adblock Plus needs to change icon state and does that both in the document and in the palette, without expecting the icon to exist at any of those locations. So this change only eliminated the case where the icon exists both in document and in palette. Too bad it doesn't leave <toolbarpalette> in the document, that would make the icon always accessible by ID - would make such hacks unnecessary.

Comment 57

[[e-mail]]

(In reply to comment #56)
> Too bad it doesn't leave <toolbarpalette> in
> the document, that would make the icon always accessible by ID - would make
> such hacks unnecessary.

That was my one of my ideas, but...

(In reply to comment #4)
> I'm also a bit concerned that keeping the palette in the document will have
> unintended consequences.

Comment 58

[Tony Mechelynck [:tonymec]]

(In reply to comment #55)
> (In reply to comment #52)
> > Well, does anything need to be done for [...]?
> 
> Nothing needs to be done unless the extension 1) uses customizable toolbars
> (none of the ones you cited do) and 2) are "messing" with the internals of the
> customizable toolbars mechanism, which most apps and extensions don't need and
> want to do, unless they are specifically extending/changing the mechanisms of
> customizations.

I see. Well, maybe Mel Reyes (MR-Tech Toolkit et al.) will like to be notified then.

Comment 59

[Philip Chee]

> I see. Well, maybe Mel Reyes (MR-Tech Toolkit et al.) will like to be notified
> then.

What's stopping you from adding him to the CC list then?

Comment 60

[Mel Reyes]

Outside of the standard toolbar icons, icon dropdown menus and customizing some of my toolbar's context menus I don't explicitly handle adding/removing toolbar icons so I think I'm safe, thanks.

Comment 61

[Tony Mechelynck [:tonymec]]

(In reply to comment #59)
> > I see. Well, maybe Mel Reyes (MR-Tech Toolkit et al.) will like to be notified
> > then.
> 
> What's stopping you from adding him to the CC list then?

If you check history, you'll see I did, when posting the comment you quoted above.

Comment 62

[Paul Rouget [:paul]]

I would like to add a reference to this change to MDC.
I want to be sure to have understood well how this can affect addons developers.
Please correct me if I'm wrong :

Items in browser toolbars are now moved from the toolbarpalette rather than cloned from it. That means the toolbarpalette now only hosts non used items.

Comment 63

[Philip Chee]

> Items in browser toolbars are now moved from the toolbarpalette rather than
> cloned from it. That means the toolbarpalette now only hosts non used items.

Correct.

Comment 64

[Stefan [:stefanh]]

See https://developer.mozilla.org/En/Updating_extensions_for_Firefox_3.1#Customizable_toolbars