User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b7pre) Gecko/20100923 Firefox/4.0b7pre Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b7pre) Gecko/20100923 Firefox/4.0b7pre If I put a toolbar button (eg. "Print") to Add-on bar, "Restore Default Set" doesn't remove it at all. Reproducible: Always Steps to Reproduce: 1. Open Customization palette. 2. Drag a toolbar item to Add-on bar. Click on Done. 3. Open Customization palette again. Click on Restore Defatult Set. Click on Done. Actual Results: The toolbar item wont be removed from Add-on bar. Expected Results: The toolbar item should be removed from Add-on bar.

Confirmed the STR in comment 0 with Mozilla/5.0 (Windows NT 5.1; rv:2.0b7pre) Gecko/20100923 Firefox/4.0b7pre

Comment on attachment 479801 [details] [diff] [review] patch >diff --git a/toolkit/content/customizeToolbar.js b/toolkit/content/customizeToolbar.js >--- a/toolkit/content/customizeToolbar.js >+++ b/toolkit/content/customizeToolbar.js >@@ -621,31 +621,22 @@ function addNewToolbar() > * Restore the default set of buttons to fixed toolbars, > * remove all custom toolbars, and rebuild the palette. > */ > function restoreDefaultSet() > { > // Unwrap the items on the toolbar. > unwrapToolbarItems(); > >- // Remove all of the customized toolbars. >- var child = gToolbox.lastChild; >- while (child) { >- if (child.hasAttribute("customindex")) { >- var thisChild = child; >- child = child.previousSibling; >- thisChild.currentSet = "__empty"; >- gToolbox.removeChild(thisChild); >- } else { >- child = child.previousSibling; >- } >- } >- >- // Restore the defaultset for fixed toolbars. >+ // Remove all of the customized toolbars, and >+ // restore the defaultset for fixed toolbars. > forEachCustomizableToolbar(function (toolbar) { >+ if (toolbar.hasAttribute("customindex")) >+ gToolbox.removeChild(toolbar); >+ toolbar.currentSet = "__empty"; Hrm, why is that not within the |if (toolbar.hasAttribute("customindex"))| block as it was before?

Good question. The only way the add-on bar would get reset is if i set currentSet = "__empty". So I changed this code to apply that to all customizable toolbars instead of just those with "customindex" attribute set. Which seems to work. I don't know why the add-on bar doesn't have a customindex value.

> I don't know why the add-on bar doesn't have a customindex value. Because it is not a user created toolbar?

Comment on attachment 479801 [details] [diff] [review] patch Hrm, why aren't you setting defaultset to "" on the addonbar (in browser.xul)?

(In reply to comment #7) > Comment on attachment 479801 [details] [diff] [review] > patch > > Hrm, why aren't you setting defaultset to "" on the addonbar (in browser.xul)? in addition to, or instead of what i'm doing now? i don't understand what you're asking.

Instead, AFAICT not having the defaultset attribute set to "" is the cause for this bug.

I tried it, and it doesn't seem to result in any change at all.

Comment on attachment 479801 [details] [diff] [review] patch > forEachCustomizableToolbar(function (toolbar) { >+ if (toolbar.hasAttribute("customindex")) >+ gToolbox.removeChild(toolbar); >+ toolbar.currentSet = "__empty"; Setting currentSet on a removed toolbar isn't going to work. > var defaultSet = toolbar.getAttribute("defaultset"); > if (defaultSet) > toolbar.currentSet = defaultSet; Setting currentSet to __empty before setting it to defaultSet seems to be unnecessary work. I'd suggest this instead: toolbar.currentSet = toolbar.getAttribute("defaultset") || "__empty"; if (toolbar.hasAttribute("customindex")) gToolbox.removeChild(toolbar);

I was a little bit wrong! It shouldn't be just empty it should be __empty.

It probably / hopefully doesn't matter in practice, but the "correct" default set would actually be "status-bar".

Comment on attachment 488655 [details] [diff] [review] patch As far as I understand it, we don't treat static elements as part of the "set".

(In reply to comment #14) > As far as I understand it, we don't treat static elements as part of the "set". We certainly do for toolbars in the navigator-toolbox.

Can this bug be related to bug 607600 (https://bugzilla.mozilla.org/show_bug.cgi?id=607600)?

Comment on attachment 488655 [details] [diff] [review] patch Hm, I think that as long as the statusbar compat hack is there it should be treated as a default.

Comment on attachment 493364 [details] [diff] [review] patch r=author.