626382 - Simplify toolbarbutton stylesheet requirements to make life easier for extensions

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[:Paolo Amadini]

Attachment: Set "wantediconsize", "wantedicondimensions" and "wantediconmonochrome" based on the current theme

+++ This bug was initially created as a clone of Bug #616472 +++

As Dan Mosedale explained in bug 616472 comment 41, an important half of
the issue originally discussed there is to allow extension authors to use
simple stylesheet rules to select which icon size and color should be used
for their toolbar buttons, instead of relying on complex host application
detection in chrome.manifest.

An additional win would be for the stylesheet to choose whichever artwork is
most suited to the current theme, regardless of the platform on which the
browser is executing. For example, there is a custom Mac OS X theme that
might want monochrome icons like the default "pinstripe" theme for Mac.
This is very easy to achieve using the detection technique suggested by
Markus Stange in bug 616472 comment 37.

Using the simple test case provided by Nils Maier (thank you very much!)
in bug 616472 comment 48, I wrote a patch that implements this detection
technique. It's quite polished except for these known issues:
 - "Wanted style" attributes are not yet applied to the customization palette.
 - I haven't tested it on platforms other than Windows, although I've tested
    it with some custom themes.
 - Contains debugging code that outputs detected values to the Error Console.
 - The new Toolkit toolbar customization event named "style" could use an
    automated test case.
 - I'm not sure if and what other automated tests might be needed.

Here is how an extension's stylesheet would appear. Note that all the new
styling-specific attribute names begin with a common prefix, currently
"wantedicon...", but I'm open to suggestions.





/* --- MANDATORY ICONS --- */

/* Basic (24x24) icon */
#my-button {
  list-style-image: url(icon24.png);
}

/* Small (16x16) icon. The iconsize check is for FF3 compatibility only. */
toolbar[wantediconsize="small"] #my-button,
toolbar[iconsize="small"] #my-button {
  list-style-image: url(icon16.png);
}

/* --- OPTIONAL ICONS --- */

/* Small (16x16) monochrome icon. The default on Mac. */
toolbar[wantediconsize="small"][wantediconmonochrome] #my-button {
  list-style-image: url(icon16-monochrome.png);
}

/* Extra large (32x32) icon. Only for custom themes. */
toolbar[wantediconsize="x-large"] #my-button {
  list-style-image: url(icon32.png);
}

/* Custom sized (example: 18x18) icon. Only for custom themes.
   Note that this icon will not be used if the theme wants
   icons that are 20x20. However, in this case we still have
   wantediconsize="small" that allows us to use an icon
   that fits in the available space. */
toolbar[wantedicondimensions="18x18"] #my-button {
  list-style-image: url(icon18.png);
}






Note that extensions that handled the disabled or hover button states can
now check if the "wantediconsize" attribute is set and, if this is the case,
avoid to provide specific images for that case. I also considered setting
a "wantediconwithouteffects" boolean attribute for that case, but maybe
it's just overkill.

Firefox 3 for Mac still needs the add-on to draw the whole button broder and
state, like before. Extensions that want to support this case must do it like
they did previously.

Comment 1

[Nils Maier [:nmaier]]

I'd simply use some #ifdef XP_* and leave the detection stuff out.
Theme authors had to support default sizes in some way in the past and will still have to support default sizes in the future, as it cannot be expected that extensions will provide x-large icons or any of the detected custom sizes, so there isn't really much of a use case of wantedicondimensions and/or wantediconsize="x-large".

Anyway, wantediconsize vs. iconsize seems confusing (e.g. the primary toolbar would have iconsize=large and wantediconsize=small?!)
Better use another name. I opted for compacticons in https://github.com/nmaier/compatible-icons which admittedly isn't that great either.

Comment 2

[Pardal Freudenthal (:ShareBird)]

