1288747 - Define common styles for panels and icon buttons

Status: NEW

(Firefox :: Theme, defect)

Description

[:Paolo Amadini]

The main menu, the redesigned doorhangers from bug 1267604, and the redesigned Downloads Panel from bug 1269962 will all share a similar footer style, with new style buttons using the colors we already defined for in-content styles.

We are also adding an icon button inside the Control Center in bug 1203292 for removing granted permissions, and we will use the same style of button in the Downloads Panel.

The new style of footer was only used in the main menu, so the styles that define it are in panelUI.inc.css, like the button height of 40px, and are associated to specific element IDs rather than classes.

It's not clear to me if panelUI.inc.css should also contain generic classes usable by other buttons in panels. There seem to be some generic <panelmultiview> styling there, but not all panels will necessarily use the multiview.

Defining styles for icon buttons is also very verbose - we always have to revert styling defined by default for <button> or <toolbarbutton> bindings, like hiding the element reserved for the button text. Again, we do this for each specific button. Ideally we would have a class that does that for all icon buttons, or maybe we should even have a separate binding!

I'd like to fix this situation, but I'm not sure of the best file layout, or even if we want to share some parts, like the color of the buttons, with the in-content CSS.

Dão, Gijs, Jared, any thoughts?

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Some thoughts:

panelUI.inc.css was created for the Firefox menu. It is called "panelUI" because we didn't have a good name internally for the new Firefox menu. Whether we would call that menu the hamburgerMenu, threeSausageMenu, or applicationMenu is up for debate. With that being said, I don't think these shared rules should exist in that file.

<panelmultiview> is very new, in that it was only created for Australis. Because of that, we ended up with some styles referencing the tagName when they should have probably referenced classNames or specific IDs. These should get changed to not style based on the tagName.

A separate binding for these probably makes most sense, and with that we can have a separate CSS file in /browser/themes/shared that defines the themed styling for this new binding. /browser/base/content would hold the CSS that attaches the binding and any other 'content' styling, obviously.

Ideally we would also share the colors. We have been using a blue color tone in places, and sharing the color through a CSS variable would prevent us from accidentally using the wrong color in places.

Comment 2

