540838 - Toolbar elements losing the command attribute

Status: RESOLVED FIXED

(Toolkit :: Toolbars and Toolbar Customization, defect)

Description

[Andrea Scartabelli]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.7) Gecko/20091221 Firefox/3.5.7 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.7) Gecko/20091221 Firefox/3.5.7

When an element is dragged away from the toolbar and dropped in the customization dialog and later repositioned on the toolbar, without reloading the chrome window, loses its command attribute.

Reproducible: Always

Steps to Reproduce:
1. Open the "customize toolbar" dialog
2. Remove an element with a command attribute from the toolbar (e.g. the "stop" button)
3. Put back the button in the toolbar, without restarting the application.
Actual Results:  
The element has no more its command attribute.

Expected Results:  
The element should have the command attribute as intended.

The problem is in how things are handled in chrome://global/content/customizeToolbar.js
Just noticed the bug, hoping to have time to propose a solution for that.

Comment 1

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

I cannot reproduce this. The oncommand handler is still there after dragging it back to the toolbar. Can you also see this in Safe Mode (http://support.mozilla.com/kb/Safe+Mode)?

Comment 2

[Andrea Scartabelli]

(In reply to comment #1)
> I cannot reproduce this. The oncommand handler is still there after dragging it
> back to the toolbar.

The oncommand handler is still in place, but the command attribute is gone.
Actually there is no "bond" with the intended command unless you restart the application.

Comment 3

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

How do you check that? The DOMi still shows the assigned oncommand handler for the stop button for me after doing the steps in your comment 0.

Comment 4

[Andrea Scartabelli]

(In reply to comment #3)
> How do you check that? The DOMi still shows the assigned oncommand handler for
> the stop button for me after doing the steps in your comment 0.

I'm not talking about the oncommand handler, but about the command attribute (you can check that with the DOM inspector).
Without that attribute the relation between the element and the command is gone and the command element isn't broadcasting anymore its attributes to the element.

Comment 5

[Philip Chee]

In unwrapToolbarItems() the command attributes should have been restored from the wrapper.

Do you have any extensions interfering with the customization code? Can you reproduce this in firefox.exe -safe-mode ?

Comment 6

[Andrea Scartabelli]

(In reply to comment #5)
> In unwrapToolbarItems() the command attributes should have been restored from
> the wrapper.

That's the point: should have.
But when you follow my steps the wrapper has no itemcommand attribute, therefore no command attribute is attached to the element.

A simple test you can do is with the stop button of the browser: drop it in the toolbar customization dialog and the put it back in place.
Then load a web page and you can notice that the button is not disabled, as it should be, because the element doesn't receive the command broadcast.

> Do you have any extensions interfering with the customization code? Can you
> reproduce this in firefox.exe -safe-mode ?

Nothing interfering: I noticed that first in my XULRunner application (no extensions but the DOM inspector), then I tested firefox 3.5.7 on Windows and MacOS X.

Comment 7

[Ria Klaassen (not reading all bugmail)]

I tested this with the latest DOM inspector and latest Minefield on Windows Vista.
This is the output:

Before step 1: 

id="stop-button" class="toolbarbutton-1 chromeclass-toolbar-additional" label="Stop" command="Browser:Stop" tooltiptext="Stop loading this page" oncommand="BrowserStop();" disabled="true"

After step 3: 

oncommand="BrowserStop();" id="stop-button" class="toolbarbutton-1 chromeclass-toolbar-additional" label="Stop" tooltiptext="Stop loading this page"

Comment 8

[Philip Chee]

Regression range?

Comment 9

[Andrea Scartabelli]

I had only time for a *very quick* test and tried adding a couple of lines to chrome://global/content/customizeToolbar.js (XULRunner 1.9.1.7) in the "onDrop" method of "paletteDNDObserver", just before line 913.

Current code:
912 > appendPaletteItem(document.importNode(wrapper.firstChild, true));
913 > gToolbox.palette.appendChild(wrapper.firstChild);

My code:
912 > appendPaletteItem(document.importNode(wrapper.firstChild, true));
913 > if (wrapper.hasAttribute("itemcommand"))
914 >   wrapper.firstChild.setAttribute("command", 
                                        wrapper.getAttribute("itemcommand"));
915 > gToolbox.palette.appendChild(wrapper.firstChild);

That seems to do the trick but, again, it was only a real quick test in my XULRunner application.

Comment 10

[Philip Chee]

Based on unwrapToolbarItems()
I think you need to do this as well:

    if (wrapper.hasAttribute("itemdisabled"))
      wrapper.firstChild.disabled = true;

Comment 11

[Philip Chee]

I suspect that this is a regression caused by Bug 407725. cc Neil

Comment 12

[[e-mail]]

Attachment: Proposed patch

Comment 13

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

Comment on attachment 422967 [details] [diff] [review]
Proposed patch

>diff -r 64b0a446982b toolkit/content/customizeToolbar.js

>+function restoreItemForToolbar(aItem, aWrapper)

>+    //XXX Bug 309953 - toolbarbuttons aren't in sync with their commands after customizing
>+    var command = gToolboxDocument.getElementById(commandID);
>+    if (command && command.hasAttribute("disabled"))
>+      aItem.setAttribute("disabled", command.getAttribute("disabled"));

Seems to me like you should continue to use .disabled, assuming it works...

Comment 14

[Andrea Scartabelli]

(In reply to comment #10)
> Based on unwrapToolbarItems()
> I think you need to do this as well:
> 
>     if (wrapper.hasAttribute("itemdisabled"))
>       wrapper.firstChild.disabled = true;

You are absolutely right.
I'm always in big hurry these days, but I noticed that they already considered that in the proposed patch.

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 422967 [details] [diff] [review]
Proposed patch

>+  if (aWrapper.hasAttribute("itemcommand")) {
>+    var commandID = aWrapper.getAttribute("itemcommand");
>+    aItem.setAttribute("command", commandID);
>+
>+    //XXX Bug 309953 - toolbarbuttons aren't in sync with their commands after customizing
>+    var command = gToolboxDocument.getElementById(commandID);

s/var/let/

Comment 16

[Dão Gottwald [:dao]]

(In reply to comment #13)
> >+    var command = gToolboxDocument.getElementById(commandID);
> >+    if (command && command.hasAttribute("disabled"))
> >+      aItem.setAttribute("disabled", command.getAttribute("disabled"));
> 
> Seems to me like you should continue to use .disabled, assuming it works...

It doesn't work for command elements.

Comment 18

[[e-mail]]

Comment on attachment 422967 [details] [diff] [review]
Proposed patch

Sigh. So because gavin added an additional review, and dao cancelled his review, I didn't realise that this was waiting for me to check in...

Comment 19

[[e-mail]]

Pushed changeset 3e405cf3eef8 to mozilla-central.