807130 - Building blocks: clean up header.css

Status: RESOLVED DUPLICATE of bug 893707

(Firefox OS Graveyard :: Gaia, defect)

Description

[Gordon Brander :gordonb]

Problem
-------

Building Blocks' header.css implementation has a couple of issues:

**Inefficient CSS selectors.** For example:
  
      section[role="region"] > header:first-child:after { ... }
      section[role="region"] > header:first-child form button[type="reset"] {}

This type of deep complex selector has a negative performance effect, and is completely unnecessary.

**Brittle CSS selector structure** -- e.g. the style of a unit depends heavily on parent's markup structure. This type of approach invites unforeseen visual bugs due to cascade, unanticipated markup structures, etc.

**No comments on how to use**. We should include comments with shared styles that describe their intended uses. Otherwise, you end up reverse-engineering markup from selectors (which are a lossy way to describe markup).

User story
----------

As a front-end developer, I want to have reusable building blocks that are composable, reliable, and agnostic about what tag they are applied to.

**Composable** because I want to be able to mix building blocks with my own interface elements, without having to re-implement because of styling collisions.

**Reliable** because it is important that styles consistently "just work" when applied to the correct markup pattern. I don't want to have to stress out about the cascade.

**Tag agnostic** because I should be able to pick the most semantic, accessible markup for a given scenario, not be boxed into the scenarios that a style author could think of offhand when he authored his styles. Also, style authors can never anticipate all the ways they may want to use a style in future.

Proposed approach
-----------------

A lot of thought has been given to this topic[1]. It all boils down to taking a modular approach to creating CSS styles.

For example, we could express header styles as a module using either a classname or aria role. I'll use classnames:

    .header { ... }
    .header .title { ... }
    .header input { ... }
    .header .icon-add { ... }
    ...

All styles are scoped to their given module. Modules don't care about what tags you use (made an exception for input, which contains no elements), so .icon-add could be a <button>, or <a> tag, or <input> as the case demands.

Selector is kept short (usually no more than one level deep) for better performance. classnames are preferred for their flexibility, readability and speed.

Header styles are also not dependent on the structure of the parent. A module can be thought of as a self-contained unit, more or less.

Building in this way also makes responsive layouts easier -- a module should expand to fill the width of it's containing unit.

---

Here's a full list of apps that currently use shared/style/headers.css: http://pastebin.mozilla.org/1892654

---

[1] On modular front-end architecture:

