220701 - "collapsed" is not persisted the same way that "currentset" is for custom toolbars

Status: NEW

(Toolkit :: Toolbars and Toolbar Customization, defect)

Description

[djk]

User-Agent:       Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.5b) Gecko/20030904 Firebird/0.6.1+
Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.5b) Gecko/20030904 Firebird/0.6.1+

"collapsed", "mode", and "iconsize" are not persisted the same way that
"currentset" is for custom toolbars:

I fixed this issue as a part of my patch for bug 173444.  However, that
particular part of the patch was not checked in, so this is the spinoff bug.

The only user-visable problem that this discrepency _currently_ causes is
regarding the "collapsed" toolbar attribute; a custom toolbar will not remain
collapsed.  If it becomes possible to customize "iconsize" and "mode" for
individual toolbars, persistance problems with those attributes will occur with
the custom toolbars as well.


Reproducible: Always

Steps to Reproduce:
0. Create / Use new profile
1. View -> Toolbars -> Customize...
2. Press "Add New Toolbar" button.
3. Name the toolbar (I called mine newbar).
4. Drag and Drop an icon to the new toolbar.
5. Press "Done" button.
6. View -> Toolbars, choose the newly created toolbar to make it collapsed.
7. Observe the new custom toolbar is not shown in the current window.
8. File -> New Window.

Actual Results:  
A new window appears with the newly created custom toolbar showing (uncollapsed).


Expected Results:  
A new window appears without the newly created custom toolbar showing, since it
is not showing in the first window.


Contrast persistCurrentSets() with updateIconSize() and updateToolbarMode() in
customizeToolbar.js, also contrast it to onViewToolbarCommand() in browser.js.

It has to do with the fact that the custom toolbars are created dynamically from
the ctor of the toolbarset of browser.xul (see toolbar.xml, binding id="toolbox").

"currentset" persistance is 'emulated' in an attribute of the toolbarset,
similar to this:

  <RDF:Description about="chrome://browser/content/browser.xul#customToolbars"
                   toolbar1="newbar:new-tab-button" />

The browser has an element called "customToolbars", which has attributes for
each custom toolbar the user creates.  Each attribute contains the name of the
custom toolbar and the current set of icons on the toobar, separated by a colon.
 Note that there is no corresponding "mode", "iconsize", or "collapsed"
information in this attribute.

It must be done this way, I presume, because of the aforementioned dynamic
toolbar creation.  I also imagine that's why 'normal' persistance (eg,
"collapsed") doesn't work.  See that the "mode", "iconsize", and "collapsed" are
actually persisted on the custom toolbar:

  <RDF:Description
about="chrome://browser/content/browser.xul#__customToolbar_newbar"
                   collapsed="true" />

But that info is not used because browser.xul has no element with
id='__customToolbar_newbar' when the browser is created.

My patch in bug 173444 adds "mode", "iconsize", and "collapsed" handling similar
to that of "currentset" to the custom toolbars.

I'll attach a new patch to this bug later.

David

Comment 1

[djk]

> If it becomes possible to customize "iconsize" and "mode" for
> individual toolbars, persistance problems with those attributes will occur 
> with the custom toolbars as well.

More accurately, users would see the problem if individual toolbar "mode" and
"iconsize" customization were in the UI (bug 172517).  If you modify
localstore.rdf by hand, you could customize in this way alredy, but _not_ for
custom toolbars (for reasons explained above).

Comment 2

[djk]

Attachment: persist other user-visable attributes the same way that "currentset" is persisted on custom toolbars

Comment 3

[djk]

Attachment: same as previous patch, with a fix to a call of appendCustomToolbar()

This version of the patch is the same as the first one I posted, except I
additionaly changed the call to appendCustomToolbars in customizeToolbar.js
(when initially creating the toolbar) to use the correct number of args.

I fixed some indentation problems, too.

Comment 4

[Jon Henry]

Came across this while looking for another bug. Marking NEW, since this
definitely happens and already has a patch ready.

