173444 - additional toolbars show up on popup windows

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Jason Kersey]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.2b) Gecko/20020927 Phoenix/0.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.2b) Gecko/20020927 Phoenix/0.3

hyatt wanted me to file this, and mentioned something about setting chromeclass
stuff on user created toolbars

Reproducible: Always

Steps to Reproduce:
1.add another toolbar so you have menubar, navigation, new tooblar, bookmarks bar
2.disable popup blocking
3.go to www.iowrestling.com

Actual Results:  
popup windows open with toolbar(s) at the top of them

Expected Results:  
popups should have no toolbars

Comment 1

[Will Levine]

in Mozilla/5.0 (Windows; U; Win95; en-US; rv:1.2b) Gecko/20021014 Phoenix/0.3 
the following javascript command:

window.open("http://foo.com","foo","notoolbars")

does not have the default toolbars or menubar but does have the custom toolbar.

Comment 2

[Will Levine]

i get the same results on latest nightly build: Mozilla/5.0 (Windows; U; Win95;
en-US; rv:1.2b) Gecko/20021020 Phoenix/0.3

Comment 3

[Lance L. Forest]

Same results, W2K SP3.
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.2b) Gecko/20021020 Phoenix/0.3

Comment 4

[Simon Paquet [:sipaq]]

Confirming bug. I can reproduce this with my 10/21/2002 build under W2k.

Comment 5

[sammy_c]

 Confirmed with http://www.xulplanet.com/downloads/prefbar/ installed. 

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

Comment 6

[djk]

Attachment: patch for popups (and other user-defined toolbar fixes)

I added a class of 'chromeclass-toolbar' to the user-defined toolbars, which
fixes the fact that those toolbars show up in popup windows.  (Thanks for the
hint about chromeclass in the bug report.  That pointed me in the right
direction.)

I also fixed it so that user-defined toolbars persist their collapsed state, as
well as their mode and iconsize.  That way, collapsed user-defined toolbars
will stay that way, even through new windows and restarts.  The mode and
iconsize persistance is there in case each toolbar can ever have its own
mode/iconsize (bug 172517).

A final little (1-line) fix wormed its way into this patch:  If the permanent
toolbars start out as small icons, you customize them to large icons, and then
open a new window, they revert back to small icons.  That may be bug 171731
(especially bug 171731 comment 1 and bug 171731 comment 2).

Comment 7

[[e-mail]]

Comment on attachment 118430 [details] [diff] [review]
patch for popups (and other user-defined toolbar fixes)

You should probably delete old code rather than commenting it out.

Comment 8

[djk]

neil:
Oh yeah, I suppose I ought to do that.

When hacking on a bug, I don't like to remove code outright.  But for a patch,
you're right, gotta remove that code.  I'll post a new patch as soon as I get
the chance.

Comment 9

[djk]

Attachment: removal of commented out code

Comment 10

[Simon Paquet [:sipaq]]

Taking QA Contact

Comment 11

[David Hyatt]

Fixed.  I only addressed the popup issue.  If you get the rest of the patch into bugs 
describing each problem, it will be more digestible.  For example, I am not sure why the 
normal collapsing persistence doesn't work for custom toolbars and don't want to add 
special code unless I'm sure it's really needed.

Comment 12

[Mike Connor [:mconnor]]

Verified using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5b)
Gecko/20030817 Mozilla Firebird/0.6.1+

Comment 13

[djk]

> Fixed.  I only addressed the popup issue.  If you get the rest of the patch
> into bugs describing each problem, it will be more digestible.  For example, I
> am not sure why the normal collapsing persistence doesn't work for custom
> toolbars and don't want to add special code unless I'm sure it's really
> needed.

First, David, I'm sure you know much of what I will say here, but I'm going to
say it so that you can point out any errors in _my_ thinking. :-)


There are 2 problems the patch fixes, besides the popup issue:

1) A little bug where if we switch from small to large icons, the permanent
toolbars don't keep their setting if one opens a New Window before localstore is
written. (bug 171731 comment 1 and bug 171731 comment 2)

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():

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

Since the value becomes null, a new window relies on the default attribute on
the xul element, which happens to be "small" in browser.xul.  Changing the null
to "large" in the conditional statement fixes things.
 

2) "collapsed", "mode", and "iconsize" are not persisted the same way that
"currentset" is for custom toolbars. (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" />

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"
                   iconsize="small"
                   collapsed="true" />

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

The rest of my patch adds "mode", "iconsize", and "collapsed" handling similar
to that of "currentset" to the custom toolbars.


I'll open some bugs and update the patches if I ever get the time...  Someone
feel free to do so for me.

Thanks,
David

Comment 14

[djk]

bug 220681 filed about the iconsize (issue 1 of comment 13).

Comment 15

[djk]

bug 220701 filed about the "collapsed", "mode", and "iconsize" attributes of
custom toolbars (issue 2 of comment 13).