* [Nicolas Gallagher has a good post on this concept](http://nicolasgallagher.com/about-html-semantics-front-end-architecture/
).
* [OOCSS](http://oocss.org/) is also a pretty complete take on these concepts.
* [Another, similar take from Snook.ca](http://smacss.com/book/).
* [Mailchimp also does a good job](http://www.flickr.com/photos/littlemad/5729783584/sizes/o/in/photostream/)

Comment 1

[Ismael Gonzalez [:basiclines]]

I'm totally agree with the proposed approaches.
This was my first intentionality, if you look at "bootzilla", the ancestor of building-blocks: https://github.com/basiclines/bootzilla maybe you can see a lot of your pruposes, maybe the file structure was not good enough, but the CSS was organized in performance/reusable friendly way: https://github.com/basiclines/bootzilla/blob/master/style/css/base/app.css

Maybe you should talk with Jose Manuel Cantera or Fabien Kazenave that are the ones that had a very strong opinion in how to use the selectors and the philosophy of this project.

Thank you for your feedback!

Comment 2

[Gordon Brander :gordonb]

Header: visual issues

* Hit colors are incorrect.
* Visual separator between icons should not appear unless there is more than one action.

Comment 3

[Ismael Gonzalez [:basiclines]]

Please provide documents or tangible specifications for that visual issues.
* Hit colors are correct at least it changed in the latest weeks
* The visual separator should appear always for the last icon, even if it is the only one present.

There is the specs that i've been following, please let me know if there is any other ones with more updated information:

https://www.dropbox.com/sh/tr2cnlzbupg79yu/VtbEcrlVo5/_Production/Building%20Blocks/02%20Widgets/03%20Headers#f:OWD_03_Headers.jpg

Thx

Comment 4

[Gordon Brander :gordonb]

Thanks for your help Ismael! I was making a "note to self" so I could come back to it and check my assumptions against comps.

It looks like the hit target colors do match the comp, but they look strange to me for these reasons:

* The orange + bright blue combo might cause issues for people that are red/green colorblind.
* The blue matches Gaia in other places, but clashes with the orange because it has a similar saturation and brightness.

Perhaps we should consider a design tweak: hit targets that match the color but darker, giving a depressed look?

Thoughts?

Comment 5

[Ismael Gonzalez [:basiclines]]

Ok sorry, someone cc'ed me on this ticket and i thought that it was a request.
I think also that the blue color as a hit state is not the more accurate one, maybe a darker orange will do the work.!

Comment 6

[Fabien Cazenave [:kaze]]

It’s a good time for this conversation to happen. :-)

(In reply to Gordon Brander :gbrander from comment #0)
> For example, we could express header styles as a module using either a
> classname or aria role. I'll use classnames:
> 
>     .header { ... }
>     .header .title { ... }
>     .header input { ... }
>     .header .icon-add { ... }
>     ...
> 

One of my main worries when using arbitrary class names is to keep this consistent and documented, because we’re going to have to support those for a long time. Adding classes on the fly does not look like a good way to reach this goal to me…

We already considered using CSS class names instead of attributes, and we’d be OK with that if:
 • CSS class names are prefixed (especially for common names such as “.header” or “.title”);
 • CSS class names represent a feature rather than a style — i.e. I think matching/extending ARIA roles makes sense in the long run;
 • all these class names are properly documented. [1]

> All styles are scoped to their given module. Modules don't care about what
> tags you use (made an exception for input, which contains no elements), so
> .icon-add could be a <button>, or <a> tag, or <input> as the case demands.
> 

This is a feature. The main goal is not to provide a flexible toolbox for front-end developers.

There are three goals we want to reach:
 • (dev request) ensure some markup consistency, precisely;
 • (UX request) enforce as much visual consistency as possible;
 • (UX+dev request) make it easy to deeply change the UI appearance with as little impact on the HTML markup as possible.

> Selector is kept short (usually no more than one level deep) for better
> performance. classnames are preferred for their flexibility, readability and
> speed.
> 

I see the point (especially performance-wise) but if we drop ARIA roles for arbitrary class names, it has to be done properly in order to keep it consistent on all building blocks. Before doing it on a per-component basis, I’d like to see how it would look like if we used arbitrary class names on all shared stylesheets.

Could you please propose a documented list of *all* classes that we’d use for the shared/style resources, like the one I started to write in the README.md file [1], before we can start changing all existing building blocks?

[1] https://github.com/mozilla-b2g/gaia/blob/master/shared/style/README.md

Comment 7

[Fabien Cazenave [:kaze]]

PS: some metrics regarding the possible performance gains would be nice to have. I have no doubt that shorter, class-based selectors will be more efficient, but I’d like to have an idea on their impact on the load time: ten percent, one percent?

Comment 8

[Gordon Brander :gordonb]

Thanks for your thoughts Fabien!

It sounds like we have a few goals floating around, and I'm pretty sure you and I are saying the same thing in different ways.

> * (dev request) ensure some markup consistency, precisely;

Completely agreed here. My experience is that it works best if you enforce this at a code review level. Tightly coupling markup and styles invites more bugs than it enforces markup (indeed, it looks like we're already experiencing some of these problems). Keeping tag names/deep selector structures out of CSS isn't so much about making a flexible front-end toolkit, it's about **loose coupling** and minimizing the factors that can cause bugs. As Postel's law points out, we'll end up with the the wrong thing happening if we make it difficult to do the right thing.

> * (UX request) enforce as much visual consistency as possible;

YES! The best way to do this is to make it easy to do the right thing (offering a library of styles that are easy to use).

> if we drop ARIA roles for arbitrary class names, it has to be done properly in order to keep it consistent on all building blocks

As I mentioned above, I'm actually ambivalent about whether we use classnames or aria roles. The code sample given above is only an example. If we want to use aria roles, and there is an aria role for everything we want to style, so much the better. Realistically speaking, we will probably want to use a mixture of aria roles and classes.

> * CSS class names are prefixed (especially for common names such as “.header” or “.title”);

Prefixes are fine by me, but obtuse prefixes like `mozb2g-title` will be unnecessary because building blocks are opt-in -- we don't have to worry much about naming collisions. We could do something to the effect of ".view-title", ".view-header", etc... whatever works as long as it makes sense.

> * CSS class names represent a feature rather than a style — i.e. I think matching/extending ARIA roles makes sense in the long run;

Agreed! But I don't want to get caught up too much in naming bikeshedding. Let's make sure we solve the problems at hand.

> Could you please propose a documented list of *all* classes that we’d use for the shared/style resources, like the one I started to write in the README.md file [1], before we can start changing all existing building blocks?

Realistically since Building Blocks are shared code, we will want to update them one at a time to avoid changing too many things at once and risk introducing bugs. We have a short timeline, so the goal would be to improve everything we can, probably starting with common elements that currently have visual inconsistencies. To this effect, I want to kick this off with a reference implementation of the header... that should give us a good idea if this is a good fit for the timeline. It also scopes the problem so we don't introduce bugs.

I've run an audit of all apps that currently use the shared header, so that should give us a picture of what our testing footprint should look like...

    browser.gaiamobile.org
    communications.gaiamobile.org
    costcontrol.gaiamobile.org
    email.gaiamobile.org
    feedback.gaiamobile.org
    gallery.gaiamobile.org
    homescreen.gaiamobile.org
    music.gaiamobile.org
    settings.gaiamobile.org
    sms.gaiamobile.org
    system.gaiamobile.org
    video.gaiamobile.org
    wallpaper.gaiamobile.org
    geoloc.gaiamobile.org

Thoughts?

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I honestly don't think changing the selectors in header.css would have any perceptible impact on performance.

If this not fix any real bugs, or need a rewrite to fix a small bug, I won't approve such code change unless you come with real data showing that there is a real gain.

Comment 10

[Gordon Brander :gordonb]

Agreed. At this point in the project, the focus for changes to building blocks should be on performance improvement or improved user experience. Any further improvement can wait until v2.

Comment 11

[Patryk Adamczyk [:patryk] UX]

The header font size is also too small, it should 2.5rem.