191637 - Customize Toolbar Bustage

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[alanjstr]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3b) Gecko/20030202 Phoenix/0.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3b) Gecko/20030202 Phoenix/0.5

Also noted by Asa at http://bugzilla.mozilla.org/show_bug.cgi?id=191524#c12

When returning from Customize Toolbar, toolbars do not return to the normal state.
The Bookmarks Toolbar is still collapsed and you can't click in the URLbar.

Restarting Phoenix corrects the problem.

Ben said he doesn't think this is caused by his new preferences changes.  As far
as I know, Phoenix's toolbars are still seperate from Mozilla's.  

Reproducible: Always

Steps to Reproduce:

Comment 1

[alanjstr]

Attachment: Toolbars not restored to normal functionality

Comment 2

[Sébastien Delahaye]

Confirming.

Comment 3

[David P James]

Seeing in Linux as well

-->All OS

Comment 4

[David Tenser [:djst]]

Severity -> Major
Correcting summary and setting target milestone.

Comment 5

[David Tenser [:djst]]

The reported problem is not all of it. I'm not sure if I should file a new bug
for this, but I think it's related to the same problem.

Anyway, try this:

1. Create a new profile and start Phoenix
2. Select Customize Toolbar...
3. Just move a button to a new location, or add/remove a button
4. Click Done and close Phoenix and restart

Expected Results:
The new toolbar is saved properly

Actual Results:
Only the Forward and Reload button is left on the toolbar.

This is tested with the feb 02 nightly on WinXP.

Comment 6

[alanjstr]

David James reported to me that this happens to him with random pairs of buttons
missing, having gone back to the Customize Window for no apparent reason.  They
can be re-added, and they dissappear again.  

Comment 7

[David Tenser [:djst]]

*** Bug 191650 has been marked as a duplicate of this bug. ***

Comment 8

[David Tenser [:djst]]

*** Bug 191649 has been marked as a duplicate of this bug. ***

Comment 9

[Emily Bowman]

Items I've noted since installing:

After fighting with the browser over the toolbar, I finally wrote the items I
wanted into my localstore.rdf, and they do appear and stick fine as long as I
don't use customize again.

urlbar-container doesn't work at all, however. It never shows the current url
and never responds to anything I type. Also, the autocomplete menu appears far
to the left, mostly off the screen. Will try a fresh reinstall and see if that
helps.

Comment 10

[Emily Bowman]

Sorry, urlbar was a different problem. There was one in the (collapsed) nav-bar,
and one in the toolbar, so I guess it would only read from the one in the
nav-bar. Adding currentset="__empty" to all collapsed bars fixes. And now that
I'm kind of figuring out how to edit the bars by hand, it's not so big a deal...
but a quick fix would not be unappreciated. ;)

For those who don't know what I'm talking about: Toolbar info is stored in
{profile}/localstore.rdf where you can edit it by hand. The various tools you
can use are listed by id in /chrome/browser.jar!content/browser/browser.xul in
the various toolbar and toolbaritem tags. Hope this helps a few of you who don't
yet want to go back.

Here's my (minimalist) configuration. Each bar you later describe must be listed
as NC:persist.

  <RDF:Description about="chrome://browser/content/browser.xul">
    <NC:persist resource="chrome://browser/content/browser.xul#main-window"/>
    <NC:persist resource="chrome://browser/content/browser.xul#PersonalToolbar"/>
    <NC:persist resource="chrome://browser/content/browser.xul#toolbar-menubar"/>
    <NC:persist resource="chrome://browser/content/browser.xul#nav-bar"/>
  </RDF:Description>
  <RDF:Description about="chrome://browser/content/browser.xul#nav-bar"
                   collapsed="true"
                   currentset="__empty" />
  <RDF:Description about="chrome://browser/content/browser.xul#PersonalToolbar"
                   collapsed="true"
                   currentset="__empty" />
  <RDF:Description about="chrome://browser/content/browser.xul#toolbar-menubar"
                  
currentset="menubar-items,back-button,forward-button,stop-button,urlbar-container"
/>

Other toolbaritems:
 reload-button, home-button, search-container, go-container, print-button,
personal-bookmarks, history-button, bookmarks-button, new-tab-button,
new-window-button

Comment 11

[Corwin K. Watts]

The problem still exists in the 20030203 build and seems to be worse. Even the
default theme has non-functional toolbar buttons from the first use. And I've
noticed that the location bar remains blank, that is, it will not display the
url of the web page, nor allow me to type in a new url.

Comment 12

[Sébastien Delahaye]

*** Bug 191779 has been marked as a duplicate of this bug. ***

Comment 13

[Gustavo Araujo Bittencourt]