Comment 5

[Wayne Woods]

Bug exists on Mac OS X version too.
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.6a) Gecko/20031003

Comment 6

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

+ing since there's a patch

Comment 7

[Mike Connor [:mconnor]]

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

Comment 8

[Mike Connor [:mconnor]]

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

Comment 9

[Asa Dotzler [:asa]]

p4 priority - not a blocker. if a patch materializes, please nominate for aviary
approval. 

Comment 10

[Wayne Woods]

There is a patch. It's now a year old, however.

Just need someone to check if it's still valid or if it's bit rotted.

Comment 11

[djk]

I don't have the time to mess with my firefox patches ATM; sorry.

It _would_ be great to have this bug fixed, though.  IMHO, this bug makes custom
toolbars pretty much worthless since most of their states are not persisted.

David

Comment 12

[Prakash Mehrotra]

The last update to this was before release of FF 1.0 We are now officially 1.02

Is this targetted for 1.1? This bug is really nasty. Owners, please raise its
priority.

Comment 13

[mark bokil]

(In reply to comment #12)
> The last update to this was before release of FF 1.0 We are now officially 1.02
> 
> Is this targetted for 1.1? This bug is really nasty. Owners, please raise its
> priority.

I just ran into this bug in Deer Park alpha. I created several custom toolbars,
used the view menu to hide them. Opened a new window and all the custom toolbars
become visible. Please elevate priority. This bug makes custom toolbars not very
useful. Can the patch be rolled in before 1.1 releases?

Comment 14

[Jon Henry]

I'm willing to take a crack at this bug sometime this week...if nothing else,
I'll unbitrot djk's patch :). This is definitely still happening on the 20051005
Firefox branch nightly on Win 2000.

Comment 15

[Jon Henry]

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

Comment 16

[Jon Henry]

Attachment: djk's patch, now with less bitrot!

This applies cleanly to the trunk as of today and seems to work as advertised.
Note that adding a custom toolbar will not work well until bug 265798 is fixed.

Comment 17

[Frédéric Jeanbart]

This behavior *always* hapens, not only when opening a new window after creating a new toolbar - the collapsed attribute seems to be of no use:

Steps to Reproduce:
1. Create a new toolbar, and drag one or several buttons in it;
2. View -> Toolbars, then deactivate the newly created custom toolbar. From now on, it is supposed to be hidden, which is the case in the current session
3. Close Firefox
4. Start Firefox (new session) : the toolbar is *NOT* hidden anymore, as it should be. In localstore.rdf, the collapsed attribute does not seem to be taken into account, as mentioned above

Comment 18

[Frédéric Jeanbart]