[Stephen Horlander [:shorlander]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #1)
> Some thoughts:
> 
> panelUI.inc.css was created for the Firefox menu. It is called "panelUI"
> because we didn't have a good name internally for the new Firefox menu.
> Whether we would call that menu the hamburgerMenu, threeSausageMenu, or
> applicationMenu is up for debate. With that being said, I don't think these
> shared rules should exist in that file.

This probably doesn't add anything useful to the problem of how to structure this, but I have always found the "panelUI" name a little confusing. We have a lot of Panel based UI, not just the application menu. FWIW I would call it the Application Menu, since that is what it is and it might not always be three dots or a hamburger ;)

> Ideally we would also share the colors. We have been using a blue color tone
> in places, and sharing the color through a CSS variable would prevent us
> from accidentally using the wrong color in places.

This would be really great. Any way we can centralize these common styles would be helpful to avoid duplication and drift. For example our current problem of many shades of blue, green, red, yellow, etc.

Comment 3

[:Paolo Amadini]

I'm still trying to wrap my head around the current structure of the theme files. Especially to determine how much these shared rules should be integrated with or separated from other theme files.

Most of the modern styles we're currently integrating are defined here:

https://firefoxux.github.io/StyleGuide/#/introduction

This interactive document shows what areas the new common styles will cover, for example colors, size of controls, hover and focus behavior. These will be mostly independent from the operating system, although some user interfaces look desktop-specific to me. There will be small differences in appearance between platforms and operating system versions, for example flat or rounded corners.

We'll need to define CSS variables and rules to cover the situations indicated in the style guide, for example to style a primary button.

Whether these should live in "toolkit" or "browser" may be the easy part. The in-content CSS file that already relies on the style guide is currently in Toolkit, and the <popupnotification> binding that will depend on these styles is in Toolkit too. I _guess_ this is because these styles are shared by Android? If that's not the case and this is desktop-specific then _maybe_ these all belong in "browser", but even moving all of them may be out of scope.

Then there is the question of whether these style rules and definitions should be included by default whenever the "global.css" Toolkit file is included. Maybe they shouldn't, since we have XUL windows other than the browser window that don't use the style guide and may still show classic platform-specific controls only. On the other hand I guess that just including the rules won't apply the styles automatically, so maybe this isn't an issue.

A similar but different question is, even if including "global.css" implies including the new styles, whether they should be stored in a separate, included file.

Then we have a lot of small CSS files in Toolkit for individual platform controls, like standard buttons. Probably these should be left alone, as they're usually very different across platforms and will not rely on the new style guide.

But we also have similarly-looking small files for bindings that are not really for platform controls, but only define our own interface constructs, for example the <notification> and <popupnotification> binding. These are now evolving to depend on the modern style guide, that will be the only style that they will use.

The new <popupnotification>, to pick one as an example, will depend on the new CSS variables and rules, for example to style primary buttons or icon buttons inside it. The binding already relies on CSS not explicitly included in the binding itself, for example to style the close button. It's already an implied dependency on "global.css", and using the modern styles will now imply a dependency on them too, in other words these bindings will only be usable in windows that include the new styles globally.

For platform controls like standard buttons I'm not sure if we have this implied dependency from "global.css" already, in other words what happens if you have a XUL window where you don't include "global.css" and use a <button> or another control in there.

There is also the question of whether panel-specific styles, like the footer height, should be  separated into a panel-specific CSS file, maybe simply included by the main CSS file or included where necessary. Or in general the granularity we want to use for styles bound to individual new-style controls. I'm thinking that, because the style guide aims to unify visual elements across different kinds of controls, maybe the right direction is to define all these styles centrally. We would have all the variables first, and then all the control-specific rules, like we do for the current in-content CSS.

Ah, the in-content CSS will need to include the shared modern style definitions too, but may end up needing just the variables. Maybe we need another separation between parts shared by chrome and in-content, and parts used only by chrome. The in-content styling overrides the default control styles without the need for specific classes, and I think we don't want that for chrome, where for now we need to mix old and new style controls.

For the first application of this new system, I'd like to share the colors of the primary button, the size and border color of the panel footer, and the dropmarker for the menu-button. It's a few lines, but where to place them partly depends on the above questions, so it's not that easy and immediate!

Comment 4

[:Paolo Amadini]

I'll try to put together a patch in bug 1291682 and ask Jared for review. Feel free to share any opinions or pointers with regard to comment 3 in the meantime.

Comment 5

[:Paolo Amadini]

Also related to the modern control styles, is there any way to avoid the platform styles completely instead of having to override them, which sometimes also requires "!important" rules?

Take the example of the <toolbarseparator>. We use it in a custom way to separate buttons in the footer of the application menu:

https://dxr.mozilla.org/mozilla-central/rev/6608e5864780589b25d5421c3d3673ab30c4c318/browser/components/customizableui/content/panelUI.inc.xul#51

The default styles are defined here for Windows:

https://dxr.mozilla.org/mozilla-central/rev/6608e5864780589b25d5421c3d3673ab30c4c318/toolkit/themes/windows/global/toolbar.css#51

We have to give them a new style including "-moz-appearance: none". Here is the code for Windows:

https://dxr.mozilla.org/mozilla-central/rev/6608e5864780589b25d5421c3d3673ab30c4c318/browser/themes/shared/customizableui/panelUI.inc.css#592

Note that in the code above we currently use an ID rule but we'll need a class, but it's less relevant for this discussion anyways.

One issue here is that for Linux the platform styling is different:

https://dxr.mozilla.org/mozilla-central/rev/6608e5864780589b25d5421c3d3673ab30c4c318/toolkit/themes/linux/global/toolbar.css#47

Note the "!important". So we have additional special styling for Linux to override just that:

https://dxr.mozilla.org/mozilla-central/rev/6608e5864780589b25d5421c3d3673ab30c4c318/browser/themes/linux/customizableui/panelUI.css#99

Four files for just a vertical line, and this is a very simple case. For someone new to the code and trying to implement the new style guide, it's a really complex situation to handle.

Maybe we could/should just use a different tag in the example above, but we also have widgets defined in XBL bindings that have behavior associated with them, for example buttons, which incur in the same override problem. Is there a way to simplify the styling situation for them too?

P.S.: Since there is discussion about deprecating full custom themes, I'll mention that the complexity discussed here is not related to support for custom themes, it's mostly orthogonal. Part of this complexity is instead due to the support for platform-specific widget styles in product, which I guess we'll continue to have in various places.

Comment 6

[Dão Gottwald [:dao]]

(In reply to :Paolo Amadini from comment #5)

toolkit/themes/linux/global/toolbar.css just shouldn't be using !important there.

Comment 7

[:Paolo Amadini]

The general issue with the approach of overriding platform styles is that we waste hours of our time and dozens of lines of code in chasing these small declarations to override.

Bug 1291998 is another recent example, but regardless of the specific examples of <toolbarseparator> or <button>, I suspect that everyone who has written or reviewed front-end CSS overall has spent hours between MXR, the debugger and testing just for checking such small changes.

Is there a better way?

Comment 8

[:Paolo Amadini]

It seems to me that, for certain instances of controls, we should not apply the platform styling in the first place instead of reverting it. We probably need these controls to co-exist with traditional ones because on one hand it doesn't seem realistic to convert all places at once, and on the other hand even in the long term we might still need both styles in the same window or in different windows.

This means we should declare the intent of using different styling, we could have for example a <button class="modern"> or <button styling="modern"> that co-exists with a normal platform <button>.

Excluding platform styles by adding selectors to the platform rules like "button:not([styling])" seems unfeasible, because this would increase specificity and cause all kind of issues.

We could keep the same <button> tag and associate it to a different XBL binding inside a rule like "button[styling='modern']". The binding would share all the functionality through inheritance, without importing the <stylesheet> section. This would work well for most controls like buttons whose general styling is not defined inside "global.css", but may not work for all cases.

Another way is to use a different tag entirely associated to the binding, examples may include <iconbutton>, <modernbutton>, <plaindescription>, <plaindescription class="modern">, or something like that. This will probably result in the highest simplification of our front-end styling.

One good example of the difference between a <plaindescription> and a <description class="plain"> is that the latter uses styling overrides, introducing both higher specificity and "!important" rules:

https://dxr.mozilla.org/mozilla-central/rev/763fe887c37cee5fcfe0f00e94fdffc84a41ea1c/toolkit/themes/osx/global/global.css#169

This has always made it difficult to do further restyling on top of the "plain" styling. This would not be an issue with a <plaindescription>.

Comment 9

[Dão Gottwald [:dao]]

First of all, I'll note that overriding default styles never struck me as a huge deal. Our toolkit stylesheets are centralized and easy to find. Even if toolkit stylesheets do stupid things, it's pretty easy to deal with that.

If we can make this simpler, that's good, but I don't think involving XBL let alone new element names is a can of worms we want to open for that.

> This means we should declare the intent of using different styling, we could
> have for example a <button class="modern"> or <button styling="modern"> that
> co-exists with a normal platform <button>.

Btw, OS themes are changed and updated all the time, so there's nothing inherently modern about not using the platform style. class="modern" would invite misconception that we should avoid.

> One good example of the difference between a <plaindescription> and a
> <description class="plain"> is that the latter uses styling overrides,
> introducing both higher specificity and "!important" rules:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> 763fe887c37cee5fcfe0f00e94fdffc84a41ea1c/toolkit/themes/osx/global/global.
> css#169
> 
> This has always made it difficult to do further restyling on top of the
> "plain" styling. This would not be an issue with a <plaindescription>.

The intent of the plain class isn't entirely clear to me, but I think it's different from what you'd want here. Clearly a class resetting the default styling shouldn't use !important. When it seems like it would have to use !important, we should treat that as a bug.

Comment 10

[Dão Gottwald [:dao]]

One more thing: many toolkit stylesheets especially on Windows set -moz-appearance:foo and then specify borders and backgrounds and whatnot additionally. This was necessary as a fallback on Windows 2000 I think and other platforms where widget code wouldn't implement the platform appearance. I don't think this is relevant anymore, so we can probably simplify those stylesheets a lot.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

This is quite the tome.

I'd say that it's my understanding that some of the styles are placed in /toolkit because the controls are defined in /toolkit, even though they may not be used by Android. They may be used by SeaMonkey though.

To keep things simple, these new styles should just be defined in /toolkit. I would also prefer that controls opt-out of this styling instead of needing to opt-in to it.

I also think that we can use this styling for in-content styles as well as chrome styles. The preferences UI uses special designed buttons, but these are basically the next version of those. No need to keep two similar but different cross-platform buttons defined. We probably don't want to change webpage content controls with this though.

Comment 12

[:Paolo Amadini]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #11)
> We probably don't want to change webpage content controls with this though.

Yeah, this is why we might need different controls, substantially an opt-in.

However it seems from the previous comments on the bug that, despite my recent experience to that effect, there isn't a consensus that having to override the default styles hinders front-end productivity. So, even if I had the opportunity to work on this aspect between other things I'm doing, it doesn't make sense for me to do that, and this discussion needs to move forward among developers more familiar with Firefox theme code anyways.

Comment 13

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #10)
> One more thing: many toolkit stylesheets especially on Windows set
> -moz-appearance:foo and then specify borders and backgrounds and whatnot
> additionally. This was necessary as a fallback on Windows 2000 I think and
> other platforms where widget code wouldn't implement the platform
> appearance. I don't think this is relevant anymore, so we can probably
> simplify those stylesheets a lot.

I started doing this in bug 1334429.

Comment 14

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #13)
> (In reply to Dão Gottwald [:dao] from comment #10)
> > One more thing: many toolkit stylesheets especially on Windows set
> > -moz-appearance:foo and then specify borders and backgrounds and whatnot
> > additionally. This was necessary as a fallback on Windows 2000 I think and
> > other platforms where widget code wouldn't implement the platform
> > appearance. I don't think this is relevant anymore, so we can probably
> > simplify those stylesheets a lot.
> 
> I started doing this in bug 1334429.

Filed a meta bug to further track that work: bug 1341048