1144612 - [Messages][Refactoring] Make CSS more efficient by reducing the rule in tag category

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::SMS, defect)

Description

[Steve Chung [:steveck]]

Please ref MDN guide[1] that the single biggest cause of slowdown is too many rules in the tag category. In this bug we will need to revisit CSS again to remove the unnecesary rules and simply them into better shape.

[1] https://developer.mozilla.org/en-US/docs/Web/Guide/CSS/Writing_efficient_CSS#Try_to_put_rules_into_the_most_specific_category_you_can.21

Comment 1

[Steve Chung [:steveck]]

In the most of css guideline, the descendant/child selector is not recommended pattern and we should specify the element by semetic classname that could decouple from layout. Sadly the building block styling affect the app css deeply and make this goal harder to reach. For example message app is highly rely on shared list.css, but the super long rules like [data-type="list"] li > .pack-checkbox causes apps writting longer and more complicated rules to override BB. Ideally we should replace BB with gaia componenet in the future, but it means the refactoring scale is very limited before gaia component ready.

Hi Julien, I have some concern before refactoring the message css:
1) Is it meaningful to refactor BB, even we know it will be gone in the future?
2) Do you think we could only follow the rule partially, and keep the rules we need to override?

TBH I can't think of recatoring without BB, this limitation will make that chages more like polishing at most. But the future is still not clear yet, and wait from the uncertainty plan seems not a good move, especially we might have more release before v3 arrival.

Comment 2

[Julien Wajsberg [:julienw]]

In my opinion we should refactor BB CSS files one by one, one per bug, without changing the apps CSS unless there is an obvious incompatibility.

The only incompatibility is if some app CSS is overridden by the BB. Of course this should not normally happen but we never know what history can bring :)

An example (completely imaginary, but possible) is:

  // in HTML:
  <section role="dialog" data-type="confirm">
    <div>
      <p class='title'>
        Steve Chung
      </p>
    </div>
  </section>

  // in BB:
  [data-type=confirm][role=dialog] div p {
    padding: 0;
  }

  // in app:
  .title {
    padding-left: 5em;
  }


So if we change "<p class='title'>" with "<p class='title dialog-confirm-title'>" and the BB selector to ".dialog-confirm-title", the padding will be incorrect.

Of course, the app CSS was wrong, but it was not applied because of the specificity rule.

I expect that it should be rare in the project, but because of such issues we'll need to grep properly the project and test properly all cases.

Still this is IMO the right way to fix the BB: starting with the BB.

Comment 3

[Steve Chung [:steveck]]

Attachment: shared lists WIP

Ok, We all agree that BB is the first thing we need to deal with. It's just a WIP about the class rule naming(I think it's easier to start with a real BB):
- Starting with namespace -s to represent the class is shared BB
- Keep the last element tag name as the end of the class name. I'm not sure if it's a good nameing rule, because if some rules need to apply on different elements, I'll merge into another class name(EX: .-s-list-aside-transparent). Maybe we can rethink the structure and move some pattern into a rule instead of reply on element(like DRY css)

It's just a rough idea that starting from naming and this changes will need more html/js changes eventually. We'll need another bug for this patch atually, but just want to know if it's the right direction.

Comment 4

[Julien Wajsberg [:julienw]]

Comment on attachment 8592168 [details] [review]
shared lists WIP

Yeah, I think I like this :)

Anthony, what do you think?

Comment 5

[Anthony Ricaud (:rik)]

Comment on attachment 8592168 [details] [review]
shared lists WIP

This is definitely an improvement over the existing state because you have to explicitly choose the rules that apply. I like the namespacing (maybe s- is enough).

Next step is to rethink the component. For example the fact that an item is displayed in a list shouldn't imply the styling of the item. Names like pack-end and pack-span don't make sense to me. From the outside, you can't really understand the difference between -s-list-img and -s-list-icon. Maybe the edit states should be triggered by a modifier class instead of having edit in the name of the class?

Also one very nice feature of the current building blocks is the styleguide examples to see how to use them from HTML.

The title of this bug is also not the reason why I would refactor the building blocks. It's not about performance but about maintainability. So good first step but maybe not enough.

Comment 6

[Julien Wajsberg [:julienw]]

About styleguides: Probably the best and most maintainable should be an application (that could be run in Firefox so that we can also show it on github) that uses these styles.

About other comments: yes, good feedback!
This is a bigger wok but likely worth it -- if we keep these BB, that is.

Comment 7

[Steve Chung [:steveck]]

Comment on attachment 8592168 [details] [review]
shared lists WIP

Hi Wilson, sorry about the misleading bug title, but we're in discussion about the css styling convention for shread component, and we might apply this in gaia component in the future.

Since this WIP is mainly for shread lists in BB, and we also have a gaia-list component repo there. Would you suggest that we should polish gaia-list and apply them directly, or it's not ready for general use case?  It would be great if you could sharing the progress about the components and we can address the refactoring in gaia-component or shared BB.

BTW if you agree the concept about the styling convention(Use most specific category instead of tag name, avoid descendant/child selector and namespace prefix for gaia component), I can create a meta for it and maybe start this task from gaia-header first since it's applied in almost all the gaia apps, what do you think?

Comment 8

[Wilson Page [:wilsonpage]]

Right now this seems like a sensible direction if we can get quick perf gains. GaiaList component is of a very different visual style to the current building-block so might not be possible to use it until we begin some type of visual refresh.

Unperformant global selectors become less of an issue when shadow-dom comes into play. When the scope of selectors is dramatically reduced the global mess disappears and the engine can optimize style-recalc.

Comment 9

[Julien Wajsberg [:julienw]]

Mass closing of Gaia::SMS bugs. End of an era :(

Comment 10

[Julien Wajsberg [:julienw]]

Mass closing of Gaia::SMS bugs. End of an era :(