220681 - New window uses small icons even though original window has large icons

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect, P2)

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+

New window uses small icons even though original window has large icons:

If iconsize="small" is in localstore, and then you switch to large icons, new
windows you launch will still have small icons.  This condition continues until
you close a window that has large icons, after which new windows will have large
icons.

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 issue, I believe, was first mentioned in bug 171731 comment 1 and bug 171731
comment 2.


Reproducible: Always

Steps to Reproduce:
0. Create / Use new profile.
1. Observe localstore.rdf has no toolbar info in it.
2. View -> Toolbars -> Customize...
3. Uncheck "Use Small Icons" checkbox.
4. Press "Done" button.
5. Observe localstore.rdf has iconsize="" for browser.xul#toolbar-menubar,
#navigator-toolbox, #PersonalToolbar, and #nav-bar
6. View -> Toolbars -> Customize...
7. Check "Use Small Icons" checkbox.
8. Press "Done" button.
9. Observe localstore.rdf has iconsize="small" for the aforementioned elements.
10. View -> Toolbars -> Customize...
11. Uncheck "Use Small Icons" checkbox.
12. Press "Done" button.
13. Observe localstore.rdf has no toolbar iconsize info in it.

So, even though the toolbar icons are currently large (or rather, 'not small'),
there is no indication in localstore.rdf that this is their state.

14. File -> New Window

Actual Results:  
A new window appears with small icons.  This is because there is no information
currently in localstore.rdf, and the default for the toolbox and toolbars in
browser.xul is iconsize="small".


Expected Results:  
A new window appears with large icons (the same size that the first window has).


The problem happens because the setAttribute function in customizeToolbar.js
will _remove_ the attribute if null is passed as the value, and null is used to
represent a switch to "large" icons in updateIconSize(aUseSmallIcons):

  var val = aUseSmallIcons ? "small" : null;
  setAttribute(gToolbox, "iconsize", val);

The issue is worked around if we s/null/"large"/ above, so that the custom
setAttribute() does not remove the attribute.

Alternately (though I have not yet tested this), we could use the DOM's
setAttribute instead of the custom function:  gToolbox.setAttribute("iconsize",
val); etc...

I'll attach a patch later.

David

Comment 1

[djk]

> The problem happens because the setAttribute function in customizeToolbar.js
> will _remove_ the attribute if null is passed as the value, and null is used
> to represent a switch to "large" icons in updateIconSize(aUseSmallIcons):

OK, this is sort of a lie.

I changed from the custom setAttribute function to the DOM version instead, and
got the same buggy results.

This prompted me to look closer at exactly what was being stored in
localstore.rdf, and when.

Now, I think that the real 'problem' is that if you try to persist a blank
(null) value _when a value is already persisted_, the persistance is deleted. 
If you persist a null value when there is _not_ a value already persisted, the
null value _is_ persisted!

This seems to explain why the first 'switch' to large icons actually works WRT
new windows.  There is no persistance, then we switch to persisting "" (large),
and that works.  Then, we switch back to small icons (persist "small", which
also works).  Finally, attempting to persist the "" (large) value again (while
currently persisting "small") removes the persistance this time.

The workaround of s/null/"large"/ in the line:
  var val = aUseSmallIcons ? "small" : null;
will work, because we would not attempt to persist a null value anymore.

David

Comment 2

[djk]

Attachment: avoid persisting a null value for iconsize

Comment 3

[José Jeria]

Dont know if this is the correct bug, I have use big icons, but i get small
icons when I
1) righ-click to customize
2) click on "use small icons" to uncheck it, then click it again
3) restart browser --> icons are small

Comment 4

[Mike Connor [:mconnor]]

don't reassign to someone else, if you're working on the bug, you should be the 
one who its assigned to.

also, this seems sane and simple enough, I'll get to this during the week.

Comment 5

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

+ing since there's a patch

Comment 6

[djk]

The reason I reassigned to Ben was that I don't have the time or setup available
ATM to update / revise the patch.

Thanks for taking a look at it.

David

Comment 7

[Mike Connor [:mconnor]]

Comment on attachment 132418 [details] [diff] [review]
avoid persisting a null value for iconsize

moving review to myself so I remember to do this.

Comment 8

[Mike Connor [:mconnor]]

fixed, branch and trunk

Comment 9

[Mike Connor [:mconnor]]

done, sorry about the incredibily long delay! :)