498100 - Can not remove Toolbarbutton from menubar after Customize toolbars & uncheck Menubar.

Status: VERIFIED FIXED

(Toolkit :: Toolbars and Toolbar Customization, defect)

Description

[Alice0775 White]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1pre) Gecko/20090612 Firefox/3.5.0 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090613 Minefield/3.6a1pre ID:20090613042114

Can not remove Toolbarbutton from menubar after Customize toolbars & uncheck Menubar.

Reproducible: Always

Steps to Reproduce:
1.Start Minefield with New profile
2.View > Toolbars > Menubar and set it to uncheck state.
3.View > Toolbars > Customize
4.Drag & drop "Print"button  to the Menubar and  Click done.
5.View > Toolbars > Menubar and set it to check state.
6.View > Toolbars > Customize
7.Drag & drop "Print"button to Customize Toolbar Window from  the Menubar


3.
Actual Results:  
Can not remove

Expected Results:  
Can remove

Comment 1

[Dão Gottwald [:dao]]

Attachment: patch

sigh, xbl.

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 383114 [details] [diff] [review]
patch

this breaks persisting, since it removes the attribute rather than setting it to "false"

Comment 3

[Dão Gottwald [:dao]]

Attachment: patch v2

this should work, still need to test it

Comment 4

[[e-mail]]

Comment on attachment 383149 [details] [diff] [review]
patch v2

This looks mostly good (and fixes DSS too, yay!) but one thing I don't understand is why you changed the currentSet getter...

Comment 5

[Dão Gottwald [:dao]]

(In reply to comment #4)
> (From update of attachment 383149 [details] [diff] [review])
> This looks mostly good (and fixes DSS too, yay!) but one thing I don't
> understand is why you changed the currentSet getter...

For the same reason currentSetChanged is called in there: toolbar customization doesn't set the currentSet, but it modifies the toolbar content and lets toolbar.xml rebuild the currentSet (via the getter) in order to persist it.

Comment 6

[[e-mail]]

Comment on attachment 383149 [details] [diff] [review]
patch v2

Ah, that makes sense, thanks!

>             var node = this.firstChild;
>             var currentSet = "";
>+            var i = 0;
>             while (node) {

>               node = node.nextSibling;
>+              i++;
>             }
[I wonder why this wasn't ever written as a for loop]

Comment 7

[Dão Gottwald [:dao]]

If you prefer it more compact, I could offer this:

> for (let i = 0, node; node = this.childNodes.item(i); i++) {

Comment 8

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/eb2734e5956a

Comment 9

[[e-mail]]

(In reply to comment #7)
> If you prefer it more compact, I could offer this:
> > for (let i = 0, node; node = this.childNodes.item(i); i++) {
That's an ingenious approach ;-)

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090706 Minefield/3.6a1pre (.NET CLR 3.5.30729) ID:20090706042748