cozy / cozy-ui

React components and CSS styles for Cozy apps
https://cozy.github.io/cozy-ui/react/
MIT License
48 stars 37 forks source link

Find out why order of the CSS is not preserved #335

Open ptbrowne opened 6 years ago

ptbrowne commented 6 years ago

We have experienced problems with buttons that needed more specificity since order of the CSS rules was not preserved. Thanks @enguerran for the description below.

We need to find out why the order is not preserved in contacts.

Contexte

Dans cozy/cozy-contacts, on utilise un bouton (import { Button } from "cozy-ui/react/button") dans un menu (import { Menu } from "cozy-ui/react/Menu"). À ce bouton, on lui ajoute la classe narrow via l'attrinut extension :

<Button theme="secondary" extension="narrow">
  <Icon icon="dots" />
</Button>

Voir la doc

Mais dans l'app contacts, le css embarqué n'est pas dans le bon ordre et la règle de la classe c-bt--narrow est écrasée par la version de c-btn comme le montre cette capture

enguerran commented 6 years ago

Related to #328

enguerran commented 6 years ago

(2) ETU je peux supprimer un contact depuis sa fiche

CPatchane commented 6 years ago

Visiblement normalize.css est chargé en plein milieu de tous les autres styles. Le problème a aussi été rencontré dans cozy-store et normalize apparait à la ligne 77. En dev, il semble apparaitre ligne 1527 et du coup aussi après le style de la bar.

CPatchane commented 6 years ago

May be related to https://github.com/webpack/webpack/issues/215

GoOz commented 5 years ago

This is also happening on the close modal button on Styleguidist.

Button styles are loaded before in Modal but are still output after close modal button styles causing a failing override.

y-lohse commented 5 years ago

tl;dr : I don't know what the problem is, but use the transpiled version of cozy-ui and you should be fine.

I've spent some time investigating this issue. It doesn't look like I'll get to the bottom of it this time, so leaving notes for the next person who digs into it.

Right now (125519e714967620a83e65052c97b00d954aeed3) the problem can be seen in the react styleguidist, so that's what I focused on. I did not investigate any of the other symptoms mentioned in this issue.

The problem is that the button styles appear after the modal styles in the css order. Because the selectors have the same weight, some button rules override the button rules. Interestingly, this only happens in production mode. In development mode, the orders are inverted and the problem isn't visible. This is why you won't see the problem if you run yarn watch:doc:react but you will if you use build:doc:react (and open the resulting index.html file). It's possible to reproduce the problem in watch mode by extending the webpack configuration with mode: 'production',.

Based on that, I think the issue is that webpack changes the output order depending on whether we're in production or not. There are many existing issues about that, but I couldn't find one with a fix that works for us. I tried the latest versions of webpack and the MiniCssExtractPlugin, but it didn't work any better. Some issues are still open and I probably haven't seen all of them, so there's more to investigate there.

The exact line that imports the button css is the one importing the Button component in the Modal component. I built the styleguidist with only the Modal component in it, and moving this line has no effect at all. the only "fix" is to actually remove the import, but then of course a lot of other stuff breaks.
I'd like to have a better understanding of what exactly goes into the styleguidist build, and maybe that would help us figure out who exactly is loading files out of order. That might require diving into styleguidist's code.

Final observation, the problem happens when a webpack configuration compiled the css for cozy-ui. By using the transpiled version of cozy-ui, webpack doesn't get involved in the css order, and that css happens to be correct. So a workaround is to use the transpiled version of cozy-ui.

y-lohse commented 5 years ago

After using the transpiled version of cozy-ui, we discovered more css-order problems:

y-lohse commented 5 years ago

Following a git bisect by @gregorylegarec it appears the problem was introduced in https://github.com/cozy/cozy-ui/commit/98d96bb7dbf8f906203d3420968af46f16cb06db. This comment is a very good overview of what's happening and why it should not be a problem… but evidently there's something fishy going on.

ptbrowne commented 5 years ago

The problem is that the button styles appear after the modal styles in the css order. Because the selectors have the same weight, some button rules override the button rules.

Another lead to solve the problem is not to rely on the order of CSS and increase the selector value of the modal CSS. Could this be considered a solution ?

GoOz commented 5 years ago

For the Modal, yes. But it's not a global solution.

ptbrowne commented 5 years ago

For component styles, I strongly believe we should not rely on the order of rules. Order of component loading is not guaranteed. The loading of a component should not have side effects on another one. And we should not expect a component to be loaded before another one.

Do you have examples where order of CSS is really important ?

By order of the rules, I mean order of the rules inter files, no inside a file.

CPatchane commented 5 years ago

For component styles, I strongly believe we should not rely on the order of rules. Order of component loading is not guaranteed.

I don't completely agree, since order is important in CSS, we should take care of this order for better maintaining. Make an increased selector could be a fix but just a quick-fix for this specific use case, it's not guaranteed that the issue will never come back. Using more important selectors also make things very difficult to overwrite on the application side (we may need it even if we avoid), we have to find this selector and add a more increased selector to overwrite that. I know the case is rare but the application should have the possibility to overwrite a style of CozyUI if needed IMHO.

Could this issue be about importing many times cozy-ui styles or stylus files in many files? This usage of CozyUI is something that we don't want in the future, isn't it? I think transpiled components fixes this issue because we only import one css file at one moment inside one file. I don't have this issue anymore in the Cozy-Store since I don't use CozyUI stylus file directly, I just import the CozyUI build styles in my index file and use CSS variables for colors. Maybe it would be better to have this same approach in Contacts also?

y-lohse commented 5 years ago

I think transpiled components fixes this issue because we only import one css file at one moment inside one file.

Yes, the problem with the modal cross only appears when we build cozy-ui in production mode. In our apps, we want to use the transpiled version, in which case cozy-ui is already built and doesn't have the problem. The styleguidist still doesn't use the transpiled version (see #877) — but yes, ideally I'd rather focus on using the transpiled version of cozy-ui everywhere and leave this problem alone.

However there are also problems in the transpiled version. Not as critical, and less understood, but the problem might still be there.

I agree with @ptbrowne , we should not rely on the css order as we don't have much control over it. But just bumping selector specificity when we need it isn't a long term solution. Maybe we can be stricter on our BEM usage which should naturally give rules with proper specificity, or maybe there's a css module configuration that has gone wrong somewhere. I'd like to have a better understanding of the css order problem in the transpiled version before we choose a solution anyway.

ptbrowne commented 5 years ago

Maybe we can be stricter on our BEM usage which should naturally give rules with proper specificity

I think this is the key, the fact that the modal CSS overrides the CSS of a button is to me the culprit. Trying to find sources and advice on the object ("strategy css order css component" on google) I found this gist that give good guidelines IMHO.

In particular this paragraph speaks exactly about our subject.