(In complement to comment #17)
> This behavior *always* hapens, not only when opening a new window after
> creating a new toolbar - the collapsed attribute seems to be of no use:
> ...
> 4. Start Firefox (new session) : the toolbar is *NOT* hidden anymore, as it
> should be. In localstore.rdf, the collapsed attribute does not seem to be taken
> into account, as mentioned above
> 

Note: the Webdeveloper extension which provides its own toolbar is the only one to have a correct behavior. I didn't dig into their code, but that extension doesn't seem to use the same FF "facility" with their toolbar (else it would behave the same way, no?)... How did they do it? Something to dig.

Comment 19

[mark bokil]

Frederic if you read through the bug report you will see all your comments have already been recorded in previous comments. The custom toolbar is different from a standard toolbar since it uses the currentset attribute to persist the list of toolbarbuttons which have been added. Webdeveloper is an extension and there is nothing unusual about the code it is using. Webdeveloper is using an overlayed toolbar with a collapsed attribute -- nothing special. The collapsed attribute for custom toolbars is just broken.

Comment 20

[Frédéric Jeanbart]

(In reply to comment #19)
> Frederic if you read through the bug report you will see all your comments have
> already been recorded in previous comments.

I've noticed that fact after reading bugs marked as duplicates of this one. Sorry for repeating them here, but consider this other fact : it has been more than a year since this problem arised (since 2003), and the bug is still there. Considering everyone knows why, how come this important functionality is still broken? I don't understand...

Anyways, I wouldn't want to put too much pressure on you guys, you're doing a great job in traking this myriad of bug reports, in building a great tool, while knowing that every one has to make a living... But isn't it the goal of a non-lucrative foundation, to pay for those who are making things happen so they can work on it with serenity, more than to please some share holder?

Best regards.

Comment 21

[mark bokil]

Frederick, I agree with your assessment that it has been a long time for this bug to stick around. I think when it comes down to it most Firefox uses don't know how to add a custom toolbar.  I don't think it is a very popular feature so I guess that makes it a low priority bug. Most users would probably add an extensions first instead of making a custom toolbar. 

Comment 22

[Frédéric Jeanbart]

O(In reply to comment #21)
> Frederick, I agree with your assessment that it has been a long time for this
> bug to stick around. I think when it comes down to it most Firefox uses don't
> know how to add a custom toolbar.  I don't think it is a very popular feature
> so I guess that makes it a low priority bug. Most users would probably add an
> extensions first instead of making a custom toolbar. 
> 

That would be an interresting survey to do, what are the functionalities users appreciate the most. Easy to gather via some form that could be available from your home page (a point form of difernet functinalities where the user just have to put a check mark or to chosse a weight factor between ket's say 1 and 5)... I think that the assertion Mozilla is making about the customization capabilities of Firefox, using this as a "market share" leverage, includes being able to manage custom toolbars, hence being able to group related functionalities together. Actually, the personal toolbar can get filled quite rapidly with the personal bookmarks folder... Customization goes a lot more beyond cosmetics (themes)! It is a choice FF has made imho, but maybe I'm wrong about this assertion...

Ok, let me ask you for one last thing then: I am using 1.5RC1 on an XP Pro machine. Until version 1.0.7, I didn't have any problem with custom toolbars. Is this a problem related with an XP platform (I was using Win2K before)? What has changed since then, could you lead me to some documentation so I can make this work manually? The fact is that I am a consultant for companies whishing to make their not only their Web Sites complyant with the W3C standard body, but also to integrate FF entreprise-wide, under the promise that I can modify the whole GUI to suit their needs (Montreal area). At the same time, my intentions are to donate a part of my revenues in this regards to the Mozilla Foundation, since everything is free with you guys, and since I want to capitalize with your magnificient work! :-) Should I change my pretentions?

Thanks and regards

Comment 23

[Jon Henry]

Comment on attachment 199497 [details] [diff] [review]
djk's patch, now with less bitrot!

Trying another reviewer, since mconnor hasn't had time to get to this.

Comment 24

[Asaf Romano (gone)]

Comment on attachment 199497 [details] [diff] [review]
djk's patch, now with less bitrot!

>Index: mozilla/browser/base/content/browser.js
>===================================================================

Why does this live in browser/ too?

>   toolbar.collapsed = aEvent.originalTarget.getAttribute("checked") != "true";
>-  document.persist(toolbar.id, "collapsed");
>+  // Persist the collapsed state of custom toolbars differently (in the toolbarset).
>+  // Need to store other persisted information as well.
>+  // (see persistCurrentSets() in customizeToolbar.js)
>+  var customIndex = toolbar.hasAttribute("customindex");

Use getAttribute (which returns |null| if the requested attribute isn't set) instead, then you could use it inside the |if (customIndex)| block

>+  if (customIndex) {
>+    // Persist custom toolbar info on the <toolbarset/>
>+    var customNumber = toolbar.getAttribute("customindex");
>+    var currentSet = toolbar.getAttribute("currentset");
>+    var mode = toolbar.hasAttribute("mode") ? toolbar.getAttribute("mode") : "";
>+    var iconSize = toolbar.hasAttribute("iconsize") ? toolbar.getAttribute("iconsize") : "";
>+    var collapsed = toolbar.hasAttribute("collapsed") ? toolbar.getAttribute("collapsed") : "";
>+    toolbox.toolbarset.setAttribute("toolbar"+(customNumber),

Get rid of extra brackets. Also, add spcaes around + operators.

>+                                     toolbar.toolbarName + ":" + currentSet +
 ":" + mode + ":" + iconSize + ":" + collapsed);

|currentSet| should be stored in a separate attribute, otherwise the parse code will be broken if the stored set has ":" in it.

>+    document.persist(toolbox.toolbarset.id, "toolbar"+customNumber);
>+  }
>+
>+  if (!customIndex) {

Use |else| instead.


>Index: mozilla/toolkit/content/customizeToolbar.js
>===================================================================

>-  gToolbox.appendCustomToolbar(name.value, "");
>+  // The new toolbar will have the same mode and iconsize as its toolbox for defaults.
>+  // It will not be collapsed.
>+  gToolbox.appendCustomToolbar(name.value, "", gToolbox.getAttribute("mode"), gToolbox.getAttribute("iconsize"), "");

Make the new parameters optional instead.

>Index: mozilla/toolkit/content/widgets/toolbar.xml
>===================================================================
>             while (toolbarset.hasAttribute("toolbar"+(++index))) {
>               var toolbarInfo = toolbarset.getAttribute("toolbar"+index);
>               var infoSplit = toolbarInfo.split(":");
>-              this.appendCustomToolbar(infoSplit[0], infoSplit[1]);
>+              // Parse out the mode, iconsize, and collapsed state also.
>+              this.appendCustomToolbar(infoSplit[0], infoSplit[1], infoSplit[2], infoSplit[3], infoSplit[4]);

This line is really too long.


>       <method name="appendCustomToolbar">
>         <parameter name="aName"/>
>         <parameter name="aCurrentSet"/>
>+        <!-- Get the mode, iconsize, and collapsed attributes also. -->

Remove this comment.

>+        <parameter name="aMode"/>
>+        <parameter name="aIconSize"/>
>+        <parameter name="aCollapsed"/>

As mentioned, they should be optional.

>         <body>
>           <![CDATA[            
>             var toolbar = document.createElementNS("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul",
>@@ -69,8 +74,14 @@
>             toolbar.setAttribute("customindex", ++this.customToolbarCount);
>             toolbar.setAttribute("toolbarname", aName);
>             toolbar.setAttribute("currentset", aCurrentSet);
>-            toolbar.setAttribute("mode", this.getAttribute("mode"));
>-            toolbar.setAttribute("iconsize", this.getAttribute("iconsize"));
>+            // Restore the mode, iconsize, and collapsed state.
>+            // Don't need to get mode or iconsize from the toolbox because
>+            // they're always persisted with the custom toolbar.
>+            toolbar.setAttribute("mode", aMode);
>+            toolbar.setAttribute("iconsize", aIconSize);
>+            toolbar.setAttribute("collapsed", aCollapsed);
>+            toolbar.setAttribute("context", this.toolbarset.getAttribute("context"));
>+            toolbar.setAttribute("class", "chromeclass-toolbar");

"class" is already set:

>             toolbar.setAttribute("context", this.toolbarset.getAttribute("context"));
>             toolbar.setAttribute("class", "chromeclass-toolbar");

Comment 25

[Asaf Romano (gone)]

Actually, I would like to see each attribute stored alone (no separators).

Comment 26

[Jon Henry]

Thank you for the review, Asaf. I will try to address your comments, although the patch is not originally mine so I don't claim to understand all the choices the original author made.

Comment 27

[djk]

It's been sooooo long since I made this patch, I've forgotten many things about it.  I'll see how many of the reviewer's comments I can address.  I still don't have time to update the patch itself, sorry.


(In reply to comment #24)
> (From update of attachment 199497 [details] [diff] [review] [edit])
> >Index: mozilla/browser/base/content/browser.js
> >===================================================================
> 
> Why does this live in browser/ too?

That bit of code is so the 'collapsed' attribute is stored for the custom toolbars when you show/hide them with the View menu.  The other things (mode, iconSize, currentSet) are there because they're placed into the same persisted attribute as 'collapsed'.


> >   toolbar.collapsed = aEvent.originalTarget.getAttribute("checked") != "true";
> >-  document.persist(toolbar.id, "collapsed");
> >+  // Persist the collapsed state of custom toolbars differently (in the toolbarset).
> >+  // Need to store other persisted information as well.
> >+  // (see persistCurrentSets() in customizeToolbar.js)
> >+  var customIndex = toolbar.hasAttribute("customindex");
> 
> Use getAttribute (which returns |null| if the requested attribute isn't set)
> instead, then you could use it inside the |if (customIndex)| block

I just copied this code from function persistCurrentSets() in the toolkit's customizeToolabr.js, with the necessary variable/DOM changes. 


> >+  if (customIndex) {
> >+    // Persist custom toolbar info on the <toolbarset/>
> >+    var customNumber = toolbar.getAttribute("customindex");
> >+    var currentSet = toolbar.getAttribute("currentset");
> >+    var mode = toolbar.hasAttribute("mode") ? toolbar.getAttribute("mode") : "";
> >+    var iconSize = toolbar.hasAttribute("iconsize") ? toolbar.getAttribute("iconsize") : "";
> >+    var collapsed = toolbar.hasAttribute("collapsed") ? toolbar.getAttribute("collapsed") : "";
> >+    toolbox.toolbarset.setAttribute("toolbar"+(customNumber),
> 
> Get rid of extra brackets. Also, add spcaes around + operators.

Forgive me, I don't understand what you mean with 'extra brackets'.  The lack of spaces is (again) because I copied from persistCurrentSets().

 
> >+                                     toolbar.toolbarName + ":" + currentSet +
>  ":" + mode + ":" + iconSize + ":" + collapsed);
> 
> |currentSet| should be stored in a separate attribute, otherwise the parse code
> will be broken if the stored set has ":" in it.

I was only extending the code that already existed.  The _current_ code would break if the stored set has ":" in it, but it wouldn't break as badly as it would with the new code.  You're right; another attribute would be the way to go.


> >+    document.persist(toolbox.toolbarset.id, "toolbar"+customNumber);
> >+  }
> >+
> >+  if (!customIndex) {
> 
> Use |else| instead.

Another artifact from copying persistCurrentSets()...


> >         <body>
> >           <![CDATA[            
> >             var toolbar = document.createElementNS("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul",
> >@@ -69,8 +74,14 @@
> >             toolbar.setAttribute("customindex", ++this.customToolbarCount);
> >             toolbar.setAttribute("toolbarname", aName);
> >             toolbar.setAttribute("currentset", aCurrentSet);
> >-            toolbar.setAttribute("mode", this.getAttribute("mode"));
> >-            toolbar.setAttribute("iconsize", this.getAttribute("iconsize"));
> >+            // Restore the mode, iconsize, and collapsed state.
> >+            // Don't need to get mode or iconsize from the toolbox because
> >+            // they're always persisted with the custom toolbar.
> >+            toolbar.setAttribute("mode", aMode);
> >+            toolbar.setAttribute("iconsize", aIconSize);
> >+            toolbar.setAttribute("collapsed", aCollapsed);
> >+            toolbar.setAttribute("context", this.toolbarset.getAttribute("context"));
> >+            toolbar.setAttribute("class", "chromeclass-toolbar");
> 
> "class" is already set:

That's probably due to the patch's age.  The original version of this patch was in bug 173444.  The setting of "class" was the solution to the "popup" issue fixed by that bug.  I guess I forgot to remove that line when moving the patch to this bug.

David

Comment 28

[Jos Vermeulen]

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

Comment 29

[Ria Klaassen (not reading all bugmail)]

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

Comment 30

[Phil Ringnalda (:philor)]

Gah. The stew of intertwingled bugs around this says it's time to fix *something*, somehow. Step one: trim the prospective mode and iconsize parts off this bug; whoever supports per-toolbar size and mode as something other than hand-editing localstore.rdf can do that.

Comment 31

[Phil Ringnalda (:philor)]

Attachment: WIP

Still a little rough around the edges, in need of testing, and the fix for bug 364111 wandered into it, but it does seem to work.

Comment 32

[Scott Field]

Almost 4 years after it was reported this bug still persists in Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1.7pre) Gecko/20070812 BonEcho/2.0.0.7pre - Build ID: 2007081205

What heppened to the patch from December?

Comment 33

[[e-mail]]

> Almost 4 years after...

Make that "Over 4 years after..." 

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.12) Gecko/20080201 Firefox/2.0.0.12

Comment 34

[Rob Griffiths]

It would be *really* nice if this could be fixed in Firefox 3 -- the ability to add a custom toolbar is useful. But if it's always springing back into existence every time you open a new window (or relaunch the app), its usefulness is greatly diminished.

Personally, I use a custom toolbar to get rid of the Home button (can we have that as a prefs option, please?), as I never use it. You can't, however, get rid of it when customizing toolbars. So I created a new toolbar, moved Home there, then hid the new toolbar. Sadly, it springs back to life any chance it gets.

-rob.

Comment 35

[Larry Steeger]

I created a custom tool bar to differentiate between development and regular browsing tools. I added various tools onto the tool bar.  After creating the tool bar, my intention was to hide it when not in development mode by using View -> Toolbars -> MyTools. I could see creating more tool bars for different use modes, to keep the browser clean and uncluttered, but this bug put a stop to that idea.

Unfortunately, as many have already described, the state of the tool bar is only remember for the current firefox window.

Firefox V2.0.0.17 and V3.0.3 both have the problem.

Thank you.

Comment 36

[RenegadeX]

Attachment: Screenshot of issue

As per Faaborg via a recent Mozillazine thread asking for Firefox 3.1 'polish'
suggestions (http://forums.mozillazine.org/viewtopic.php?f=23&t=938165) - I'm
adding him to the CC list and requesting someone with Bug permissions to add
the following Whiteboard terms to this bug (less single quotes on each):
'[polish-hard]', '[polish visual]', and '[polish-high-visibility]'. Not that we really need it but as it was requested, I'm also adding a screenshot of the issue.

Faaborg - if need be, pls add Whiteboard terms to  Bug 215489 -  'User-created toolbars don't remember on/off settings' and its dependencies.

Comment 38

[Philip Chee]

SeaMonkey uses hidden instead of collapsed. I have a WIP patch that persists the hidden attribute of custom toolbars in SeaMonkey. See <https://bugzilla.mozilla.org/attachment.cgi?id=354648&action=edit>. Look for |function persistCustomToolbar(toolbar)| in that patch. I also persist the icon size and button mode as well which I don't think Firefox does either.

Comment 39

[Brandon Mitchell]

So this has been fixed in SeaMonkey? Can the final version (v1.7) of Philip Chee's patch <https://bugzilla.mozilla.org/attachment.cgi?id=358618&action=edit> for the SeaMonkey bug (Bug 471372) be adapted to Firefox? Anyone still paying attention to this?

Comment 43

[MrHatken]

Is this really still a bug?  Why is there an option to create a custom toolbar if one cannot persist it's state (visibility primarily)?  Am I missing some other approach?

Comment 45

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

(bug 215489 also had some patches)

Comment 48

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is relatively low, S3. However, the bug has 12 duplicates and 20 votes.
:Gijs, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Comment 49

[BugBot (nomail) [:suhaib / :marco/ :calixte]]

The last needinfo from me was triggered in error by recent activity on the bug. I'm clearing the needinfo since this is a very old bug and I don't know if it's still relevant.