(In reply to comment #1)
> Theme authors had to support default sizes in some way in the past and will
> still have to support default sizes in the future
Theme authors have always been free to choose their own sizes of icons and to interpret what "large" or "small" means in terms of size. And this was very important. See, for example, the contribution of the excellent work from Ed Hume with his large themes for people with visual impairments.
My themes have always had 32x32px images for large icons and 24x24px for small. I was able to solve all alignment issues with extensions icons. So, everything works just fine in the way it is.
I'm a little worried how this bug can impact third-party themes...

Comment 3

[CatThief]

Rather than adding new conditions such as wantedicondimensions, wantediconsize, or wantediconmonochrome, wouldn't a less complicated approach be to just use 16px icons for the default theme and include the margins needed for toolbarbutton-icon inside the theme's CSS -- 1px for Windows, 2px for Darwin, none for Linux?  Extensions would simply provide the 16px image needed for all platforms with an additional 24px image for large icons mode in Linux.

Custom stylesheets and a few extra lines inside the chrome.manifest to handle separate platforms really are no big deal, and the inclusion of separate platform-specific images if desired is not a big deal as well.

This approach would also ensure backward compatibility for earlier Firefox versions and an undesirable impact on third-party themes would be averted.

Comment 4

[:Paolo Amadini]

(In reply to comment #1)
> I'd simply use some #ifdef XP_* and leave the detection stuff out.

I'd agree with you if we were forced to do things this way because the
detection code was long and difficult to write, to test, or affected
performance unavoidably and badly. But I don't think it's the case
(I've not made performance measurements but even if Twinopen was
affected by the current patch we could easily fallback to persisting
the styling attributes).

Instead, I think that this solution to the problem at hand has the same
impact as the build-time options, and likewise needs someone to test it
on all platforms. But it has the additional advantage of considering the
default theme "just another theme". In other words, with build-time
options, things would not work as expected as soon as you want use the
custom "Mac OS X" theme on Windows: monochrome icons would not be used
even if an add-on author took the time to create one.

> Theme authors had to support default sizes in some way in the past and will
> still have to support default sizes in the future, as it cannot be expected
> that extensions will provide x-large icons or any of the detected custom
> sizes,

True.

> so there isn't really much of a use case of wantedicondimensions and/or
> wantediconsize="x-large".

Not exactly a consequence of the above. These attributes give themes the
freedom to communicate to extensions that they have room for an icon of
a specified size, even if they don't impose that width on the icon. No
add-on currently provides a 32x32 icon simply because there's no way for
themes to tell them they have room to host icons of this size.

(In reply to comment #2)
> See, for example, the contribution of the excellent work from Ed
> Hume with his large themes for people with visual impairments.
> My themes have always had 32x32px images for large icons and 24x24px
> for small.
> I was able to solve all alignment issues with extensions icons.

To me, this seems a good use case for wantediconsize="x-large" and even
wantediconmonochrome, for people with visual impairments. You still
have to handle default sizes, but then you enable high-quality add-ons
to use bigger button icons that suit more naturally with your theme.

> I'm a little worried how this bug can impact third-party themes...

The current situation is that, without this detection patch, every add-on
updated for Firefox 4 will use an icon that is no bigger than 16x16 on
Windows, no matter what you do in your theme. You might easily see your
32x32 icons for default buttons near 16x16 icons for add-ons.

With this patch, if you don't make changes to your custom theme,
everything should work as expected and add-ons updated for Firefox 4
will use the correct icon size for your theme on any platform.

You can then update your theme, if you want, to request monochrome
icons (like I did in the default Mac theme) or a bigger icon size.

(In reply to comment #1)
> Anyway, wantediconsize vs. iconsize seems confusing (e.g. the primary toolbar
> would have iconsize=large and wantediconsize=small?!)
> Better use another name. I opted for compacticons in
> https://github.com/nmaier/compatible-icons which admittedly isn't that great
> either.

Maybe using boolean attributes for standard sizes would make the rules
more readable, what do you think? I'm dumping some ideas here...

toolbar[want16x16] #my-button
toolbar[style32x32] #my-button
toolbar[icons24x24][monochrome] #my-button
toolbar[styledimensions="18x18"][stylemonochrome] #my-button
toolbar[iconsdimensions="18x18"] #my-button

Comment 5

[:Paolo Amadini]

(In reply to comment #3)
> just use
> 16px icons for the default theme and include the margins needed for
> toolbarbutton-icon inside the theme's CSS

See above, this won't work very well with themes that want to use 24x24 or
32x32 icons in their main toolbar - on Windows, extensions would still use
their 16x16 icons even if they have 24x24 artwork!

Comment 6

[CatThief]

(In reply to comment #5)
> (In reply to comment #3)
> > just use
> > 16px icons for the default theme and include the margins needed for
> > toolbarbutton-icon inside the theme's CSS
> 
> See above, this won't work very well with themes that want to use 24x24 or
> 32x32 icons in their main toolbar - on Windows, extensions would still use
> their 16x16 icons even if they have 24x24 artwork!
I'm not understanding why is this an issue now since size differences have always been the case in the past wherever third-party themes deviate from the default. Users are not expecting the buttons in extensions to match.

Comment 7

[:Paolo Amadini]

(In reply to comment #6)
> > See above, this won't work very well with themes that want to use 24x24 or
> > 32x32 icons in their main toolbar - on Windows, extensions would still use
> > their 16x16 icons even if they have 24x24 artwork!
> 
> I'm not understanding why is this an issue now since size differences have
> always been the case in the past wherever third-party themes deviate from the
> default. Users are not expecting the buttons in extensions to match.

I don't know the expectations of other users, to me a 16x16 add-on icon between
other 32x32 buttons sounds weird. In the past, the same issue existed but it
was a 24x24 icon between 32x32 icons, a less noticeable size difference.

My guess is that theme authors in general haven't had the opportunity to test
this case in practice because most add-on authors haven't (rightly) updated
their add-ons with the chrome.manifest tweaks yet. If you test a FF4 theme
that uses large icons with a FF3 add-on, your theme still looks fine.

Then, strictly speaking, the goal here is to simplify extension stylesheets,
but decoupling the styling attributes from the functional attributes so that
things work fine across all platforms and themes is an easy win.

Comment 8

[CatThief]

(In reply to comment #7)
> Then, strictly speaking, the goal here is to simplify extension stylesheets,
> but decoupling the styling attributes from the functional attributes so that
> things work fine across all platforms and themes is an easy win.

But if images are resized to match the theme, won't there be issues with their perceived sizes?  Forgetting third-party themes for a moment, let's use the default theme for Windows as an example.  Its images are 18px, but each one is essentially a smaller image within a 1px margin.  If an extension contains an image that is 16px and it is resized to 18px, won't it appear larger than the theme's other images?  Or will the theme know to keep it at 16px and instead apply a 1px margin to it?

Comment 9

[Pardal Freudenthal (:ShareBird)]

> (In reply to comment #2)
> > See, for example, the contribution of the excellent work from Ed
> > Hume with his large themes for people with visual impairments.
> > My themes have always had 32x32px images for large icons and 24x24px
> > for small.
> > I was able to solve all alignment issues with extensions icons.
> 
> To me, this seems a good use case for wantediconsize="x-large" and even
> wantediconmonochrome, for people with visual impairments. You still
> have to handle default sizes, but then you enable high-quality add-ons
> to use bigger button icons that suit more naturally with your theme.
> 
> > I'm a little worried how this bug can impact third-party themes...
> 
> The current situation is that, without this detection patch, every add-on
> updated for Firefox 4 will use an icon that is no bigger than 16x16 on
> Windows, no matter what you do in your theme. You might easily see your
> 32x32 icons for default buttons near 16x16 icons for add-ons.
> 
> With this patch, if you don't make changes to your custom theme,
> everything should work as expected and add-ons updated for Firefox 4
> will use the correct icon size for your theme on any platform.
> 
> You can then update your theme, if you want, to request monochrome
> icons (like I did in the default Mac theme) or a bigger icon size.
> 

Thank you Paolo for clarifying this!
I have to say I didn't investigate the patch further ("real life" sometimes demands full attention). Sorry for that...

Actually the patch looks great!

I did apply it on a recent nightly build and it didn't work for me as expected though. My issue is that I'm using for a long time a similar approach like I've proposed on Bug 347661. I don't set width and height for toolbar buttons icons to avoid that they stretch... So I think those icons must be inside a container. In this way I can set height to the container and align them vertically solving the issues I had with smaller extensions icons:

/*:::::: Hack for 3d party extensions icons ::::::*/

toolbox > toolbar[customizable]:not([id="TabsToolbar"])/*FF3.7*/ > toolbarbutton:not([type]):not([id^="custombuttons-button"]):not([newMail]),/*newMail - GMail Manager extension*/
toolbox > toolbar[customizable] > toolbarbutton toolbarbutton:not([type]):not([newMail]),
toolbox > toolbar[customizable] > toolbarbutton[type="checkbox"],
toolbarpaletteitem toolbarbutton{
  -moz-binding: url("chrome://global/skin/toolbarbutton.xml#toolbarbutton-image");
}

.image-container {
  -moz-box-align: center;
}

toolbar toolbarbutton > .image-container {
  height: 32px;
}

toolbar[iconsize="small"] toolbarbutton > .image-container {
  height: 24px;
}

and my toolbarbutton.xml binding looks like:

<!-- Binding to fix bug 347661 -->

  <binding id="toolbarbutton-image" 
           extends="chrome://global/content/bindings/toolbarbutton.xml#toolbarbutton">
    
    <content>
      <children includes="observes|template|menupopup|tooltip|panel"/>
      <xul:box class="image-container" flex="0">
        <xul:image class="toolbarbutton-icon" xbl:inherits="validate,src=image,toolbarmode,buttonstyle"/>
      </xul:box>
      <xul:label class="toolbarbutton-text" crop="right" flex="1"
                 xbl:inherits="value=label,accesskey,crop,toolbarmode,buttonstyle"/>
    </content>
  </binding>

In this way the function fails to get imageElement since themeDetector is not the parent from .toolbarbutton-icon.

I can simply fix this adding an exception to don't bind #theme-detection-button and set width and height for its icon.

I hope extension authors will provide at least 24x24px for the "large" icon size...

Just a side note: I'm wondering if Bug 302952 would impact this bug?

Comment 10

[:Paolo Amadini]

(In reply to comment #9)
> My issue is that I'm using for a long time a similar approach like I've
> proposed on Bug 347661. I don't set width and height for toolbar buttons icons
> to avoid that they stretch... So I think those icons must be inside a
> container. In this way I can set height to the container and align them
> vertically solving the issues I had with smaller extensions icons:

Using a custom XBL binding for this purpose is a clever solution!

> In this way the function fails to get imageElement since themeDetector is not
> the parent from .toolbarbutton-icon.

Probably the issue is that, under some conditions, XBL bindings aren't
applied to hidden elements, thus getAnonymousElementByAttribute fails
even if your binding has an element of the correct class.

> I can simply fix this adding an exception to don't bind
> #theme-detection-button and set width and height for its icon.

Yes, I think this is the correct solution. For reference, the rules for
communicating your preferred icon size would be similar to these ones:

#theme-detection-button > .toolbarbutton-icon {
  width: 24px;
  height: 24px;
}

toolbar[iconsize="small"] #theme-detection-button > .toolbarbutton-icon {
  width: 16px;
  height: 16px;
}

> I hope extension authors will provide at least 24x24px for the "large" icon
> size...

As long as they have a custom theme where they can test it, I bet all
add-on authors that aim to a certain quality will do :-)

> Just a side note: I'm wondering if Bug 302952 would impact this bug?

As far as I can tell, now the "iconsize" attribute is set on all toolbars,
even when it matches the default value in a clean profile. Can you still
reproduce that bug on the latest beta or nightly? Maybe it's already solved
and you can close it.

Comment 11

[patrickjdempsey]

How would this work with the toolbars which are being artificially forced into small icons mode, especially if a non-default theme chose not to honor that to keep consistency with previous versions?  In Firefox 3.6, buttons in the menu bar, navigation toolbar, any custom toolbars, and the bookmarks toolbar all obey the iconsize value.  In 4.0, only the navigation toolbar and custom toolbars obey the iconsize value, with the menu bar, bookmarks toolbar, tabs toolbar, and addons bar all being set to small icons regardless of the user setting.  However, a theme could still choose to display large icons on these toolbars in large icons mode.

Comment 12

[:Paolo Amadini]

To clarify, this patch separates the set of attributes that represent the
toolbar _configuration_ from the set of attributes that represent the
_actual size_ and style of toolbar buttons, which is what is required
for add-ons to choose the right icon in a simple way.

In the end there will be three groups of toolbar attributes. I'm writing
about all of them here (although only some of them are new) to provide a
preliminary structured documentation of both the patch and what I found
from reading the internal code, since I couldn't find this information
documented anywhere else.



=== Existing purely functional attributes ===

defaultmode="icons"|"text"|"full"|unspecified

  Specifies the default display mode of the toolbar on new profiles.

lockmode="true"|unspecified

  If "true", indicates that the display mode preference in the toolbar
  customization dialog has no effect on this toolbar. The display mode
  is locked to the default value.

defaulticonsize="large"|"small"|unspecified

  Specifies the default "large or small" preference of the toolbar on
  new profiles. The actual icon dimensions used depend on the theme.

lockiconsize="true"|unspecified

  If "true", indicates that the "large or small" preference in the toolbar
  customization dialog has no effect on this toolbar. The "large or small"
  preference is locked to the default value.

These attributes are generally _not_ checked by CSS rules in themes.



=== Existing functional attributes used by the theme ===

mode="icons"|"text"|"full"

  Specifies the effective display mode preference of the toolbar.

iconsize="large"|"small"

  Specifies the effective "large or small" preference for this toolbar.

The actual value of these attributes is a combination of the above
"default..." and "lock..." values and the current user preferences
in the customization dialog. The theme uses these values to set the
size and style of all toolbar elements, like button borders, image
container space, image actual size, and so on.



=== New styling attributes ===

[The following names are one of various possible proposals]

style32x32="true"|unspecified

  If "true", indicates that a 32x32 icon is best suited for this toolbar.

style24x24="true"|unspecified

  If "true", indicates that a 24x24 icon is best suited for this toolbar.

style16x16="true"|unspecified

  If "true", indicates that a 16x16 icon is best suited for this toolbar.
  This is also set in case the icon size wanted by the theme cannot be
  detected for any reason.

styledimensions="WxH" (example: styledimensions="24x30")

  Actual recommended icon dimensions for themes with custom icon sizes.
  For example, an accessible theme might want a 48x48 monochrome icon.
  Note that most add-ons don't provide such non-standard icons at present.
  One of style16x16, style24x24 or style32x32 will be set regardless of
  the value of this attribute.

stylemonochrome="true"|unspecified

  If "true", indicates that a monochrome (Mac style) icon is best suited
  for this toolbar. This attribute can be present or not, independently
  from the attributes representing the icon size, although the default
  Mac theme that requires monochrome icons uses 16x16 icons.

These attributes will be generally used by extensions to choose the correct
icon size and style. Extensions that want to be compatible with applications
other than Firefox 4 should also include rules based directly on the
functional attributes. Extensions for Firefox 4 only need to define
rules based on these attributes.

Comment 13

[patrickjdempsey]

Am I correct in assuming that non-"standard" icon sizes would be rounded to the nearest choice?  For instance a theme with 22x22 icons would be rounded up to 24x24, and 18x18 would be rounded down to 16x16? Also, is it even necessary to discuss the width of icons at all?  All that is really important to the theme is the height, as that determines the vertical padding and margins and therefore the height of the toolbar.  In fact, themes that assume all buttons to be square may run into problems with some extensions which use non-square buttons, for instance Toolbar Buttons has a few.

Comment 14

[:Paolo Amadini]

(In reply to comment #13)
> Am I correct in assuming that non-"standard" icon sizes would be rounded to the
> nearest choice?  For instance a theme with 22x22 icons would be rounded up to
> 24x24, and 18x18 would be rounded down to 16x16?

With regard to custom themes, this patch is optimized for themes that don't
stretch the icon, but add padding around it if necessary (see comment 9). If
such a theme uses a custom icon size, with a rule similar to this one...

#theme-detection-button > .toolbarbutton-icon {
  width: 22px;
  height: 22px;
}

...the patch would set style16x16="true", since it is the only icon size
that fits in the available space. Custom themes that prefer add-ons to use
their 24x24 icon, stretching it to 22x22 behind the scenes, can simply
declare 24x24 in the above rule, and 22x22 in the actual button rules.

> Also, is it even necessary to
> discuss the width of icons at all?  All that is really important to the theme
> is the height, as that determines the vertical padding and margins and
> therefore the height of the toolbar.  In fact, themes that assume all buttons
> to be square may run into problems with some extensions which use non-square
> buttons, for instance Toolbar Buttons has a few.

Add-ons are free to use icons of any size in response to the styling
attributes if appropriate. A good theme design is one that accomodates
icons of every size, of course. However, for compatibility with default
themes and most custom themes, add-on authors should provide at least
a 24x24 icon when the style24x24 attribute is set, and a 16x16 icon
when style16x16 is set, and this is why these attribute names indicate
both the width and the height.

Comment 15

[:Paolo Amadini]

Attachment: Polished patch

Polished final patch. Note that the event required for updating the styles
on customization might still want a simple test case. To test the theme,
download Nils' add-on from https://github.com/nmaier/compatible-icons
and replace "overlay.css" with the following stylesheet:

@namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");

.compatible-icons-button {
  list-style-image: url(icon24.png) !important;
}

toolbar[style16x16] .compatible-icons-button,
#palette-box[style16x16] .compatible-icons-button,
toolbar[iconsize="small"]:not([styledimensions]) .compatible-icons-button {
  list-style-image: url(icon16.png) !important;
}

toolbar[style16x16][stylemonochrome] .compatible-icons-button,
#palette-box[style16x16][stylemonochrome] .compatible-icons-button {
  list-style-image: url(icon16-monochrome.png) !important;
}

Comment 16

[:Paolo Amadini]

I'd like to avoid that this very important bug slips through the cracks again.

This is the last missing step, after bug 616472, for making life *much*
easier to add-on developers that want to create toolbar buttons. With
this patch, the stylesheet in comment 15 is everything that is required
to support Firefox 4 on every platform and even with custom themes.
Otherwise, complex "chrome.manifest" contortions would be necessary,
and the explanation would be probably as long as this blog post:

http://blog.mozilla.com/addons/2010/12/02/toolbar-buttons-in-firefox-4/

This also fixes a very important limitation: without this patch, custom
themes on Windows and Mac wouldn't be able to host add-on icons bigger
than 16x16 pixels. Moreover, chrome.manifest tweaks can't be tested
unless you have the target platform available, while with this patch
a custom theme would be enough to test all the icon sizes and colors.
For example, I could install a custom Mac OS X theme on Windows to
test my monochrome icon.

At present:
 * There is a polished patch ready for review
 * New attributes have been documented thoroughly in advance
 * Most edge-cases and potential issues have been discussed and worked out
 * The patch is small and should not require a lot of review bandwidth
 * I'm available in the next days to address review comments if necessary

I think we should mark this bug as "hardblocker" to avoid that it is
forgotten. A lot of add-on developers showed interest and participated
in the preliminary discussions, and Jorge of the Mozilla Add-ons website
has been very helpful with moving this forward. It seems that we missed
code freeze for this beta, but missing even the next beta because the bug
is forgotten would be a waste of time and a loss for all the volunteers
that have participated in this discussion.

Comment 17

[Markus Stange [:mstange]]

I've pushed this to the tryserver as http://tbpl.mozilla.org/?tree=MozillaTry&rev=ab744b0cb227 and the twinopen numbers are consistently in the upper half of the graphs' swing ranges - i.e. they hint at a very small regression (not more than 2ms) but are not conclusive evidence.

Comment 18

[:Paolo Amadini]

(In reply to comment #17)
> I've pushed this to the tryserver as
> http://tbpl.mozilla.org/?tree=MozillaTry&rev=ab744b0cb227 and the twinopen
> numbers are consistently in the upper half of the graphs' swing ranges - i.e.
> they hint at a very small regression (not more than 2ms) but are not conclusive
> evidence.

Thank you very much for running this test! The regression seems very small
indeed, hinting at the fact that implementing attribute persistence is not
necessary. If I understand correctly, this is one of the graphs showing the
results of these tests on Windows, compared with mozilla-central builds
(look for changeset ab744b0cb227):

http://graphs.mozilla.org/graph.html#tests=[[17,23,886],[17,1,886]]

Comment 19

[Markus Stange [:mstange]]

(In reply to comment #18)
> If I understand correctly, this is one of the graphs showing the
> results of these tests on Windows, compared with mozilla-central builds

Correct.

Comment 20

[LegNeato]

I don't think we'd block a release on this (as the functionality works), but making life easier for extension developers is a noble goal and this has a path that is close to ready -> adding back softblocker.

Comment 21

[:Paolo Amadini]

(In reply to comment #20)
> I don't think we'd block a release on this (as the functionality works), but
> making life easier for extension developers is a noble goal and this has a path
> that is close to ready -> adding back softblocker.

Thank you for evaluating the request. In fact, what I'm actually interested in
is not having this bug labeled one way or the other, but that this request gets
attention from the people that can help completing the "last 10%" of the
process required to include it in the release. If everyone in this category
is working only on hardblockers, I'm afraid we might not be able to finalize
this bug before the Friday deadline.

Comment 22

[:Paolo Amadini]

Also, let's not forget that at present custom Firefox 4 themes on Windows
and Mac can't host add-on icons bigger than 16x16 pixels. This is an
accessibility regression in my opinion, although not one that affects
vanilla Firefox.

Comment 23

[Asaf Romano (gone)]

Review comments coming in a bit.

Comment 24

[Asaf Romano (gone)]

Comment on attachment 506000 [details] [diff] [review]
Polished patch

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
>@@ -1459,16 +1459,21 @@ function delayedStartup(isLoadingBlank, 
>   if (isLoadingBlank && gURLBar && isElementVisible(gURLBar))
>     gURLBar.focus();
>   else
>     gBrowser.selectedBrowser.focus();
> 
>   gNavToolbox.customizeDone = BrowserToolboxCustomizeDone;
>   gNavToolbox.customizeChange = BrowserToolboxCustomizeChange;
> 
>+  // Since the style of the add-on bar is not customizable, we only have to set
>+  // the additional styling attributes on it once, at startup.
>+  BrowserToolbarSetStylingAttributes(document.getElementById("addon-bar"));
>+  BrowserToolboxSetStylingAttributes();
>+

Toolkit-speaking, the addon-bar does get the iconsize attribute set (updateIconSize calls updateToolboxProperty, and updateToolboxProperty calls  forEachCustomizableToolbar. This method does take care of "external toolbars"). I guess we force a static size for the addon toolbar somewhere (in the theme?), but it can change (by a theme?). All in all, I think you should avoid this optimization.


>@@ -3507,29 +3513,38 @@ function BrowserCustomizeToolbar()
>     else
>       sheetFrame.setAttribute("src", customizeURL);
> 
>     // Open the panel, but make it invisible until the iframe has loaded so
>     // that the user doesn't see a white flash.
>     panel.style.visibility = "hidden";
>     gNavToolbox.addEventListener("beforecustomization", function () {
>       gNavToolbox.removeEventListener("beforecustomization", arguments.callee, false);
>+      BrowserPaletteSetStylingAttributes();
>       panel.style.removeProperty("visibility");
>     }, false);
>     panel.openPopup(gNavToolbox, "after_start", 0, 0);
>-    return sheetFrame.contentWindow;
>+    gCustomizeWindow = sheetFrame.contentWindow;

The new global variable isn't necessary.  Declare a local variable and pass it to BrowserPaletteSetStylingAttributes in both clousures.


>-function BrowserToolboxCustomizeChange() {
>+function BrowserToolboxCustomizeChange(aEvent) {

In browser, aEvent is usually a dom event.  Please find a better label. I would consider creating a new callback

>+  if (aEvent == "style") {
>+    BrowserToolboxSetStylingAttributes();
>+  }

Nit: Remove the braces.

>   gHomeButton.updatePersonalToolbarStyle();
>   BookmarksMenuButton.customizeChange();
>   allTabs.readPref();

Do we need to do so for a "style" change?


>+ * Sets styling attributes on the toolbar customization palette.
>+ */
>+function BrowserPaletteSetStylingAttributes() {
>+  if (gCustomizeWindow) {

Why was this check necessary? You're calling this method only as the dialog opens.

>+    let customizeDocument = gCustomizeWindow.document;

>+    let paletteBox = customizeDocument.getElementById("palette-box");
>+    let themeDetector = document.getElementById("theme-detection-button");
>+    let elmClone = customizeDocument.importNode(themeDetector, true);
>+    BrowserToolbarSetStylingAttributes(paletteBox, elmClone, customizeDocument);

This is a little bit confusing. pallete-box is not a toolbar.

>+function BrowserToolboxSetStylingAttributes() {
>+  // Apply styling attributes on default and on custom toolbars.
>+  let XULNS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
>+  Array.forEach(gNavToolbox.getElementsByTagNameNS(XULNS, "toolbar"),
>+                function(toolbar) BrowserToolbarSetStylingAttributes(toolbar));

Hrm, how would it affect a static toolbar (toolbar which is not customizable)? iconsize is set only on customizable toolbars.

>+
>+  // Move the button used for icon property detection back out of any toolbar.
>+  let themeDetector = document.getElementById("theme-detection-button");
>+  document.documentElement.appendChild(themeDetector);

>+/**
>+ * Sets styling attributes on the specified toolbar or customization palette.
>+ * These attributes can be used by stylesheets of add-ons to select toolbar
>+ * icons of the correct size and color, depending not only on the platform and
>+ * the specific toolbar iconsize preference, but also on the applied theme.
>+ *
>+ * @param aToolbar
>+ *        Toolbar element on which the attributes should be updated.
>+ *
>+ * @param aDetector [optional]
>+ *        Hidden toolbar button element used for parameters detection, or null
>+ *        to use the element in the current browser window.

Nit: It is not |null|, actually. It's |undefined|. Just phrase "If not specified…"

>+ *
>+ * @param aDocument [optional]
>+ *        Document where the elements are located. Not specified if the elements
>+ *        are located in the current browser window.

This is all just for the customization window, right?  If so, consider splitting this code to separate function.

>+ */
>+function BrowserToolbarSetStylingAttributes(aToolbar, aDetector, aDocument) {
>+  // Move the button used for icon size detection to the current toolbar.
>+  let themeDetector = aDetector ||
>+                      document.getElementById("theme-detection-button");
>+  aToolbar.appendChild(themeDetector);
>+
>+  // Get the used CSS values for the size of the inner image element.
>+  let doc = aDocument || document;
>+  let imageElement = doc.getAnonymousElementByAttribute(themeDetector, "class",
>+                                                        "toolbarbutton-icon");

Maybe I'm missing something here. As the button is hidden anyway. I don't understand why a real-full-complete toolbarbutton element is necessary. Instead, you could add some dummy xul element, and let the theme style it.

>+  let usedStyle = window.getComputedStyle(imageElement);
>+  let usedWidth = parseInt(usedStyle.getPropertyValue("width")) || 24;
>+  let usedHeight = parseInt(usedStyle.getPropertyValue("height")) || 24;

I prefer getPropertyCSSValue("width").getFloatValue(CSSPrimitiveValue.CSS_PX) over parseInt.

>+  // Set the attribute representing the actual icon size wanted by the theme.

I think CSS classes would work better here, conceptually.

>+  aToolbar.setAttribute("styledimensions", usedWidth + "x" + usedHeight);
>+  if (usedWidth >= 32 && usedHeight >= 32) {
>+    aToolbar.setAttribute("style32x32", "true");
>+    aToolbar.removeAttribute("style24x24");
>+    aToolbar.removeAttribute("style16x16");
>+  } else if (usedWidth >= 24 && usedHeight >= 24) {
>+    aToolbar.removeAttribute("style32x32");
>+    aToolbar.setAttribute("style24x24", "true");
>+    aToolbar.removeAttribute("style16x16");
>+  } else {
>+    aToolbar.removeAttribute("style32x32");
>+    aToolbar.removeAttribute("style24x24");
>+    aToolbar.setAttribute("style16x16", "true");
>+  }

I suggest:

1. Having one class named per usedWidth and usedHeight, something like .size_**_**
2. If used-width or used-height are not normal, have another class ".fallbackSize_**_**
3. Remove the styledimensions attribute.

>+  // Some platform default themes and custom themes might want add-ons to use a
>+  // monochrome toolbar button icon if possible. The theme indicates this using
>+  // the "counter-reset" property. A custom property cannot be used because
>+  // getComputedStyle can only return the values of standard CSS properties.
>+  if (usedStyle.getPropertyValue("counter-reset") == "monochrome 0") {

Well-well-well, this sucks, sorry.  If we _must_ do this (I do realize how this simples the work of the theme-author), 
consider using -moz-appearance: anything-but-none.

I wonder, though, if there's anything else we could detect here.

>+    aToolbar.setAttribute("stylemonochrome", "true");
>+  } else {
>+    aToolbar.removeAttribute("stylemonochrome");
>+  }

Nit: Remove the braces.

>diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul
>--- a/browser/base/content/browser.xul
>+++ b/browser/base/content/browser.xul
 
>+# The following item is used to calculate, for every toolbar, the actual icon
>+# size and other properties wanted by the current theme for add-on buttons.

This is the kind of comment that we shouldn't pre-process.

>+# Its identifier is not included in the primaryToolbarButtons list.

This doesn't say much.

>+++ b/browser/themes/gnomestripe/browser/browser.css
>@@ -879,16 +879,21 @@ toolbar[iconsize="small"] #sync-button[s
>   list-style-image: url("chrome://browser/skin/sync-16-throbber.png");
>   -moz-image-region: rect(0px 16px 16px 0px);
> }
> 
> toolbar[iconsize="small"] #feed-button {
>   -moz-image-region: rect(0px 112px 16px 96px);
> }
> 
>+toolbar[iconsize="small"] #theme-detection-button > .toolbarbutton-icon {

One more thing: While it must remain optional for backwards compatibility at this point, I would prefer if all default themes did set the 
width and height explicitly for each iconsize.  The current setup is quite confusing, imo.

Comment 25

[Dão Gottwald [:dao]]

Comment on attachment 506000 [details] [diff] [review]
Polished patch

I don't think it's reasonable to expect add-on authors to start providing 32x32 icons. I'd rather just set an "effectiveiconsize" attribute (or similar with a better name) to "small" for 16px and "large" otherwise.

Also, you could use the counter-reset trick to express the desired icon size instead of the "theme detection button", which seems quite messy.

The "monochrome" stuff doesn't belong in this bug.

Comment 26

[Asaf Romano (gone)]

I actually prefer the button over the rule, esp. as a semi-api.

Comment 27

[:Paolo Amadini]

(In reply to comment #24)
> Comment on attachment 506000 [details] [diff] [review]
> Polished patch

Thank you very much for your detailed review! I'll address all the comments
in a few minutes. My local build is failing now, after the latest pull, I'm
trying to restore it and I'll provide a new patch as soon as I can test it.

> I guess we force a static size for the addon toolbar somewhere (in the theme?),

In browser.xul, where we set lockiconsize="true". Themes can't override it.

http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/browser.xul#996

> but it can change (by a theme?). All in all, I think you should avoid this
> optimization.

In any case, I can remove the optimization to make the patch future-proof.

> The new global variable isn't necessary.  Declare a local variable and pass it
> to BrowserPaletteSetStylingAttributes in both clousures.
> 
> >+function BrowserPaletteSetStylingAttributes() {
> >+  if (gCustomizeWindow) {
> Why was this check necessary? You're calling this method only as the dialog
> opens.

Good catch, they're both leftovers from the time when I thought the
customization toolbox was affected by the iconsize change.

> >-function BrowserToolboxCustomizeChange() {
> >+function BrowserToolboxCustomizeChange(aEvent) {
> 
> In browser, aEvent is usually a dom event.  Please find a better label.

aType ?

> I would consider creating a new callback

I also considered doing it, but found it introduced a bit of overhead
that maybe is not required?

> >+  if (aEvent == "style") {
> >+    BrowserToolboxSetStylingAttributes();
> >+  }
> 
> Nit: Remove the braces.

This becomes:

  if (aType == "style")
    BrowserToolboxSetStylingAttributes();
  else {
    gHomeButton.updatePersonalToolbarStyle();
    BookmarksMenuButton.customizeChange();
    allTabs.readPref();
  }

Looks good?

> >+    let customizeDocument = gCustomizeWindow.document;
> 
> >+    let paletteBox = customizeDocument.getElementById("palette-box");
> >+    let themeDetector = document.getElementById("theme-detection-button");
> >+    let elmClone = customizeDocument.importNode(themeDetector, true);
> >+    BrowserToolbarSetStylingAttributes(paletteBox, elmClone, customizeDocument);
> 
> This is a little bit confusing. pallete-box is not a toolbar.

What about just "BrowserSetStylingAttributes"?

> >+function BrowserToolboxSetStylingAttributes() {
> >+  // Apply styling attributes on default and on custom toolbars.
> >+  let XULNS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
> >+  Array.forEach(gNavToolbox.getElementsByTagNameNS(XULNS, "toolbar"),
> >+                function(toolbar) BrowserToolbarSetStylingAttributes(toolbar));
> 
> Hrm, how would it affect a static toolbar (toolbar which is not customizable)?
> iconsize is set only on customizable toolbars.

Rules in the theme determine the actual button size, based on both "mode"
and "iconsize" attributes. Generally, when "iconsize" is missing, themes
assume large icon mode. In any case we set iconsize explicitly for all
non-customizable toolbars if I remember correctly.

> >+ *
> >+ * @param aDocument [optional]
> >+ *        Document where the elements are located. Not specified if the elements
> >+ *        are located in the current browser window.
> 
> This is all just for the customization window, right?  If so, consider
> splitting this code to separate function.

Ok, I'll split the function and call a third one as soon as imageElement is
retrieved.

> Maybe I'm missing something here. As the button is hidden anyway. I don't
> understand why a real-full-complete toolbarbutton element is necessary.
> Instead, you could add some dummy xul element, and let the theme style it.

This is for backwards compatibility with existing custom themes. We may
consider requiring themes to set it explicitly from now on anyway. What
do you think?

> >+  let usedStyle = window.getComputedStyle(imageElement);
> >+  let usedWidth = parseInt(usedStyle.getPropertyValue("width")) || 24;
> >+  let usedHeight = parseInt(usedStyle.getPropertyValue("height")) || 24;
> 
> I prefer getPropertyCSSValue("width").getFloatValue(CSSPrimitiveValue.CSS_PX)
> over parseInt.

I've read getPropertyCSSValue is going to be deprecated. And maybe the check
for the default value when no width is specified will be more complex and
require exception checking. This seemed simpler. What should I do?

> I think CSS classes would work better here, conceptually.

I can use classes, of course. One small difficulty is that I'll have to
enumerate all classes and remove the old size_**_** values, but probably
it can be done efficiently with regular expressions.

> I suggest:
> 
> 1. Having one class named per usedWidth and usedHeight, something like
> .size_**_**
> 2. If used-width or used-height are not normal, have another class
> ".fallbackSize_**_**
> 3. Remove the styledimensions attribute.

That would require extension stylesheets to check two classes (for the
fallback). We should at least set size_16x16 and size_24x24 both for
sizes that are exact and sizes that are not. I don't think there's a
need for a class named "fallbackSize" at this point.

> >+  if (usedStyle.getPropertyValue("counter-reset") == "monochrome 0") {
> 
> Well-well-well, this sucks, sorry.  If we _must_ do this (I do realize
> how this simples the work of the theme-author), 
> consider using -moz-appearance: anything-but-none.

Actually, I chose "counter-reset" because it is very unlikely to have any
other meaning when styling buttons. However, since themes will be using
a dedicated rule for this, thinking it over it's fine to use any attribute,
so "-moz-appearance" is fine.

> >diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul
> >--- a/browser/base/content/browser.xul
> >+++ b/browser/base/content/browser.xul
> 
> >+# The following item is used to calculate, for every toolbar, the actual icon
> >+# size and other properties wanted by the current theme for add-on buttons.
> 
> This is the kind of comment that we shouldn't pre-process.

You mean it should be visible to theme authors? Yes, that's fine.

> >+++ b/browser/themes/gnomestripe/browser/browser.css
> >@@ -879,16 +879,21 @@ toolbar[iconsize="small"] #sync-button[s
> >   list-style-image: url("chrome://browser/skin/sync-16-throbber.png");
> >   -moz-image-region: rect(0px 16px 16px 0px);
> > }
> > 
> > toolbar[iconsize="small"] #feed-button {
> >   -moz-image-region: rect(0px 112px 16px 96px);
> > }
> > 
> >+toolbar[iconsize="small"] #theme-detection-button > .toolbarbutton-icon {
> 
> One more thing: While it must remain optional for backwards compatibility at
> this point, I would prefer if all default themes did set the 
> width and height explicitly for each iconsize.  The current setup is quite
> confusing, imo.

I'll update all themes consequently.

Comment 28

[:Paolo Amadini]

One note: using classes instead of attributes probably closes the way to
the persistence optimization, but this should be OK since performance tests
showed it isn't necessary.

Comment 29

[Dão Gottwald [:dao]]

The performance test doesn't necessarily reflect reality, let alone that it doesn't test with add-on toolbars and add-on buttons where calling BrowserToolboxSetStylingAttributes would actually make a difference.

Comment 30

[:Paolo Amadini]

(In reply to comment #25)
> The "monochrome" stuff doesn't belong in this bug.

Actually, there is bug 618324 that is about providing an opt-in automatic
filter for extensions that don't have custom artwork for the monochrome case,
that would be nice to have actually.

This bug is instead for add-ons that have custom monochrome artwork, because
it changes which icon is used based on the toolbar styling. Nils' test case
in this bug includes this check for this reason.

Comment 31

[:Paolo Amadini]

Attachment: Revised patch

Revised patch. I've interpreted this also based on my answers to the previous
discussions, let me know if there is still something I should change.

To test, download Nils' add-on from https://github.com/nmaier/compatible-icons
and replace "overlay.css" with the following stylesheet:

@namespace
url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");

.compatible-icons-button {
  list-style-image: url(icon24.png) !important;
}

toolbar.size_16x16 .compatible-icons-button,
#palette-box.size_16x16 .compatible-icons-button,
toolbar[iconsize="small"]:not(.has_styling) .compatible-icons-button {
  list-style-image: url(icon16.png) !important;
}

toolbar.style_monochrome .compatible-icons-button,
#palette-box.style_monochrome .compatible-icons-button {
  list-style-image: url(icon16-monochrome.png) !important;
}

Comment 32

[Dão Gottwald [:dao]]

Please don't overload this bug, keep it focused on the icon sizes.

Comment 33

[Dão Gottwald [:dao]]

Comment on attachment 507971 [details] [diff] [review]
Revised patch

Per my earlier comment, I don't think we should add 32x32 to the mix. This invites authors to target toolbar.size_16x16 and toolbar.size_24x24 and forget about the rest. Again, I think we should set something like an "effective icon size" attribute which would match the "iconsize" attribute, except when the theme wants small icons on a toolbar despite the iconsize attribute. And I still think letting the theme set something like "counter-reset: smallicons;" on a toolbar is the simplest way to achieve this (although I agree it's nasty).

Comment 34

[:Paolo Amadini]

(In reply to comment #33)
> Per my earlier comment, I don't think we should add 32x32 to the mix. This
> invites authors to target toolbar.size_16x16 and toolbar.size_24x24 and forget
> about the rest.

Explicit support for 32x32 removed :-) In any case, if we allow themes to
specify their actual icon dimensions, we could allow theme authors and authors
of major extension to work together to provide bigger icon sizes, like 32x32,
for example to allow greater accessibility. Why should we discourage this?
I think it's a nice feature for no more than two additional lines of code :-)

> Again, I think we should set something like an "effective icon
> size" attribute which would match the "iconsize" attribute, except when the
> theme wants small icons on a toolbar despite the iconsize attribute.

Like in comment 0, wantediconsize="small" and wantedicondimensions="18x18"?

> And I
> still think letting the theme set something like "counter-reset: smallicons;"
> on a toolbar is the simplest way to achieve this (although I agree it's
> nasty).

Actually, the advantage of testing "width" and "height" is that it is
immediately backwards compatible with existing custom themes that don't
have an explicit rule for the detector button, so it makes things simpler
for theme authors. Do you have a specific reason for doing this differently?

Comment 35

[Dão Gottwald [:dao]]

(In reply to comment #34)
> (In reply to comment #33)
> > Per my earlier comment, I don't think we should add 32x32 to the mix. This
> > invites authors to target toolbar.size_16x16 and toolbar.size_24x24 and forget
> > about the rest.
> 
> Explicit support for 32x32 removed :-) In any case, if we allow themes to
> specify their actual icon dimensions, we could allow theme authors and authors
> of major extension to work together to provide bigger icon sizes, like 32x32,
> for example to allow greater accessibility. Why should we discourage this?
> I think it's a nice feature for no more than two additional lines of code :-)

Because it automatically provides the footgun mentioned above.

> > Again, I think we should set something like an "effective icon
> > size" attribute which would match the "iconsize" attribute, except when the
> > theme wants small icons on a toolbar despite the iconsize attribute.
> 
> Like in comment 0, wantediconsize="small" and wantedicondimensions="18x18"?

wantediconsize sounds ok.

> > And I
> > still think letting the theme set something like "counter-reset: smallicons;"
> > on a toolbar is the simplest way to achieve this (although I agree it's
> > nasty).
> 
> Actually, the advantage of testing "width" and "height" is that it is
> immediately backwards compatible with existing custom themes that don't
> have an explicit rule for the detector button, so it makes things simpler
> for theme authors. Do you have a specific reason for doing this differently?

Letting the attribute match the iconsize attribute except when specified otherwise should be compatible with most themes out there as well.

The "theme detector" stuff looks unnecessarily expensive, long-winded and fragile (e.g. it seems to assume that toolbars are actually visible).

Comment 36

[:Paolo Amadini]

(In reply to comment #35)
> Because it automatically provides the footgun mentioned above.

Sorry, I probably misunderstood. Did you originally mean that theme authors
should only target 16x16 and 24x24 for add-on buttons and forget about other
sizes? Ok.

> > Actually, the advantage of testing "width" and "height" is that it is
> > immediately backwards compatible with existing custom themes that don't
> > have an explicit rule for the detector button, so it makes things simpler
> > for theme authors. Do you have a specific reason for doing this differently?
> 
> Letting the attribute match the iconsize attribute except when specified
> otherwise should be compatible with most themes out there as well.
> 
> The "theme detector" stuff looks unnecessarily expensive, long-winded and
> fragile (e.g. it seems to assume that toolbars are actually visible).

This I still don't understand. Since a theme cannot set arbitrary attributes,
the "detector" is the only mean that the theme has to communicate to the
browser the desired icon size. We can't do without it. The "detector" can be
a style on any element, of course, and we use a toolbar button for two reasons:

 * It requires no changes to existing themes (including default themes).
 * Different toolbars might have different rules that determine the icon size,
    depending on what the theme wants. By using a normal toolbar button, we
    don't need a special rule for the detector.

Is there something I'm missing here?

Comment 37

[Dão Gottwald [:dao]]

(In reply to comment #36)
> (In reply to comment #35)
> > Because it automatically provides the footgun mentioned above.
> 
> Sorry, I probably misunderstood. Did you originally mean that theme authors
> should only target 16x16 and 24x24 for add-on buttons and forget about other
> sizes? Ok.

Theme authors may ship different sizes. But add-on authors shouldn't have to worry about that.

> > > Actually, the advantage of testing "width" and "height" is that it is
> > > immediately backwards compatible with existing custom themes that don't
> > > have an explicit rule for the detector button, so it makes things simpler
> > > for theme authors. Do you have a specific reason for doing this differently?
> > 
> > Letting the attribute match the iconsize attribute except when specified
> > otherwise should be compatible with most themes out there as well.
> > 
> > The "theme detector" stuff looks unnecessarily expensive, long-winded and
> > fragile (e.g. it seems to assume that toolbars are actually visible).
> 
> This I still don't understand. Since a theme cannot set arbitrary attributes,
> the "detector" is the only mean that the theme has to communicate to the
> browser the desired icon size. We can't do without it. The "detector" can be
> a style on any element, of course, and we use a toolbar button for two reasons:
> 
>  * It requires no changes to existing themes (including default themes).
>  * Different toolbars might have different rules that determine the icon size,
>     depending on what the theme wants. By using a normal toolbar button, we
>     don't need a special rule for the detector.
> 
> Is there something I'm missing here?

Themes can set counter-reset: smallicons; on specific toolbars. If not set, you simply map iconsize to wantediconsize, providing backward compatibility.

Comment 38

[:Paolo Amadini]

Attachment: Patch, setting attributes, without monochrome and icon dimensions checks

I'm not sure about what's being asked with regard to the previous comment,
but apart from that this should be near to completion.

Thank you very much for your help with review! This patch looks much neater
to me than the first revision.

Comment 39

[:Paolo Amadini]

(In reply to comment #38)
> I'm not sure about what's being asked with regard to the previous comment,

That being comment #35.

Comment 40

[:Paolo Amadini]

> Themes can set counter-reset: smallicons; on specific toolbars. If not set, you
> simply map iconsize to wantediconsize, providing backward compatibility.

I see what you mean. We would likewise need "counter-reset: largeicons;",
for example to allow themes to use 24x24 icons on Windows, right?

Comment 41

[Dão Gottwald [:dao]]

(In reply to comment #40)
> > Themes can set counter-reset: smallicons; on specific toolbars. If not set, you
> > simply map iconsize to wantediconsize, providing backward compatibility.
> 
> I see what you mean. We would likewise need "counter-reset: largeicons;",
> for example to allow themes to use 24x24 icons on Windows, right?

No, counter-reset: smallicons; being absent and iconsize="large" would imply wantediconsize="large".

Comment 42

[:Paolo Amadini]

(In reply to comment #41)
> No, counter-reset: smallicons; being absent and iconsize="large" would imply
> wantediconsize="large".

The point is that, except for the navigation bar, other browser toolbars have
iconsize="small" and lockiconsize="true". So, for example, iconsize="large"
would never be true on the add-on bar on Windows, and the theme would be
unable to use larger icons.

Comment 43

[Dão Gottwald [:dao]]

(In reply to comment #42)
> (In reply to comment #41)
> > No, counter-reset: smallicons; being absent and iconsize="large" would imply
> > wantediconsize="large".
> 
> The point is that, except for the navigation bar, other browser toolbars have
> iconsize="small" and lockiconsize="true". So, for example, iconsize="large"
> would never be true on the add-on bar on Windows, and the theme would be
> unable to use larger icons.

True, but this seems mostly expected. I guess we can support counter-reset: largeicons; if it doesn't add much overhead, but I don't think it's going to be incredibly useful.

Comment 44

[:Paolo Amadini]

Good. A new patch is coming, after I do a bit of testing with the new setup.

Comment 45

[:Paolo Amadini]

Attachment: Patch, with detection CSS styles applied to the toolbar

Comment 46

[Dão Gottwald [:dao]]

Comment on attachment 508014 [details] [diff] [review]
Patch, with detection CSS styles applied to the toolbar

Please remove BrowserPaletteSetStylingAttributes, add-ons were never supposed to add special rules for the palette.

>+  let usedStyle = window.getComputedStyle(aElement);
>+  let counterReset = usedStyle.getPropertyValue("counter-reset");

let counterReset = getComputedStyle(aElement).counterReset;

>--- a/browser/themes/pinstripe/browser/browser.css
>+++ b/browser/themes/pinstripe/browser/browser.css

>+toolbar, #palette-box {

This should be #navigator-toolbox > toolbar

>--- a/browser/themes/winstripe/browser/browser.css
>+++ b/browser/themes/winstripe/browser/browser.css

>+toolbar, #palette-box {

This should be just #nav-bar, given bug 616156.

>--- a/toolkit/content/customizeToolbar.js
>+++ b/toolkit/content/customizeToolbar.js
>@@ -691,16 +691,18 @@ function updateToolboxProperty(aProp, aV
>     if (toolbar.getAttribute("lock" + aProp) == "true" &&
>         toolbar.getAttribute(aProp) == toolbarDefault)
>       return;
> 
>     toolbar.setAttribute(aProp, aValue || toolbarDefault);
>     gToolboxDocument.persist(toolbar.id, aProp);
>   });
> 
>+  toolboxChanged("style");

I think you should pass on aProp here rather than "style".

Comment 47

[Dão Gottwald [:dao]]

Forgot to mention, you need to initialize the toolbars in BrowserStartup rather than delayedStartup in order to prevent jitter when opening a new window. (That you weren't doing this could explain why your early patch didn't seem to affect performance tests.)

Comment 48

[Dão Gottwald [:dao]]

A remaining downside of wantediconsize="large"/"small" is that buttons couldn't be styled as follows, as this would miss the palette, which authors might not realize immediately:

toolbar[wantediconsize=large] #my-button {
  list-style-image: ...;
}
toolbar[wantediconsize=small] #my-button {
  list-style-image: ...;
}

It seems that something like this would be simpler:

#my-button {
  list-style-image: ...;
}
toolbar[smallicons] #my-button {
  list-style-image: ...;
}

Comment 49

[:Paolo Amadini]

(In reply to comment #46)
> Please remove BrowserPaletteSetStylingAttributes, add-ons were never supposed
> to add special rules for the palette.

But this is now required because the palette has large icons with the Linux
default theme and small icons with the Windows default theme.

> >+  let usedStyle = window.getComputedStyle(aElement);
> >+  let counterReset = usedStyle.getPropertyValue("counter-reset");
> 
> let counterReset = getComputedStyle(aElement).counterReset;

Cool :-)

> >+++ b/browser/themes/winstripe/browser/browser.css
> 
> >+toolbar, #palette-box {
> 
> This should be just #nav-bar, given bug 616156.

Do we want custom toolbars to have large icons on Windows?

> >+++ b/toolkit/content/customizeToolbar.js
> >+  toolboxChanged("style");
> 
> I think you should pass on aProp here rather than "style".

If I understand correctly, the original parameter indicates the type of
event: undefined for button position changes, "reset" for when we return
to the default toolbar layout, and now "style" for when we change one of
the two appearance properties. Maybe the name of the changed appearance
property could be the second parameter.

Comment 50

[Dão Gottwald [:dao]]

(In reply to comment #49)
> (In reply to comment #46)
> > Please remove BrowserPaletteSetStylingAttributes, add-ons were never supposed
> > to add special rules for the palette.
> 
> But this is now required because the palette has large icons with the Linux
> default theme and small icons with the Windows default theme.

Not true. For instance, Adblock Plus and Flashblock have large icons in the customization dialog on Windows, which work just fine.

> > >+++ b/browser/themes/winstripe/browser/browser.css
> > 
> > >+toolbar, #palette-box {
> > 
> > This should be just #nav-bar, given bug 616156.
> 
> Do we want custom toolbars to have large icons on Windows?

They respect the "Use small icons" setting.

> > >+++ b/toolkit/content/customizeToolbar.js
> > >+  toolboxChanged("style");
> > 
> > I think you should pass on aProp here rather than "style".
> 
> If I understand correctly, the original parameter indicates the type of
> event: undefined for button position changes, "reset" for when we return
> to the default toolbar layout, and now "style" for when we change one of
> the two appearance properties. Maybe the name of the changed appearance
> property could be the second parameter.

"style" is a term not commonly associated with toolbar customization and I wouldn't know what it means without reading the code.

Comment 51

[:Paolo Amadini]

(In reply to comment #50)
> (In reply to comment #49)
> > (In reply to comment #46)
> > > Please remove BrowserPaletteSetStylingAttributes, add-ons were never supposed
> > > to add special rules for the palette.
> > 
> > But this is now required because the palette has large icons with the Linux
> > default theme and small icons with the Windows default theme.
> 
> Not true. For instance, Adblock Plus and Flashblock have large icons in the
> customization dialog on Windows, which work just fine.

Actually, I was worrying about consistency with the default buttons. They
are small in the customization palette now. This is for letting add-ons
integrate seamlessly with Firefox and allow them to offer such a high-quality
user experience as if the feature was part of Firefox itself. But if it is
decided that we don't care about add-on buttons being of the same size and
color of default buttons in the customization palette, that's fine, I'll
remove that part. Let me know.

> > > >+++ b/browser/themes/winstripe/browser/browser.css
> > > 
> > > >+toolbar, #palette-box {
> > > 
> > > This should be just #nav-bar, given bug 616156.
> > 
> > Do we want custom toolbars to have large icons on Windows?
> 
> They respect the "Use small icons" setting.

But I observe a different behavior: button icons on custom toolbars on
Windows are small (16x16 pixels) regardless of the "Use small icons" setting.
Shouldn't add-on buttons on those toolbars be of the same size?

> > > >+++ b/toolkit/content/customizeToolbar.js
> > > >+  toolboxChanged("style");
> > > 
> > > I think you should pass on aProp here rather than "style".
> > 
> > If I understand correctly, the original parameter indicates the type of
> > event: undefined for button position changes, "reset" for when we return
> > to the default toolbar layout, and now "style" for when we change one of
> > the two appearance properties. Maybe the name of the changed appearance
> > property could be the second parameter.
> 
> "style" is a term not commonly associated with toolbar customization and I
> wouldn't know what it means without reading the code.

What about "propertychanged"?

Comment 52

[:Paolo Amadini]

(In reply to comment #48)
Yes, the goal is to make add-ons use icons of the correct size and color in
the simplest way, keeping compatibility with various host applications,
versions, platforms and themes. Because of the changes to the themes
introduced in Firefox 4, a couple of new selectors are necessary, but
that's all, so the difference in simplicity is very small. Here is the
stylesheet that can be used to be compatible with *all* current host
applications, versions, platforms and themes:

#my-button {
  list-style-image: url(icon24.png);
}

toolbar[wantediconsize="small"] #my-button,
#palette-box[wantediconsize="small"] #my-button,
toolbar[iconsize="small"]:not([wantediconsize]) #my-button {
  list-style-image: url(icon16.png);
}

Note how we check the absence of "wantediconsize" to fall back to "iconsize"
for compatibility with Firefox 3.x without having to point to different files
in chrome.manifest. And here is the same, but for compatibility with all
themes and platforms on Firefox 4 only:

#my-button {
  list-style-image: url(icon24.png);
}

toolbar[wantediconsize="small"] #my-button,
#palette-box[wantediconsize="small"] #my-button {
  list-style-image: url(icon16.png);
}

Comment 53

[Dão Gottwald [:dao]]

(In reply to comment #51)
> Actually, I was worrying about consistency with the default buttons. They
> are small in the customization palette now. This is for letting add-ons
> integrate seamlessly with Firefox and allow them to offer such a high-quality
> user experience as if the feature was part of Firefox itself. But if it is
> decided that we don't care about add-on buttons being of the same size and
> color of default buttons in the customization palette, that's fine, I'll
> remove that part. Let me know.

Not worth worrying about, add-on authors shouldn't be bothered with this. This bug is about simplifying things, remember? :)

> But I observe a different behavior: button icons on custom toolbars on
> Windows are small (16x16 pixels) regardless of the "Use small icons" setting.
> Shouldn't add-on buttons on those toolbars be of the same size?

Since we don't apply the custom button style there, it doesn't look all that odd. And then we also need to take extension toolbars into account, where you usually wouldn't mix stock buttons and add-on buttons anyway.

> > > > >+++ b/toolkit/content/customizeToolbar.js
> > > > >+  toolboxChanged("style");
> > > > 
> > > > I think you should pass on aProp here rather than "style".
> > > 
> > > If I understand correctly, the original parameter indicates the type of
> > > event: undefined for button position changes, "reset" for when we return
> > > to the default toolbar layout, and now "style" for when we change one of
> > > the two appearance properties. Maybe the name of the changed appearance
> > > property could be the second parameter.
> > 
> > "style" is a term not commonly associated with toolbar customization and I
> > wouldn't know what it means without reading the code.
> 
> What about "propertychanged"?

How about just aProp? :)
While you're at it, you could also rename toolboxChanged's argument. As Mano pointed out, aEvent doesn't fit.

Comment 54

[:Paolo Amadini]

Attachment: Patch, without changes to the cutomization palette

Here is the new patch, with the following front-end changes:
- Default and add-on buttons have different sizes in custom toolbars on Windows.
- The same goes for the customization palette on Windows and Mac.

This patch also includes the requested Toolkit back-end changes.

Comment 55

[:Paolo Amadini]

(In reply to comment #47)
> Forgot to mention, you need to initialize the toolbars in BrowserStartup rather
> than delayedStartup in order to prevent jitter when opening a new window. (That
> you weren't doing this could explain why your early patch didn't seem to affect
> performance tests.)

Even with this modification, I didn't see a significant change in startup time:

http://tbpl.mozilla.org/?tree=MozillaTry&rev=771a00762914
http://graphs.mozilla.org/graph.html#tests=[[17,23,414],[17,1,414]]

But this is my first tryserver run so I might be missing something :-)

Comment 56

[Dão Gottwald [:dao]]

(In reply to comment #55)
> (In reply to comment #47)
> > Forgot to mention, you need to initialize the toolbars in BrowserStartup rather
> > than delayedStartup in order to prevent jitter when opening a new window. (That
> > you weren't doing this could explain why your early patch didn't seem to affect
> > performance tests.)
> 
> Even with this modification, I didn't see a significant change in startup time

Right, I would expect the current patch to be cheaper, as you're not messing around with the dummy button anymore.

Comment 57

[Dão Gottwald [:dao]]

(In reply to comment #52)
> (In reply to comment #48)
> Yes, the goal is to make add-ons use icons of the correct size and color in
> the simplest way, keeping compatibility with various host applications,
> versions, platforms and themes. Because of the changes to the themes
> introduced in Firefox 4, a couple of new selectors are necessary, but
> that's all, so the difference in simplicity is very small. Here is the
> stylesheet that can be used to be compatible with *all* current host
> applications, versions, platforms and themes:
> 
> #my-button {
>   list-style-image: url(icon24.png);
> }
> 
> toolbar[wantediconsize="small"] #my-button,
> #palette-box[wantediconsize="small"] #my-button,
> toolbar[iconsize="small"]:not([wantediconsize]) #my-button {
>   list-style-image: url(icon16.png);
> }
> 
> Note how we check the absence of "wantediconsize" to fall back to "iconsize"
> for compatibility with Firefox 3.x without having to point to different files
> in chrome.manifest.

Works this way as well:

toolbar[smallicons] #my-button,
toolbar[iconsize="small"] #my-button {
  list-style-image: url(icon16.png);
}

#my-button,
toolbar[iconsize="large"][smallicons] #my-button {
  list-style-image: url(icon24.png);
}

Comment 58

[:Paolo Amadini]

(In reply to comment #57)
> Works this way as well:
> 
> toolbar[smallicons] #my-button,
> toolbar[iconsize="small"] #my-button {
>   list-style-image: url(icon16.png);
> }
> 
> #my-button,
> toolbar[iconsize="large"][smallicons] #my-button {
>   list-style-image: url(icon24.png);
> }

No, for example in Firefox 4 the second rule would match the navigation bar
with greater specificity than the first one, and a 24x24 icon would be used
instead of the wanted 16x16 icon.

Comment 59

[Dão Gottwald [:dao]]

Oops, yes, that last selector was broken. Fixing that, it boils down to this:

#my-button {
  list-style-image: url(icon24.png);
}

toolbar[smallicons] #my-button,
toolbar[iconsize="small"] #my-button {
  list-style-image: url(icon16.png);
}

So this doesn't support "counter-reset: largeicons", but as I said, we should only support that if it doesn't add significant overhead, which at this point it seems to do.

Alternatively, we could just update the iconsize attribute's value, so there wouldn't be any change for add-ons. This would mean that the Windows and Mac default themes would need to use #navigator-toolbox[iconsize="large"] > #nav-bar for the styling of the back button.

Comment 60

[:Paolo Amadini]

(In reply to comment #59)
> Oops, yes, that last selector was broken. Fixing that, it boils down to this:
> 
> #my-button {
>   list-style-image: url(icon24.png);
> }
> 
> toolbar[smallicons] #my-button,
> toolbar[iconsize="small"] #my-button {
>   list-style-image: url(icon16.png);
> }
> 
> So this doesn't support "counter-reset: largeicons", but as I said, we should
> only support that if it doesn't add significant overhead, which at this point
> it seems to do.

For clarity, here is the add-on stylesheet that would allow themes to use
larger icons for the add-on bar and other toolbars if they want, and for
the add-on to support both Firefox 3, Firefox 4 and other applications:

#my-button {
  list-style-image: url(icon24.png);
}

toolbar[wantediconsize="small"] #my-button,
toolbar[iconsize="small"]:not([wantediconsize]) #my-button {
  list-style-image: url(icon16.png);
}

And here is the add-on stylesheet that would allow themes to use larger
icons for the add-on bar and other toolbars if they want, and for the
add-on to support Firefox 4 only:

#my-button {
  list-style-image: url(icon24.png);
}

toolbar[wantediconsize="small"] #my-button {
  list-style-image: url(icon16.png);
}

Does this still seem "significant overhead" to you, compared to the gain?

> Alternatively, we could just update the iconsize attribute's value, so there
> wouldn't be any change for add-ons. This would mean that the Windows and Mac
> default themes would need to use #navigator-toolbox[iconsize="large"] >
> #nav-bar for the styling of the back button.

Unfortunately that's not possible because this attribute is controlled
directly by Toolkit. That's the first thing I checked out before I started
working on the other solutions, actually.

Comment 61

[Dão Gottwald [:dao]]

(In reply to comment #60)
> Does this still seem "significant overhead" to you, compared to the gain?

Letting it dictate the API is significant, yes, and the gain is questionable.

> > Alternatively, we could just update the iconsize attribute's value, so there
> > wouldn't be any change for add-ons. This would mean that the Windows and Mac
> > default themes would need to use #navigator-toolbox[iconsize="large"] >
> > #nav-bar for the styling of the back button.
> 
> Unfortunately that's not possible because this attribute is controlled
> directly by Toolkit.

What exactly does this mean? Why can't we change the attribute after updateToolboxProperty set it?

Comment 62

[:Paolo Amadini]

(In reply to comment #61)
> What exactly does this mean? Why can't we change the attribute after
> updateToolboxProperty set it?

I was referring to the fact that Toolkit both sets the attribute and
then persists it so that it is reloaded when the next window opens.

http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/content/customizeToolbar.js#681

However you're right, since the attribute is only written in this function
and we are notified right after that, if we take care to persist our
modification too, we might be able to override Toolkit's value. We might
experience jitter as the attribute value flips back and forth, but probably
since the change is synchronous it can't be noticed because the window
doesn't refresh in the meantime. This would actually be the best solution
as far as the API is concerned, and one I should have put more thought on
from the start. Shall I proceed?

Comment 63

[Dão Gottwald [:dao]]

(In reply to comment #62)
> Shall I proceed?

Please do. Thanks!

Comment 64

[:Paolo Amadini]

Er, now I remember... "iconsize" is used by default themes to determine which
button padding to use and how to style the default back and forward button.
To preserve the difference between large and small icons we should copy the
original value of "iconsize" to another attribute ("originaliconsize"?), and
update all default themes to use that attribute instead of "iconsize".

Comment 65

[Dão Gottwald [:dao]]

See comment 59, the user-set iconsize attribute is on #navigator-toolbox.

Comment 66

[:Paolo Amadini]

(In reply to comment #65)
> See comment 59, the user-set iconsize attribute is on #navigator-toolbox.

I see, the same goes for margins and padding I think. The add-on bar is
outside the toolbox, but themes that want small icons regardless of the
user preference don't want to use different margins and padding for the
two modes, right?

Comment 67

[Dão Gottwald [:dao]]

Yes, we actually only want it for #nav-bar.

Comment 68

[:Paolo Amadini]

I'm working on the new rules right now. What do you think of making the
size of add-on buttons match the size of stock buttons on custom toolbars
on Windows too, instead that only on Mac, by replacing this rule...

#nav-bar {
  counter-reset: smallicons;
}

...with the same rule that we use in the Mac theme?

#navigator-toolbox > toolbar {
  counter-reset: smallicons;
}

Comment 69

[Dão Gottwald [:dao]]

We don't need that.

Comment 70

[:Paolo Amadini]

Attachment: Patch, overriding the iconzize attribute

Here is the new patch. I have to say that this approach is really the best
possible for add-on authors, because add-ons won't need any change to be
compatible with any theme that uses the standard 24x24 or 16x16 icons.

The changes required in the new default themes are a bit more complex. I did
a simple search-and-replace in the Windows and Mac themes, without knowing the
exact details of the theme rules. If you know of a simpler way to achieve the
same result, please tell me!

Comment 71

[Dão Gottwald [:dao]]

Comment on attachment 508754 [details] [diff] [review]
Patch, overriding the iconzize attribute

>+function BrowserToolboxCustomizeChange(aTypeOrProperty) {
>+  if (aTypeOrProperty == "iconsize")
>+    BrowserToolboxSetStylingAttributes();
>+  else {
>+    gHomeButton.updatePersonalToolbarStyle();
>+    BookmarksMenuButton.customizeChange();
>+    allTabs.readPref();
>+  }

Please use a switch instead of if/else and do nothing for "mode".

>+function BrowserToolboxSetStylingAttributes() {

Please rename... suggestion: getToolbarIconsizesFromTheme

>+  // Apply styling attributes on default and on custom toolbars.
>+  let XULNS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
>+  Array.forEach(gNavToolbox.getElementsByTagNameNS(XULNS, "toolbar"),
>+                function(toolbar) BrowserSetStylingAttributes(toolbar));
>+
>+  // The add-on bar is not in the toolbox.
>+  BrowserSetStylingAttributes(document.getElementById("addon-bar"));

Can you copy the logic from customizeToolbar's forEachCustomizableToolbar function instead?

>+function BrowserSetStylingAttributes(aElement) {

Please put this inside the previous function, it doesn't need to be global.

>--- a/browser/themes/pinstripe/browser/browser.css
>+++ b/browser/themes/pinstripe/browser/browser.css

>+/* ----- ADD-ON TOOLBAR BUTTONS ----- */
>+
>+#navigator-toolbox > toolbar {

Please merge this into the "PRIMARY TOOLBAR BUTTONS" section. (Same goes for winstripe.)

>+  /*
>+   * The following property is interpreted by browser code and makes sure that,
>+   * when this theme is used, iconsize="small" is forced on these toolbars.
>+   */
>+  counter-reset: smallicons;

This comment can be trimmed to just /* force iconsize="small" on these toolbars */. (Same goes for winstripe.)

>-toolbar:not([iconsize="small"])[mode="icons"] #back-button {
>+toolbar:not(#nav-bar):not([iconsize="small"])[mode="icons"] #back-button,
>+#navigator-toolbox:not([iconsize="small"])[mode="icons"] > #nav-bar #back-button {

Can you set iconsize="large" on navigator-toolbox in browser.xul and use #navigator-toolbox[iconsize=large] here? (Same goes for winstripe.)

I think we can safely get rid of the first selector altogether. It doesn't affect any built-in toolbar.

>--- a/browser/themes/winstripe/browser/browser.css
>+++ b/browser/themes/winstripe/browser/browser.css

>-#navigator-toolbox > toolbar:not(#toolbar-menubar):not(#TabsToolbar)[iconsize="small"],
>-#navigator-toolbox > toolbar:not(#toolbar-menubar):not(#TabsToolbar)[defaulticonsize="small"]:not([iconsize]) {
>+#navigator-toolbox > toolbar:not(#nav-bar):not(#toolbar-menubar):not(#TabsToolbar)[iconsize="small"],
>+#navigator-toolbox > toolbar:not(#nav-bar):not(#toolbar-menubar):not(#TabsToolbar)[defaulticonsize="small"]:not([iconsize]) {
>   padding-top: 1px;
>   padding-bottom: 1px;
> }

I'm not sure I understand this change.

>-#nav-bar #back-button {
>+#navigator-toolbox #nav-bar #back-button {
>   -moz-margin-end: 0;
> }
> 
>-#nav-bar #forward-button {
>+#navigator-toolbox #nav-bar #forward-button {
>   border-left: none;
>   -moz-margin-start: 0;
> }
> 
>-#nav-bar #back-button:-moz-locale-dir(ltr) {
>+#navigator-toolbox #nav-bar #back-button:-moz-locale-dir(ltr) {
>   border-top-right-radius: 0;
>   border-bottom-right-radius: 0;
> }
> 
>-#nav-bar #back-button:-moz-locale-dir(rtl),
>-#nav-bar #forward-button {
>+#navigator-toolbox #nav-bar #back-button:-moz-locale-dir(rtl),
>+#navigator-toolbox #nav-bar #forward-button {
>   border-top-left-radius: 0;
>   border-bottom-left-radius: 0;
> }

These changes don't seem to make sense.

>--- a/toolkit/content/customizeToolbar.js
>+++ b/toolkit/content/customizeToolbar.js

>-function toolboxChanged(aEvent)
>+function toolboxChanged(aTypeOrProperty)

Just "aType" is fine.

Comment 72

[:Paolo Amadini]

(In reply to comment #71)
> >+  // Apply styling attributes on default and on custom toolbars.
> >+  let XULNS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
> >+  Array.forEach(gNavToolbox.getElementsByTagNameNS(XULNS, "toolbar"),
> >+                function(toolbar) BrowserSetStylingAttributes(toolbar));
> >+
> >+  // The add-on bar is not in the toolbox.
> >+  BrowserSetStylingAttributes(document.getElementById("addon-bar"));
> 
> Can you copy the logic from customizeToolbar's forEachCustomizableToolbar
> function instead?

Not sure what you're asking... we should not filter out non-customizable
toolbars here, so we can use getElementsByTagNameNS instead of enumerating
all children and checking their attributes and local name manually.

> >--- a/browser/themes/winstripe/browser/browser.css
> >+++ b/browser/themes/winstripe/browser/browser.css
> 
> >-#navigator-toolbox > toolbar:not(#toolbar-menubar):not(#TabsToolbar)[iconsize="small"],
> >-#navigator-toolbox > toolbar:not(#toolbar-menubar):not(#TabsToolbar)[defaulticonsize="small"]:not([iconsize]) {
> >+#navigator-toolbox > toolbar:not(#nav-bar):not(#toolbar-menubar):not(#TabsToolbar)[iconsize="small"],
> >+#navigator-toolbox > toolbar:not(#nav-bar):not(#toolbar-menubar):not(#TabsToolbar)[defaulticonsize="small"]:not([iconsize]) {
> >   padding-top: 1px;
> >   padding-bottom: 1px;
> > }
> 
> I'm not sure I understand this change.

What I observed is that without this change the total height of the toolbox
in small icons mode was larger than in large icons mode. I'm not sure I
understand the change either, so if you have a better solution it's welcome.

> >-#nav-bar #back-button {
> >+#navigator-toolbox #nav-bar #back-button {
> >   -moz-margin-end: 0;
> > }
> > 
> >-#nav-bar #forward-button {
> >+#navigator-toolbox #nav-bar #forward-button {
> >   border-left: none;
> >   -moz-margin-start: 0;
> > }
> > 
> >-#nav-bar #back-button:-moz-locale-dir(ltr) {
> >+#navigator-toolbox #nav-bar #back-button:-moz-locale-dir(ltr) {
> >   border-top-right-radius: 0;
> >   border-bottom-right-radius: 0;
> > }
> > 
> >-#nav-bar #back-button:-moz-locale-dir(rtl),
> >-#nav-bar #forward-button {
> >+#navigator-toolbox #nav-bar #back-button:-moz-locale-dir(rtl),
> >+#navigator-toolbox #nav-bar #forward-button {
> >   border-top-left-radius: 0;
> >   border-bottom-left-radius: 0;
> > }
> 
> These changes don't seem to make sense.

They increase the specificity of the rules. Without this change, the
back and forward buttons would be separated by a margin. I thought
against using the "!important" modifier. Again, if you have a better
solution it's welcome.

Comment 73

[:Paolo Amadini]

Attachment: Revised patch, overriding the iconsize attribute

In the meantime, I'm attaching the revised patch, with all the requested changes
except for the three issues in the previous comment.

Comment 74

[Dão Gottwald [:dao]]

(In reply to comment #72)
> (In reply to comment #71)
> > >+  // Apply styling attributes on default and on custom toolbars.
> > >+  let XULNS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
> > >+  Array.forEach(gNavToolbox.getElementsByTagNameNS(XULNS, "toolbar"),
> > >+                function(toolbar) BrowserSetStylingAttributes(toolbar));
> > >+
> > >+  // The add-on bar is not in the toolbox.
> > >+  BrowserSetStylingAttributes(document.getElementById("addon-bar"));
> > 
> > Can you copy the logic from customizeToolbar's forEachCustomizableToolbar
> > function instead?
> 
> Not sure what you're asking... we should not filter out non-customizable
> toolbars here, so we can use getElementsByTagNameNS instead of enumerating
> all children and checking their attributes and local name manually.

You shouldn't filter out non-customizable toolbars, but you should use toolbox.childNodes and toolbox.externalToolbars.

I need to look more closely at the winstripe part to tell what's going on with the specificity.

Comment 75

[:Paolo Amadini]

(In reply to comment #74)
> You shouldn't filter out non-customizable toolbars, but you should use
> toolbox.childNodes and toolbox.externalToolbars.

I think this should work:

  Array.filter(gNavToolbox.childNodes, function(e) e.localName == "toolbar")
       .concat(gNavToolbox.externalToolbars).forEach(getToolbarIconsize);

> I need to look more closely at the winstripe part to tell what's going on with
> the specificity.

Thank you!

Comment 76

[Jez]

CCing self.  I don't think FF4 win and mac should drop a default set of 24x24 icons, personally.  Seems the Firefox graphics team is too lazy to develop 2 sets of icons.  What do they get paid for, again?

Comment 77

[:Paolo Amadini]

(In reply to comment #74)
> I need to look more closely at the winstripe part to tell what's going on with
> the specificity.

I found the rule for the margins that now has higher specificity:

#navigator-toolbox[iconsize="small"][mode="icons"] > #nav-bar .toolbarbutton-1 {
  margin-left: 2px;
  margin-right: 2px;
}

Thus the additional "#navigator-toolbox" is required only on these two rules:

#navigator-toolbox #nav-bar #back-button {
  -moz-margin-end: 0;
}

#navigator-toolbox #nav-bar #forward-button {
  border-left: none;
  -moz-margin-start: 0;
}

Comment 78

[:Paolo Amadini]

Attachment: Patch, with theme rule modifications minimized

This should be very near to final.

Comment 79

[Jez]

(In reply to comment #76)
> CCing self.  I don't think FF4 win and mac should drop a default set of 24x24
> icons, personally.  Seems the Firefox graphics team is too lazy to develop 2
> sets of icons.  What do they get paid for, again?

Apologies for my comments.  I posted this in the heat of the moment and should've been more civil with what I said.

Comment 80

[:Paolo Amadini]

(In reply to comment #78)
> This should be very near to final.

The tryserver run was successful, the two test suite failures seem unrelated:

http://tbpl.mozilla.org/?tree=MozillaTry&rev=2d8c6cddbc58

There are also Windows, Mac and Linux builds available. They're not based
on the most recent changeset in mozilla-central, but they can be used by
anyone to test the latest version of the patch on all platforms:

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/
paolo%2Emozmail%40amadzone%2Eorg-2d8c6cddbc58/

Comment 81

[:Paolo Amadini]

Today's the last day I can spend on this bug before I leave for a week.

There's little, if anything, left in the patch that's not already been
reviewed, so I think any concern should be fast to handle even if another
iteration is required. The "monochrome" part should be likewise quick to
handle in a separate bug. I'm available for this during the next few hours.

Comment 82

[Dão Gottwald [:dao]]

Comment on attachment 509135 [details] [diff] [review]
Patch, with theme rule modifications minimized

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
>@@ -1308,16 +1308,18 @@ function BrowserStartup() {
>   TabsOnTop.syncCommand();
> 
>   BookmarksMenuButton.init();
> 
>   TabsInTitlebar.init();
> 
>   gPrivateBrowsingUI.init();
> 
>+  getToolbarIconsizesFromTheme();

Since "get" usually implies a return value, maybe "retrieveToolbarIconsizesFromTheme" would be better. Or maybe you have a better idea.

>+function BrowserToolboxCustomizeChange(aType) {
>+  switch (aType) {
>+    case "iconsize":
>+      // The iconsize property might have changed.

This comment isn't helpful. The iconsize surely has changed, that's why the function gets called.

>+      getToolbarIconsizesFromTheme();
>+      break;
>+    case "mode":
>+      // Do nothing.

A theme might actually want to differentiate between icons and icons+text mode, so maybe we should move this up.

>+    default:
>+      // Button positions might have changed.

Again not really a helpful comment, slightly misleading even. Can be removed.

>+  function getToolbarIconsize(aElement) {

aElement should be aToolbar

>+  // Apply styling attributes on default and on custom toolbars.

Bogus comment, can be removed.

>--- a/browser/themes/winstripe/browser/browser.css
>+++ b/browser/themes/winstripe/browser/browser.css

>-#navigator-toolbox > toolbar:not(#toolbar-menubar):not(#TabsToolbar)[iconsize="small"],
>-#navigator-toolbox > toolbar:not(#toolbar-menubar):not(#TabsToolbar)[defaulticonsize="small"]:not([iconsize]) {
>+#navigator-toolbox > toolbar:not(#nav-bar):not(#toolbar-menubar):not(#TabsToolbar)[iconsize="small"],
>+#navigator-toolbox > toolbar:not(#nav-bar):not(#toolbar-menubar):not(#TabsToolbar)[defaulticonsize="small"]:not([iconsize]) {
>   padding-top: 1px;
>   padding-bottom: 1px;
> }

This change causes the nav bar to lose it's padding in small icons mode.
I think you need this instead:

#navigator-toolbox[iconsize="small"] > #nav-bar,
#navigator-toolbox > toolbar:not(#nav-bar):not(#toolbar-menubar):not(#TabsToolbar)[iconsize="small"],
#navigator-toolbox > toolbar:not(#nav-bar):not(#toolbar-menubar):not(#TabsToolbar)[defaulticonsize="small"]:not([iconsize]) {
  padding-top: 1px;
  padding-bottom: 1px;
}

I'm not sure the third selector is needed, actually.

>-#nav-bar #back-button {
>+#navigator-toolbox #nav-bar #back-button {
>   -moz-margin-end: 0;
> }
> 
>-#nav-bar #forward-button {
>+#navigator-toolbox #nav-bar #forward-button {
>   border-left: none;
>   -moz-margin-start: 0;
> }

Please use !important instead.

Comment 83

[:Paolo Amadini]

Thanks for your comments! I'm preparing the next patch right now. There's
only one thing I noticed:

> >-#nav-bar #back-button {
> >+#navigator-toolbox #nav-bar #back-button {
> >   -moz-margin-end: 0;
> > }
> > 
> >-#nav-bar #forward-button {
> >+#navigator-toolbox #nav-bar #forward-button {
> >   border-left: none;
> >   -moz-margin-start: 0;
> > }
> 
> Please use !important instead.

Curiously, using "!important" breaks the unified back-forward button in large
icons mode, so I guess have to find the other affected rule and put !important
there too. Should I do this, or in the light of this use the original solution
above?

Comment 84

[:Paolo Amadini]

Attachment: Revised patch, with theme rule modifications minimized

Meanwhile, revised patch except for the above comment.

Comment 85

[:Paolo Amadini]

Dão, I have to finish this today. What's the status of the latest patch for you?

Thank you in advance for your availability!

Comment 86

[Dão Gottwald [:dao]]

Comment on attachment 509603 [details] [diff] [review]
Revised patch, with theme rule modifications minimized

>+function retrieveToolbarIconsizesFromTheme() {
>+  function retrieveToolbarIconsize(aToolbar) {

Add this here:

if (aToolbar.localName != "toolbar")
  return;

>+    // The theme indicates that it wants to override the "iconsize" attribute
>+    // by specifying a special value for the "counter-reset" property on the
>+    // toolbar. A custom property cannot be used because getComputedStyle can
>+    // only return the values of standard CSS properties.
>+    let counterReset = getComputedStyle(aToolbar).counterReset;
>+    if (counterReset == "smallicons 0") {
>+      aToolbar.setAttribute("iconsize", "small");
>+      document.persist(aToolbar.id, "iconsize");
>+    } else if (counterReset == "largeicons 0") {
>+      aToolbar.setAttribute("iconsize", "large");
>+      document.persist(aToolbar.id, "iconsize");
>+    }
>+  }
>+
>+  Array.filter(gNavToolbox.childNodes, function(e) e.localName == "toolbar")
>+       .concat(gNavToolbox.externalToolbars).forEach(retrieveToolbarIconsize);

And use this instead:

Array.forEach(gNavToolbox.childNodes, retrieveToolbarIconsize);
gNavToolbox.externalToolbars.forEach(retrieveToolbarIconsize);

>-#nav-bar #back-button {
>+#navigator-toolbox #nav-bar #back-button {
>   -moz-margin-end: 0;
> }
> 
>-#nav-bar #forward-button {
>+#navigator-toolbox #nav-bar #forward-button {
>   border-left: none;
>   -moz-margin-start: 0;
> }

Replace with:

#nav-bar #back-button {
  -moz-margin-end: 0 !important;
}

#nav-bar #forward-button {
  border-left-style: none;
  -moz-margin-start: 0 !important;
}

The other !important you need to add is for -moz-margin-start: -6px on #navigator-toolbox[iconsize="large"][mode="icons"] > #nav-bar #forward-button.

r=me with that. Thanks for your patience :)

Comment 87

[Pardal Freudenthal (:ShareBird)]

Sorry for spamming this bug, but I just want to say thank you both, Paolo and Dão, for your efforts in this bug!

Comment 88

[:Paolo Amadini]

(In reply to comment #86)
> The other !important you need to add is for -moz-margin-start: -6px on
> #navigator-toolbox[iconsize="large"][mode="icons"] > #nav-bar #forward-button.

Thank you for finding this, I'll do the changes right now.

> r=me with that. Thanks for your patience :)

Thanks to you! One last thing, if you have time, I'd ask you is to have a
look at the patch on bug 631356. I'll update it to reflect the changes in
your last comment, but meanwhile, do you think I should not factor out the
forEachToolbar function and use the simpler solution above instead?

(In reply to comment #87)
> Sorry for spamming this bug, but I just want to say thank you both, Paolo and
> Dão, for your efforts in this bug!

That's much appreciated, thank you.

Comment 89

[Dão Gottwald [:dao]]

(In reply to comment #88)
> do you think I should not factor out the
> forEachToolbar function and use the simpler solution above instead?

I think we can manage well without that function.

Comment 90

[:Paolo Amadini]

Attachment: Final patch

Comment 91

[Dão Gottwald [:dao]]

Comment on attachment 509631 [details] [diff] [review]
Final patch

>--- a/browser/themes/winstripe/browser/browser.css
>+++ b/browser/themes/winstripe/browser/browser.css

> #nav-bar #forward-button {
>   border-left: none;
>-  -moz-margin-start: 0;
>+  -moz-margin-start: 0 !important;
> }

Please replace border-left with border-left-style while you're at it. Makes the computed style much nicer to look at in DOM Inspector.

Comment 92

[:Paolo Amadini]

Attachment: Final patch

(In reply to comment #91)
> Please replace border-left with border-left-style while you're at it. Makes the
> computed style much nicer to look at in DOM Inspector.

Here you are :-)

Comment 93

[:Paolo Amadini]

To recap, for extension authors this is a substantial improvement because
the following stylesheet, which is already the one recommended in existing
tutorials and documentation, now allows add-ons to use icons of the correct
size in all toolbars on both Firefox 4 and other applications:

#my-button {
  list-style-image: url(icon24.png);
}

toolbar[iconsize="small"] #my-button {
  list-style-image: url(icon16.png);
}

The 24x24 icon is used in the customization palette on all platforms.

Extensions still need to differentiate the platform in chrome.manifest if
they want to use a monochrome icon on Mac, although this would affect all
the themes on Mac, even colorful ones. This is until bug 631356 is fixed.

Comment 94

[:Paolo Amadini]

Closing remarks: I wish to say thank you to all the people that provided
feedback, test cases, first-pass review and support, and especially to Dão
for all the time he dedicated to detailed reviewing!

Comment 95

[Pardal Freudenthal (:ShareBird)]

Sorry, I didn't have the time to follow all the discussion and the changes to the original patch.
AFAICS, this will not use anymore the "wantediconsize" attribute. Could you please explain what does this mean:
// The theme indicates that it wants to override the "iconsize" attribute
// by specifying a special value for the "counter-reset" property on the
// toolbar.
?

Comment 96

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/847a825087f2

Comment 97

[Jez]

Does this allow for themes to 'want' 2 icon sizes?  I still think it'd be a good idea to have a checkbox for 'user small icons' - or maybe change that to a dropdown list of small/large.  Otherwise I think you'll see a lot of 'duplicate' themes; theme 'foo' with small icons, theme 'foo' with large icons, theme 'bar' with small icons, theme 'bar' with large icons.  Heck, maybe even with medium icons.

I think that along with this, some mechanism to let the user select an icon size and let the theme provide multiple icon sizes would be a good idea.  The obvious two are 16x16 and 24x24 as they are the ones that we have at the moment and so nearly all themes and extensions will have them already.

Comment 98

[Jez]

Just to add to my previous thought, maybe 16x16 could become 'small', 24x24 could become 'medium', and 32x32 could become 'large'.

Looking at the code, it looks like you're forcing small icons for the pinstripe theme.  Why is it a good idea to remove choice from a user arbitrarily because they're on a particular platform?  That is to say, you actually have to go to extra effort to reduce their choice, not less.

Comment 99

[:Paolo Amadini]

(In reply to comment #95)
> Could you please explain what does this mean:
> // The theme indicates that it wants to override the "iconsize" attribute
> // by specifying a special value for the "counter-reset" property on the
> // toolbar.

Yes, sorry, I outlined the changes for add-on authors but didn't mention how
it works for theme authors. By the way, everything should be covered in the
MDC documentation when Firefox 4 beta 12 is released. The information for
add-on authors should appear at <https://developer.mozilla.org/en/Extensions/Updating_extensions_for_Firefox_4#Toolbar_button_icons>, while for theme
authors the following information should appear in
<https://developer.mozilla.org/en/Theme_changes_in_Firefox_4>.

The icon size that add-ons will use for toolbar buttons in the main browser
window is determined by the "iconsize" attribute of the toolbar. Currently,
if no special rule is introduced in the theme, the "iconsize" attribute on
each individual toolbar is set according to these rules:

* Menu bar: Always "small"
* Navigation bar: Depending on user preference, either "small" or "large"
* Bookmarks toolbar: Always "small"
* Tab bar: Always "small"
* Add-on bar: Always "small"
* Custom toolbars: Depending on user preference, either "small" or "large"

Small add-on icons are 16x16 pixels and large icons are 24x24 pixels. Note
that this is true only if the add-on uses the correct stylesheet and images.

Themes can override the value of the "iconsize" attribute that is set on
each toolbar by specifying a special value for the "counter-reset" property
on the toolbar. Use "largeicons" to force the "iconsize" attribute to "large",
or "smallicons" to force the "iconsize" attribute to "small".

Sometimes, the "counter-reset" property will be set using a CSS rule that
is applied depending on the user preference for using large or small icons.
This preference is reflected as-is in the "iconsize" attribute on the
*navigator toolbox* (that is, the container of the toolbars at the top
of the screen, and not the individual toolbar elements).

For example, the default Windows theme wants to force small icons on the
navigation bar regardless of the user preference. It uses this rule:

#nav-bar {
  counter-reset: smallicons;
}

If (as a theoretical example) you want to use large icons for add-on buttons
in the bookmarks toolbar when the related user preference is set:

#navigator-toolbox[iconsize="large"] > #PersonalToolbar {
  counter-reset: largeicons;
}

To use large icons everywhere (including the menu bar):

#navigator-toolbox > toolbar, #addon-bar {
  counter-reset: largeicons;
}

Comment 100

[Jez]

(In reply to comment #99)
> Small add-on icons are 16x16 pixels and large icons are 24x24 pixels. Note
> that this is true only if the add-on uses the correct stylesheet and images.

How does this fit in with the default 18x18 icons that I've been told will be the Windows theme size, and the 20x20 icons that I've been told will be the Max theme size?

> Sometimes, the "counter-reset" property will be set using a CSS rule that
> is applied depending on the user preference for using large or small icons.
> This preference is reflected as-is in the "iconsize" attribute on the
> *navigator toolbox* (that is, the container of the toolbars at the top
> of the screen, and not the individual toolbar elements).

Can you confirm or deny what I've heard which is that the UX team plans to remove this user preference in a future version of Firefox, with the icon size being determined 100% by the current theme?

> If (as a theoretical example) you want to use large icons for add-on buttons
> in the bookmarks toolbar when the related user preference is set:
> 
> #navigator-toolbox[iconsize="large"] > #PersonalToolbar {
>   counter-reset: largeicons;
> }
> 
> To use large icons everywhere (including the menu bar):
> 
> #navigator-toolbox > toolbar, #addon-bar {
>   counter-reset: largeicons;
> }

How would you use large icons for add-on buttons in the addon bar when the related user preference is set?

Comment 101

[Jez]

s/Max/Mac/ above

Comment 102

[:Paolo Amadini]

(In reply to comment #100)
> How does this fit in with the default 18x18 icons that I've been told will be
> the Windows theme size, and the 20x20 icons that I've been told will be the
> Mac theme size?

As far as I can tell, those dimensions include padding and allow for a subtle
glow for default buttons on Windows, but the actual image is 16x16 pixels, so
the appearance of the images is the same for built-in buttons and add-ons. See
also bug 618096 for Mac.

> Can you confirm or deny what I've heard which is that the UX team plans to
> remove this user preference in a future version of Firefox, with the icon size
> being determined 100% by the current theme?

No, I can't. Maybe you should ask someone of the UX team :-)

> How would you use large icons for add-on buttons in the addon bar when the
> related user preference is set?

A theme cannot set the icon size on the add-on bar based on the current user
preference, right. It could become possible with a modification. If you have
this need, maybe you want to file a separate bug on that.

Comment 103

[Dão Gottwald [:dao]]

There's no user-facing change.

Comment 104

[:Paolo Amadini]

Jorge, thank you for your clear explanation of the effects of this bug in the
Mozilla Add-ons blog! Yes, this specific bug applies to beta 12 indeed.

I'd like to update the MDC documentation at the right time too, maybe I'll seek
some help for a one-time review of English grammar and style. I'm putting the
dev-doc-needed keyword to track the need to update the Developer Center docs.

Comment 105

[Eric Shepherd [:sheppy]]

Paolo: if you'd like to update this article, I will be happy to review and tidy it up:

https://developer.mozilla.org/en/Theme_changes_in_Firefox_4

Comment 106

[:Paolo Amadini]

(In reply to comment #105)
> Paolo: if you'd like to update this article, I will be happy to review and tidy
> it up:
> 
> https://developer.mozilla.org/en/Theme_changes_in_Firefox_4

Thank you very much! What do you think of working on the article after beta 12
is released, in a few days from now, to avoid confusion in the meantime? It
seems that the individual beta numbers where a feature becomes available are
not usually specified in MDC articles.

Comment 107

[:Paolo Amadini]

Eric, I've updated this article:

https://developer.mozilla.org/en/Theme_changes_in_Firefox_4