Closed Bug 1659584 Opened 4 years ago Closed 2 years ago

[meta] Switch from CSS includes to CSS imports or other loading mechanisms

Categories

(Toolkit :: Themes, task)

task

Tracking

()

RESOLVED FIXED
Tracking Status
firefox100 --- fixed

People

(Reporter: Gijs, Unassigned)

References

Details

(Keywords: meta, Whiteboard: fidefe-2022-mr1-css-linting)

See bug 1659444 for context.

Current list of uses: https://searchfox.org/mozilla-central/search?q=%25inclu&path=css&case=false&regexp=false

There are a number of solutions here:

  1. use @import instead
  2. concatenate files using a build step instead of %include (e.g. for the relatively long lists of included shared browser files included in all of the browser.css instances)
  3. actually just combine files where appropriate
  4. switch to using a single shared sheet (and maybe CSS variables to account for per-OS differences) for cases where we currently have OS-specific sheets that all include a single shared sheet with only some (or even most) of the rules.

This too is a metabug as the problem probably warrants splitting up a bit.

Keywords: meta

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Component: General → CSS Parsing and Computation
Product: Firefox → Core

This is really also not the perfect component but at least bugbug will stop interfering.

Component: CSS Parsing and Computation → Themes
Product: Core → Toolkit

I'm looking at how to break this down into actionable bugs. Basically, if we imagine a tree of CSS files which import/include other CSS files, we need roughly one bug for each non-empty node in that tree (though some we may be able to group together.)

Quite a number of these %includes are from a platform-specific "parent" stylesheet, which maybe defines some var()s and some overrides, then includes all the shared rules. There's 2 obvious ways to handle this case:

  1. We can inline the included stylesheet and move those platform-specifics into @media blocks - resulting in a single shared stylesheet for all platforms. This will contain some rules in there that will never be applicable for other platforms. Maybe that's ok. Maybe we treat that as an optimization problem at build time - culling out @media blocks which will never match (if we measure a problem that warrants this.)
  2. We leave the structure as-is, and @import the shared stylesheet instead of %include-ing it. For the most part that should work ok. There may be cases where going from %include e.g. at the bottom of the stylesheet, to @import at the top may change the cascade, so some case will be needed.

So, I think that might be a case by case decision, and it makes sense to have a bug for each.

Where we have nested includes - notably in platform-specific browser.css, browser.inc.css, and toolkit global.css - I suggest we work our way up the include/import tree in the same manner as above. Does that sounds reasonable? Its a lot of bugs to file so I would appreciate a sanity-check before I start.

Flags: needinfo?(gijskruitbosch+bugs)

(In reply to Sam Foster [:sfoster] (he/him) from comment #3)

I'm looking at how to break this down into actionable bugs. Basically, if we imagine a tree of CSS files which import/include other CSS files, we need roughly one bug for each non-empty node in that tree (though some we may be able to group together.)

Quite a number of these %includes are from a platform-specific "parent" stylesheet, which maybe defines some var()s and some overrides, then includes all the shared rules. There's 2 obvious ways to handle this case:

  1. We can inline the included stylesheet and move those platform-specifics into @media blocks - resulting in a single shared stylesheet for all platforms. This will contain some rules in there that will never be applicable for other platforms. Maybe that's ok. Maybe we treat that as an optimization problem at build time - culling out @media blocks which will never match (if we measure a problem that warrants this.)
  2. We leave the structure as-is, and @import the shared stylesheet instead of %include-ing it. For the most part that should work ok. There may be cases where going from %include e.g. at the bottom of the stylesheet, to @import at the top may change the cascade, so some case will be needed.

So, I think that might be a case by case decision, and it makes sense to have a bug for each.

Where we have nested includes - notably in platform-specific browser.css, browser.inc.css, and toolkit global.css - I suggest we work our way up the include/import tree in the same manner as above. Does that sounds reasonable?

Yes. For browser.css especially, I also wonder if some stuff could be imported dynamically to reduce potential performance impact. We shouldn't need UITour sheets until UITour actually starts. Ditto the translation infobar and the "edit bookmark" panel, the sidebar, etc. etc. We'd need to be cautious about the impact of images linked from such sheets not being preloaded, however.

Note that if we want to have an import for the shared sheet, in an ideal world we change packaging such that we can use a relative import that can be resolved both in the source tree and the built URL set. This would mean some changes to packaging (so that, e.g. for downloads, the shared sheet ends up in a subdirectory like chrome://browser/skin/shared/downloads.css and the non-shared sheet can import ../shared/downloads.css or something), but should be doable in a bunch of cases. That would help with the linting etc. so that tools can resolve the URLs (which they cannot for chrome://skin/blah).

Its a lot of bugs to file so I would appreciate a sanity-check before I start.

I'd start with the non-browser.css, non-global.css trees to get some practice, so it's less daunting and complex, and there's immediate results (ie more files that we can lint, whereas solving something deep inside the tree for browser.css won't improve things as quickly if the resulting files still have other includes).

Flags: needinfo?(gijskruitbosch+bugs)
Depends on: 1753757
Depends on: 1753759
Depends on: 1753760
Depends on: 1753761
Depends on: 1753762
Depends on: 1753763
Depends on: 1753764
Depends on: 1753765
Depends on: 1753767
Depends on: 1754262
Depends on: 1754263
Depends on: 1754265
Depends on: 1754266
Depends on: 1754268
Depends on: 1754269
Depends on: 1754270
Depends on: 1754271
Depends on: 1580487

(In reply to Sam Foster [:sfoster] (he/him) from comment #3)

  1. We can inline the included stylesheet and move those platform-specifics into @media blocks - resulting in a single shared stylesheet for all platforms. This will contain some rules in there that will never be applicable for other platforms. Maybe that's ok. Maybe we treat that as an optimization problem at build time - culling out @media blocks which will never match (if we measure a problem that warrants this.)

To this end, I imagined replacing some of the per-platform (windows, osx, linux) stylesheets with a single CSS file with those platform-specific rules moved inside media-queries, something like:

/* the contents of browser/themes/shared/feature.inc.css */

@media (-moz-platform: windows) {
  /* the contents of browser/themes/windows/feature.css */
}
@media (-moz-platform: osx) {
  /* the contents of browser/themes/osx/feature.css */
}
@media (-moz-platform: linux {
  /* the contents of browser/themes/osx/feature.css */
}

But while we have various -moz prefixed conditions, I don't see a simple equivalent for the mac/osx, windows, linux split we do today. Does it exist :emilio?

Flags: needinfo?(emilio)

There's none, but it should be pretty easy to add. Do you need me to do that? If so feel free to file a bug and happy to :)

Flags: needinfo?(emilio)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #6)

There's none, but it should be pretty easy to add. Do you need me to do that? If so feel free to file a bug and happy to :)

I filed Bug 1754547 - Provide a @media query to target major platform/toolkits.

Depends on: 1754547
Whiteboard: fidefe-2022-mr1-css-linting
No longer depends on: 1659576

All the dependencies here are closed and there are no more %includes in the CSS in moz-central, so I'm closing this meta.
Work to enforce the continued switch from includes to @imports will be linked to the umbrella bug 1659444.

Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.