Toolbar elements losing the command attribute

RESOLVED FIXED in mozilla2.0b4

Status

()

Toolkit
Toolbars and Toolbar Customization
--
minor
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: Andrea Scartabelli, Assigned: neil@parkwaycc.co.uk)

Tracking

({regression})

1.9.1 Branch
mozilla2.0b4
regression
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

8 years ago
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.
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)?
Version: unspecified → 1.9.1 Branch
(Reporter)

Comment 2

8 years ago
(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.
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.
(Reporter)

Comment 4

8 years ago
(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

8 years ago
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 ?
(Reporter)

Comment 6

8 years ago
(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.
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

8 years ago
Regression range?
Keywords: regressionwindow-wanted
(Reporter)

Comment 9

8 years ago
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

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

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

Comment 11

8 years ago
I suspect that this is a regression caused by Bug 407725. cc Neil
Status: UNCONFIRMED → NEW
Depends on: 407725
Ever confirmed: true
(Assignee)

Comment 12

8 years ago
Created attachment 422967 [details] [diff] [review]
Proposed patch
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #422967 - Flags: review?(dao)
(Assignee)

Updated

8 years ago
Keywords: regressionwindow-wanted → regression

Updated

8 years ago
Blocks: 407725
No longer depends on: 407725
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...
Attachment #422967 - Flags: review+
(Reporter)

Comment 14

8 years ago
(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 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/
Attachment #422967 - Flags: review?(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.

Updated

7 years ago
Duplicate of this bug: 577480
(Assignee)

Comment 18

7 years ago
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...
Attachment #422967 - Flags: approval2.0?
Attachment #422967 - Flags: approval2.0? → approval2.0+
(Assignee)

Comment 19

7 years ago
Pushed changeset 3e405cf3eef8 to mozilla-central.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b4
You need to log in before you can comment on or make changes to this bug.