652842 - _getToolbarItem(aId) returns null if the toolbaritem is on the Add-on Bar

Status: RESOLVED FIXED

(Toolkit :: Toolbars and Toolbar Customization, defect)

Description

[ithinc]

User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:6.0a1) Gecko/20110425 Firefox/6.0a1
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:6.0a1) Gecko/20110425 Firefox/6.0a1

See the following steps.


Reproducible: Always

Steps to Reproduce:
1. Customize and move the New Window button to the Add-on Bar
2. Execute in JS shell:
document.getElementById("nav-bar")._getToolbarItem("new-window-button")


Actual Results:  
The call returns null

Expected Results:  
The call returns the New Window button


http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/toolbar.xml#282

308                 // look for an item with the same id, as the item may be
309                 // in a different toolbar.
310                 var item = document.getElementById(aId);
311                 if (item && item.parentNode && item.parentNode.parentNode == toolbox) {
312                   newItem = item;
313                   break;
314                 }

The problem lies in line 311. "item.parentNode.parentNode" should be "item.parentNode.toolbox".

Comment 1

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

This is new code which has been landed with Neil's patch on bug 354048.

Comment 2

[ithinc]

(In reply to comment #1)
> This is new code which has been landed with Neil's patch on bug 354048.

I don't see it newer than Firefox 4.0.

Comment 3

[ithinc]

Besides, it seems this method should belong to the toolbox, not the toolbar.

Comment 4

[ithinc]

Some comments from https://bugzilla.mozilla.org/show_bug.cgi?id=599029#c5.

Comment 5

[Neil Deakin]

> The problem lies in line 311. "item.parentNode.parentNode" should be "item.parentNode.toolbox".

Can you make a patch which fixes this?

Comment 6

[ithinc]

Attachment: patch

Comment 7

[Neil Deakin]

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

This looks ok. You may want to do an extra conditional check of either of:
  1. 'toolbox' in item.parentNode
  2. item.parentNode.localName == "toolbar"

to avoid an error in the case where someone incorrectly used a non-toolbar id.

Comment 8

[ithinc]

We have checked !toolbox and item.parentNode.toolbox == toolbox, which implies 'toolbox' in item.parentNode. I think it's enough, isn't it?

Comment 9

[Neil Deakin]

Yeah, it's not a big issue, but item.parentNode.toolbox == toolbox will cause a warning (in js strict mode) if item.parentNode isn't a toolbar as it won't have a toolbox property.

Comment 10

[ithinc]

(In reply to comment #9)
> Yeah, it's not a big issue, but item.parentNode.toolbox == toolbox will
> cause a warning (in js strict mode) if item.parentNode isn't a toolbar as it
> won't have a toolbox property.

I see. Then it makes some sense.

Comment 11

[ithinc]

Attachment: patch v2

Comment 12

[Dão Gottwald [:dao]]

malformed patch a/toolkit/content/widgets/toolbar.xml @@ -303,17 +303,17 @@

Comment 13

[ithinc]

Attachment: patch v3

(In reply to comment #12)
> malformed patch a/toolkit/content/widgets/toolbar.xml @@ -303,17 +303,17 @@

Thanks, corrected.

Comment 14

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

no reason to stay unconfirmed if there is a patch, right?

Comment 15

[Philip Chee]

> no reason to stay unconfirmed if there is a patch, right?
Right, setting status to ASSIGNED.

Comment 16

[Neil Deakin]

Comment on attachment 538671 [details] [diff] [review]
patch v3

I already reviewed this, no? Did you mean to attach a different version?

Or is the patch ready to be checked in?

Comment 17

[ithinc]

I have no idea why you gave a review- for patch v3. Maybe it was a mis-operation. Patch v2 was malformed, but v3 should be ok. It is ready to be checked in.

Comment 18

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/cffa5e5bb0f6