- Installation (Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3b)
Gecko/20030203 Phoenix/0.5)
- Create Profile
- Run Phoenix
- Menu View/Toolbars/Customize...
- move all elements from "Navigation Toolbar" to "Menu Toolbar"
- Restart Phoenix

Result:
- Only menu, "Forward" icon, "Stop" icon and "url" box appear on "Menu Toolbar",
the other elements disappear.

Comment 14

[Gustavo Araujo Bittencourt]

Attachment: Image of comment #13

Comment 15

[Cody Woods]

Also in the 20030203 build, functioning back/forward buttons 'respawn' inside of
the customize menu, and all of the back/forward buttons on the toolbars cease to
work.

Comment 16

[Nils]

Here's something interesting I found, I've only been able to produce the toolbar
problem under Windows XP and .NET Server 2003. Under Windows 2000 I haven't
gotten the problem to occur it actually works how it's supposed to under Win2k.
so I'm not sure if it's possibly linked to the new UI of Windows XP and .NET.

Comment 17

[alanjstr]

Under Win2k, I didn't lose any of my buttons, just the functionality of those
that were replaced with placeholders while in Customize mode.

Comment 18

[Corwin K. Watts]

The problem is probably not related to the new GUI of windows 2000 and XP, as
I'm having the problem using windows 98. 

Comment 19

[David P James]

*** Bug 191893 has been marked as a duplicate of this bug. ***

Comment 20

[Gustavo Araujo Bittencourt]

Problem persist on:
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3b) Gecko/20030204 Phoenix/0.5
Mozilla/5.0 (X11; U; SunOS sun4u; en-US; rv:1.3b) Gecko/20030203 Phoenix/0.5

Comment 21

[alanjstr]

Ok, we have enough people confirming it.  Please only post if you know what is
wrong or how to fix it or have some discussion to add.

This is OS independent and happened on or around Feb 1st.  Has anyone tracked
this down to the last known good build?  That will help narrow the window for
where we can search for checkins.

Comment 22

[Arjen Anker]

Having the same problem here, customizing toolbar, restarting, only 2 buttons left.

This is the last build *without* the problem:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3a) Gecko/20030131 Phoenix/0.5

Comment 23

[Corwin K. Watts]

Speaking of something to add... I understood from the Mozillazine post about the
new preferences panel that a change was made to how Phoenix recognizes button
images in themes. Could this change be at fault. If Phoenix is expecting only
one button per image and most themes have several buttons per image (For example
back, forward, stop, etc. images are all in one image), wouldn't that be at fault?

Comment 24

[alanjstr]

Phoenix (and Mozilla) has always allowed to have multiple buttons taken from a
single image by using -moz-image-region.  This seems to be a matter of the
buttons being removed from the toolbar and returning to the Customize Box, from
what I've been told.  

Comment 25

[Sébastien Delahaye]

*** Bug 191982 has been marked as a duplicate of this bug. ***

Comment 26

[Asa Dotzler [:asa]]

I've tracked this down and it has nothing to do with Ben's preferences changes.
Backing out changes that bz landed on the 31st for bug 189384 fixes the
problems. I'm not going to be able to debug this any further than finding the
checkins that caused it. If you want to test this yourself, or make a working
build, try backing out those changes using:
cvs update -j1.68 -j1.67 mailnews/compose/src/nsSmtpUrl.cpp
cvs update -j3.416 -j3.415 content/base/src/nsDocument.cpp
cvs update -j3.258 -j3.257 content/base/src/nsGenericElement.cpp
cvs update -j1.145 -j1.144 content/xul/document/src/nsXULDocument.h
cvs update -j1.545 -j1.544 content/xul/document/src/nsXULDocument.cpp
cvs update -j1.136 -j1.135 content/xul/content/src/nsXULElement.h
cvs update -j1.446 -j1.445 content/xul/content/src/nsXULElement.cpp
cvs update -j1.40 -j1.39 editor/ui/composer/content/editorUtilities.js

If someone wants to try to narrow it down further or figure out what part of
that change is likely to be the culprit, that would be great. What we don't need
in this bug is more "I see this bug too" or "here is a screenshot of the
toolbars not working". Unless you're actually finding the problem within this
checkin then your comments are just adding noise and potential confusion to the
bug. 

Comment 27

[Boris Zbarsky [:bzbarsky]]

OK.  So the major change that went into effect when I checked in those changes
is that the return value of getElementsByTagName is now live in XUL just like it
is in HTML/XML.  That means that the list will gain or lose elements as you move
things around in the DOM (the old list implementation was a "snapshot" of the
DOM instead of being live).

So with that in mind, the code in unwrapToolbarItems() (customizeToolbar.js)
needs to be fixed.  Right now it does:

213 function unwrapToolbarItems()
214 {
215   var paletteItems = gToolbox.getElementsByTagName("toolbarpaletteitem");
216   for (var i = 0; i < paletteItems.length; ++i) {
217     var paletteItem = paletteItems[i];    
218     var toolbarItem = paletteItem.firstChild;
... // some stuff
226     paletteItem.removeChild(toolbarItem);
227     paletteItem.parentNode.replaceChild(toolbarItem, paletteItem);
228   }
229 }

So... first off, the removeChild is not even needed -- the replaceChild will
implicitly do it.  More to the point, when you replaceChild, "paletteItem" is
removed from the list.  So perhaps that code should become:

213 function unwrapToolbarItems()
214 {
215   var paletteItems = gToolbox.getElementsByTagName("toolbarpaletteitem");
216   while ((var paletteItem = paletteItems.item(0)))    
218     var toolbarItem = paletteItem.firstChild;
... // some stuff
226     paletteItem.removeChild(toolbarItem);
227     paletteItem.parentNode.replaceChild(toolbarItem, paletteItem);
228   }
229 }

Someone try that?  (yeah, the assignment in the conditional is purposeful and as
ugly as you think it is; other more verbose variants are prettier.)

Comment 28

[Boris Zbarsky [:bzbarsky]]

Attachment: This is the code that Jack built

Comment 29

[Brendan Eich [:brendan]]

bz: alas in JS, var is a statement, not an expression, so you have to hoist the
var paletteItem; out of the loop.  But I agree that an assignment nested in the
loop condition is the right thing.  To make clear what's happening, I always
compare != null as well as parenthesizing the assignment (parens are necessary
with != null, of course; without the != null as you wrote it, they're still
necessary to avoid a strict JS warning).

/be

Comment 30

[Erik]

*** Bug 192099 has been marked as a duplicate of this bug. ***

Comment 31

[Ben Goodger (use ben at mozilla dot org for email)]

Not quite. With this patch the toolbar state does indeed restore itself when you
click "Done" - but you end up with two JS errors:

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "document.getAnonymousNodes(theToolbar) has n
o properties"]' when calling method: [nsIDOMEventListener::handleEvent]"  nsresu
lt: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "<unkno
wn>"  data: yes]
************************************************************
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "document.getAnonymousNodes(theToolbar) has n
o properties"]' when calling method: [nsIDOMEventListener::handleEvent]"  nsresu
lt: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "<unkno
wn>"  data: yes]
************************************************************

a bunch of assertions:

###!!! ASSERTION: received style change for content not in document: 'doc', file
 c:/builds/mb/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp, line 1217
8

(a series of them for each time a tooltip tries to pop up)

the toolbar loses most of its style (except the icons) (hard to describe),

and a partridge in a pear tree. 

I suspect this is related to the js errors. I can look into this this weekend
possibly. 

Comment 32

[Ben Goodger (use ben at mozilla dot org for email)]

bz, 

is it likely that any of your changes could cause document.getAnonymousNodes not
to work?

this code in the bookmarks toolbar: 

<constructor>
  var theToolbar = this;
  var resizeFunc = function (event) {
    ..
    var buttons = (document.getAnonymousNodes(theToolbar))[0].firstChild;
    ..
  }
  window.addEventListener("resize", resizeFunc, false);
</constructor>

is failing after toolbar customization causes the bookmarks toolbar bound
element to be torn out and reinserted into the document. I checked |theToolbar|
and it is still a valid 'this' - i.e. the same prototype as it was initiallay. 

Comment 33

[Boris Zbarsky [:bzbarsky]]

Hm... getAnonymousNodes just gets the childNodes through the binding.... that
should be unaffected by any of these changes.

Comment 34

[Jake]

I found an easy work-around, I did not see this one anyware yet.
(It's not a fix, just a way for users to customize their toolbar):

- add flexible spaces before any (!) button (also bofore bookmarks toolbar folder)
- press ok (stop customizing)
- restart browser

(so any toolbar should start with a flexible space, these are flex. spaces are
removed at restart).

Comment 35

[Sébastien Delahaye]

*** Bug 192446 has been marked as a duplicate of this bug. ***

Comment 36

[ironk93]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3b) Gecko/20030209 Phoenix/0.5

I know there have been some workarounds involving manual manipulation of
localstore.rdf.  I have found a point-and-click workaround that is interesting.
(Note: I only tested this on Windows XP Pro.)

1.  Customize toolbar as normal.  Do not exit phoenix after this step.
2.  After clicking done, customize the toolbar again and put duplicates of the
buttons, separators, search bar, normal/flexible spaces, etc. that you placed on
the toolbars on the toolbars to the left of the button already placed.
3.  Repeat step 2 until there is two of everything you intend to place in the
toolbars.  (Remember to keep all duplicates to one side or the other of the
corresponding original toolbar item).
4.  Close phoenix and restart.

