1659576 - Package shared CSS files into a "shared/" subfolder of the skin chrome package, instead of into the root of the skin chrome package

Status: NEW

(Firefox :: Theme, task)

Description

[:Gijs (he/him)]

To make it easier to refer to these using relative URLs, let's package all the shared files into a "shared" subfolder.

Comment 1

[Bernard Igiri]

List of shared CSS files.

browser/components/contextualidentity/content/usercontext.css
browser/themes/shared/UITour.inc.css
browser/themes/shared/addon-notification.inc.css
browser/themes/shared/addons/extension-controlled.inc.css
browser/themes/shared/autocomplete.inc.css
browser/themes/shared/browser-custom-colors.inc.css
browser/themes/shared/browser.inc.css
browser/themes/shared/compacttheme.inc.css
browser/themes/shared/contextmenu.inc.css
browser/themes/shared/controlcenter/panel.inc.css
browser/themes/shared/ctrlTab.inc.css
browser/themes/shared/customizableui/customizeMode.inc.css
browser/themes/shared/customizableui/panelUI.inc.css
browser/themes/shared/downloads/allDownloadsView.inc.css
browser/themes/shared/downloads/download-blockedStates.inc.css
browser/themes/shared/downloads/downloads.inc.css
browser/themes/shared/downloads/indicator.inc.css
browser/themes/shared/downloads/progressmeter.inc.css
browser/themes/shared/fullscreen/warning.inc.css
browser/themes/shared/identity-block/identity-block.inc.css
browser/themes/shared/menupanel.inc.css
browser/themes/shared/notification-icons.inc.css
browser/themes/shared/places/editBookmarkPanel.inc.css
browser/themes/shared/places/places-tooltip.inc.css
browser/themes/shared/places/sidebar.inc.css
browser/themes/shared/preferences/dialog.inc.css
browser/themes/shared/preferences/preferences.inc.css
browser/themes/shared/searchbar.inc.css
browser/themes/shared/sidebar.inc.css
browser/themes/shared/syncedtabs/sidebar.inc.css
browser/themes/shared/tabs.inc.css
browser/themes/shared/toolbarbutton-icons.inc.css
browser/themes/shared/toolbarbuttons.inc.css
browser/themes/shared/translation/infobar.inc.css
browser/themes/shared/urlbar-searchbar.inc.css
browser/themes/shared/urlbar/dynamicResults.inc.css
browser/themes/shared/urlbarView.inc.css
browser/themes/shared/webRTC-indicator.inc.css
browser/themes/windows/browser-aero.css
toolkit/themes/shared/alert.inc.css
toolkit/themes/shared/checkbox.inc.css
toolkit/themes/shared/commonDialog.inc.css
toolkit/themes/shared/global.inc.css
toolkit/themes/shared/in-content/common.inc.css
toolkit/themes/shared/menu-scrolling.inc.css
toolkit/themes/shared/menulist.inc.css
toolkit/themes/shared/notification-popup.inc.css
toolkit/themes/shared/popupnotification.inc.css
toolkit/themes/shared/tree.inc.css

Comment 2

[Dão Gottwald [:dao]]

I think we should avoid having a shared/ folder, and come up with a file naming scheme instead; see my comment here: https://phabricator.services.mozilla.com/D137924?id=537486#inline-760912

Comment 3

[Sam Foster [:sfoster] (he/him)]

Here's the problem we're trying to solve with this bug. We want paths to resolve both within the browser, and on the filesystem. That's a requirement to linting, static analysis etc. working - which is the whole point of this exercise. For example, with downloads.css, we have a shared stylesheet currently called downloads.inc.css, which needs to be @import-ed into both downloads.css and allDownloadsView.css. The directory structure currently looks like this:

theme/
  - windows/
    - downloads/
     - allDownloadsView.css
     - downloads.css
  - shared/
    - downloads/
      - downloads.inc.css

So, you would expect the path to that shared file to be @import url('../../shared/downloads/downloads.inc.css').
But, through some manifest mapping magic, we actually "serve" downloads.css as chrome://browser/skin/downloads/downloads.css so that doesnt work - we would end up with chrome://browser/shared/downloads/downloads.inc.css

I get that we don't want windows / osx / linux in our URLs. We would need to add logic everywhere we link a stylesheet to figure out which platform we're running on. Maybe: chrome://browser/skin/platform/downloads/downloads.css, which meets this goal and adds that extra step allowing @import url('../../shared/downloads/downloads.inc.ss') to resolve correctly in both case?

Comment 4

[Dão Gottwald [:dao]]

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

I get that we don't want windows / osx / linux in our URLs. We would need to add logic everywhere we link a stylesheet to figure out which platform we're running on. Maybe: chrome://browser/skin/platform/downloads/downloads.css, which meets this goal and adds that extra step allowing @import url('../../shared/downloads/downloads.inc.ss') to resolve correctly in both case?

I think that's pretty ugly ;) and, given how convoluted the chrome:// scheme already is, I would rather not do that. Also, the .inc.css files aren't the most interesting cases, I don't think, as most of these could probably go down the @media (-moz-platform: ...) path? What's your envisioned solution for shared resources that aren't %included but get pulled directly into the theme directories at build time?

If I understand correctly, we would also need to generally ban @import url("chrome://..., so only ever have relative import paths? This seems increasingly problematic, and while I don't doubt that linting without building has some value, I'd like to see a proper cost / benefit analysis at that point. Perhaps the solution is to extend or hack the linter, making it somehow understand / rewrite our imports?

Comment 5

[Sam Foster [:sfoster] (he/him)]

(In reply to Dão Gottwald [::dao] from comment #4)

If I understand correctly, we would also need to generally ban @import url("chrome://..., so only ever have relative import paths? This seems increasingly problematic, and while I don't doubt that linting without building has some value, I'd like to see a proper cost / benefit analysis at that point. Perhaps the solution is to extend or hack the linter, making it somehow understand / rewrite our imports?

Those are all good points. I think for the purposes of resolving the bugs under bug 1659584, we should go ahead and use fully qualified chrome:// URIs for the @import urls, so we don't block progress there. That also means we might want to hold off on creating patches for this bug until most of that work is done to avoid conflicts and a moving target.

That also gives us some time to look into what it would take to add a CSS linter and what our best options are to support that case. For a basic lint pass over a changed CSS file, it doesnt need to navigate the @imports at all. But I think implicit in this project is ultimately enabling the kind of static analysis and insights that are only unlocked by a more complete understanding of the stylesheets in play and impacted by a given change. E.g. IDE suggestions, a report of rules orphaned by a change, etc.

Comment 6

[Sam Foster [:sfoster] (he/him)]

I'm removing this bug as blocking bug 1659584, as it doesnt prevent us from @import-ing rather than %include-ing CSS. My understanding is that we want to leave this open for consideration as we better understand any future requirements that might emerge for example from tooling for static analysis of our CSS

We've adopted a pattern of generally removing platform-specific CSS files and instead including those rules in @media queries in a single shared file. I.e. most of our stylesheets are now shared. Where that would result in a name conflict with a platform-specific resource, we've added the -shared.css suffix.

In almost all cases, when one stylesheet imports another, we use a fully-qualified chrome:// URI. That means its currently necessary to have processed the manifests in order to be able to map a given stylesheet's URI back to a file in the source tree.