352158 - Back/Forward buttons appear disabled in Customize Toolbar view

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Cheng Khoon]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b2) Gecko/20060911 BonEcho/2.0b2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b2) Gecko/20060911 BonEcho/2.0b2

When user is in customize toolbar view, all toolbars should show its normal state image. However, back/forward button is still showing its disabled state image.

Reproducible: Always

Steps to Reproduce:
1. Open a new Firefox window
2. Right Click on the Toolbar area and choose 'Customize...'

Actual Results:  
Back/Forward buttons show its disabled button image

Expected Results:  
Back/Forward buttons should show its normal button image

Comment 1

[Ria Klaassen (not reading all bugmail)]

Hm, I see this in the windows version of Firefox 1.0.7, but not in Firefox 1.5.0.6, Minefield or BonEcho. So it is not a recent regression a least.

Comment 2

[Ria Klaassen (not reading all bugmail)]

Regression between 1.9a1_2005082420 and 1.9a1_2005082501.
I doubt if this should block 347454 if no-one missed the green color until now.
Can't find a dupe at first sight so I'll mark it new.

Comment 3

[Peter Kasting]

(In reply to comment #2)
> Regression between 1.9a1_2005082420 and 1.9a1_2005082501.
> I doubt if this should block 347454 if no-one missed the green color until now.
> Can't find a dupe at first sight so I'll mark it new.

Tried to do a bonsai query for that range:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=MOZILLA_1_8_BRANCH&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-08-24+20%3A00%3A00&maxdate=2005-08-25+01%3A00%3A00&cvsroot=%2Fcvsroot

...but nothing looks promising... maybe my Bonsai query is wrong.

Comment 4

[Peter Kasting]

(In reply to comment #2)
> Regression between 1.9a1_2005082420 and 1.9a1_2005082501.

Wait a minute; I was doing a branch query, and you gave trunk times.

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=1124938800&maxdate=1124956800&cvsroot=%2Fcvsroot

Still doesn't look too promising.

Comment 5

[Ria Klaassen (not reading all bugmail)]

The only one that matches exactly the checkin time in branch is bug 295404.
In branch it regressed when the fix of bug 295404 was checked in.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Seems possible that this working was depending on an underlying bug that that fixed.

Comment 7

[Peter Kasting]

Hmm.  The behavior I see, with more testing, is that the back/forward buttons do not reset to "enabled" when entering Customize mode, BUT if they are dragged onto the palette, they are enabled there, and when dragged back onto the toolbar, they're still enabled, until we exit Customize mode, when the buttons reset to their true values.

Also I confirm this was true in 1.5 as well, so this has nothing to do with the theme change.  Removing whiteboard comment/blocking metabug.

Comment 8

[Boris Zbarsky [:bzbarsky]]

So what code is supposed to give things the enabled look?

Comment 9

[Phil Ringnalda (:philor)]

bz: what tries to enable things is http://lxr.mozilla.org/seamonkey/source/toolkit/content/customizeToolbar.js#458 which shouldn't have ever worked on Back and Forward, so we must rely on bugs to break the observes - I wouldn't think that even cleanUpItemForPalette()'s more aggressive attempt would actually work.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> so we must rely on bugs to break the observes

Meaning what?  And more importantly, are you saying that this code isn't supposed to work as written and the fact that it did before bug 295404 landed was a bug?

Comment 11

[Phil Ringnalda (:philor)]

Meaning that my take on it is that the only reason it ever worked as intended was a bug in the finding of broadcasters going through an element from another document. At the time this bug was filed, we would take a node with an <observes> child out of the browser window, insert it in an element created by the customize dialog, and then insert that element back into the browser window, and it looks like doing that used to break the <observes>, before bug 295404 made it not break it, and then bug 47903 made it impossible to use that intermediary element belonging to another document. The comment 7 behavior, where we have the binding clone a node from the browser window, insert that into an element adopted by the browser window, and insert the two of them into the browser window without getting the observer wired up, seems to me like another bug that we're still relying on.

But, given my track record for misinterpreting what should be, how about I try to describe what is, and you tell me whether or not it's right?

We start out with a 

<toolbarbutton>
  <observes element="Browser:Back" attribute="disabled"/>
</toolbarbutton>

in the browser window.

customizeToolbar.xul's onload calls through to customizeToolbar.js's wrapToolbarItem(), which calls createWrapper(), which creates a toolbarpaletteitem element, in the dialog's document.

Prior to bug 47903, we ignored what document it was created in; now (bug 361627) we tell the browser window to adoptNode() the toolbarpaletteitem.

We then call cleanupItemForToolbar(), which, when the toolbarbutton's disabled property is true, stores that fact in the toolbarpaletteitem's "itemdisabled" attribute, and sets the toolbarbutton's disabled property to false, but doesn't touch the observer.

We then tell the toolbarbutton's parentNode to removeChild(), and tell the toolbarpaletteitem to appendChild(), and tell the toolbarbutton's former parentNode to insertBefore() the Forward button.

At this point, we have

<toolbarpaletteitem>
  <toolbarbutton>
    <observes element="Browser:Back" attribute="disabled"/>
  </toolbarbutton>
</toolbarpaletteitem>

in the browser window, and despite having set toolbarbutton.disabled = false earlier, the observer is wired up, and if the Browser:Back command in http://lxr.mozilla.org/seamonkey/source/browser/base/content/browser-sets.inc#158 is currently disabled, then the observer will disable the toolbarbutton, and we'll get the disabled appearance.

So, _if_ bug 295404 introduced a bug, that bug would seem to be that it will now successfully find a broadcaster in document A, for an element belonging to document A, when that element is a child of an element belonging to document B, despite that parent having been shoved into document A; a case it won't be able to see again, thanks to bug 47903.

What seems more likely to be a bug that would interest you is what Peter reports in comment 7, that dragging a disabled button off the toolbar to the palette, and then back onto the toolbar, leaves it looking enabled until the customization dialog is closed.

When you drag it out, customizeToolbar.js#934 finds the node in the browser window's toolbarpalette in browser.xul#174, and gets a cloneNode() from that. wrapPaletteItem() then creates a toolbarpaletteitem in the dialog's document, and because of bug 47903 we now adoptNode() the cloned node into the dialog's document, and then stuff the cloned node into the toolbarpaletteitem, and stuff that into the dialog's document. At this point, my guess is that the observer is busted since the dialog's document doesn't have a broadcaster for it to listen to, so when we set the toolbarbutton's disabled property to false, it stays that way, and looks enabled in the dialog.

Then, when you drag it back onto the toolbar, customizeToolbar.js#850, we create a toolbarpaletteitem belonging to the dialog, we now make the browser window adoptNode() it though we used to just ignore that, and then to stir things up we call into http://lxr.mozilla.org/seamonkey/source/toolkit/content/widgets/toolbar.xml#203, passing in the adopted wrapper and the id for the Back button. That again gets a cloneNode() from browser.xul's toolbarpalette, tells the passed-in toolbarpaletteitem to appendChild() the cloned node, and tells the toolbar to insertBefore() the Forward button. However, in this case, even though we didn't go through customizeToolbar.js's (worthless) attempt to set disabled to false, the observer apparently isn't wired up, since we have an enabled-looking Back button even when Browser:Back is saying it's disabled. Then when you close the customization dialog, the replaceChild() that pulls the toolbarbutton out of the toolbarpaletteitem gets the observer wired back up.

So, back to this bug: is that behavior, that when the toolbar binding clones a node with an observer, the observer doesn't get wired up, a bug? And, is it only something to do with the binding doing the cloning, or am I wrong about why it looks enabled in the dialog, and cloneNode() will always result in a non-working observer, until the element is removed from its document and reinserted? If it is not a bug, and will stay that way, we could fix _this_ bug by doing that same thing in the initial wrapping of the toolbar items, wrapping a clone instead of the actual current button. If, as seems more likely, it is a bug that will eventually be fixed, um, got any advice about how to cleanly disable and then later re-enable an <observes> child?

Comment 12

[Boris Zbarsky [:bzbarsky]]

So...  Problem is that I know nothing about <observes> and how it should or should not act in what cases.  ccing the XUL-meisters in hopes that they do know.

Comment 13

[Phil Ringnalda (:philor)]

My apologies: I suck. That 8Kb of comment should have been nothing but "Except when you drop them from the palette, we're saving off disabled and setting enabled before they're in the document where it gets set, we just need to call cleanupItemForToolbar() later."

Comment 14

[Phil Ringnalda (:philor)]

Attachment: Fix v.1

Comment 15

[Phil Ringnalda (:philor)]

That will fail on the edge case of loading a page that does history.back() on a timeout, and customizing while the timeout fires, but compared to having to dig down to <observes> children to save-and-neuter them, seems a minor edge case.

Comment 16

[[e-mail]]

(In reply to comment #12)
>Problem is that I know nothing about <observes> and how it should or should
>not act in what cases.  ccing the XUL-meisters in hopes that they do know.
What I believe should happen is that when you add an element with an observes attribute or local name to a document containing an element with an id matching the observer then the attribute or attributes of that element get broadcast to the observing element until such time as one of the two elements is removed.
The fact that a node was cloned, imported or adopted should not be relevant.

Comment 17

[Phil Ringnalda (:philor)]

toolkit/content/customizeToolbar.js 1.35