You will notice that the toolbar customizations performed stick after this
process without any of the duplicated buttons, spaces, etc. remaining on the
toolbar.

Reproducible: always.

ps: I know this won't help much with getting the errors in code repaired except
maybe as a help with pinpointing what logical errors are occurring.

Comment 37

[David Tenser [:djst]]

*** Bug 192519 has been marked as a duplicate of this bug. ***

Comment 38

[Ben Goodger (use ben at mozilla dot org for email)]

getAnonymousNodes returns null instead of a NodeList for item id
"bookmarks-toolbar", which should really be the anonymous nodes for the binding
"bookmarks-toolbar" (in bookmarksToolbar.xml). I did a little superficial
debugging and in XBL a call is made to GetBinding providing the content node for
the <bookmarks-toolbar/> element. GetBinding returns null. !. 

hyatt?

Comment 39

[David Tenser [:djst]]

*** Bug 192623 has been marked as a duplicate of this bug. ***

Comment 40

[Pierre Chanial]

*** Bug 192624 has been marked as a duplicate of this bug. ***

Comment 41

[David Hyatt]

removeChild was deliberate.  You can't yank it.  replaceChild has never been 
patched to properly adjust things when dealing with XBL bindings.  That's why 
there's a remove and a replace in the old code.

Put the remove back in and things should work ok.

Comment 42

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to Brendan's and hyatt's comments

Now why did I never get mail for comments 29 and 31?

I filed bug 193298 on the problem hyatt mentions in comment 41

Comment 43

[jag (Peter Annema)]

This is what I ended up doing when I tried to use this code in Mozilla:

var toolbar = paletteItem.parentNode;
toolbar.insertBefore(toolbarItem, paletteItem);
toolbar.removeChild(paletteItem);

Of course, we should just fix replaceChild.

Comment 44

[Peter Roberts]

Attachment: New function to replace UnwrapToolbarItems

I'm not sure if this is relevant but I looked at the code in
customizeToolbar.js and noticed that in the function to wrap the toolbar items
it iterates through all of the customizable toolbars but when unwrapping it
doesnt.
The attachment changes the function UnwrapToolbarItems so that it iterates
through the customizable toolbars. It seems to work but I'm not familiar with
the whole code and therefore could be totally off base.
As this is my first foray into the area of bugzilla and code fixes on this type
of project forgive me if I have got it wrong but hopefully it might set the
developers off on the right track!

Comment 45

[David Lynch]

This seems to have fixed it.  (Using a toolkit.jar that gerbig on the
mozillazine forums modified according to the attachments provided, I've
customised my toolbar and restarted without any bustage.)

http://www.mozillazine.org/forums/viewtopic.php?p=44795

Comment 46

[alanjstr]

>> gerbig said:  "I've noticed one weird thing, an extra minimize button appears
after customization, but it disappears after restarting."

I'm a little uncomfortable about this patch being finalized if that happens.  

Comment 47

[David Lynch]

On further testing, it has one more problem:  only the window that Phoenix
initially creates has any modifications.  Any new windows you open after that
have the default initial-install settings.

Comment 48

[David Lynch]

Ah... ignore that last comment.  It was an extension problem.

So, only noticable problem with the patch is the strange appearance of a
minimize button on the right of the toolbar.

Comment 49

[Thamer Mahmoud]

*** Bug 193971 has been marked as a duplicate of this bug. ***

Comment 50

[John Morrison]

*** Bug 194136 has been marked as a duplicate of this bug. ***

Comment 51

[Pierre Chanial]

Comment on attachment 114407 [details] [diff] [review]
Updated to Brendan's and hyatt's comments

checked in, thanks Boris!
This patch fixes the bustage for me (after some profile cleanup)

Peter:
gToolbox.getElementsByTagName("toolbarpaletteitem");
returns a list of all the items we need.

Comment 52

[Pierre Chanial]

fixed

Comment 53

[alanjstr]

Can anyone verify the extra minimize button with a clean build?  If so, please
start a new bug and mention this one.

Comment 54

[Corey Wood]

Latest nightly fixed the problem with the buttons, but the problem with the 
dropdown autocomplete box still exists.

Comment 55

[Erik]

*** Bug 194336 has been marked as a duplicate of this bug. ***

Comment 56

[noririty]

v

Comment 57

[Simon Paquet [:sipaq]]

Taking QA Contact

Comment 58

[Simon Paquet [:sipaq]]

*** Bug 191649 has been marked as a duplicate of this bug. ***