_variables.scss is imported multiple times (5 times!)

[jelly]

Explain what happens

1. Browser to https://127.0.0.1:9090/users
2. Open developer tools
3. Click inspect element and select the body of the users page
4. Look at the styles imported and notice that :root { } is imported multiple times from _variables.scss

What seems to happen is:

pkg/lib/users.scss imports @import "global-variables.scss"; which imports @import "@patternfly/patternfly/base/patternfly-variables.scss"; to override the breakpoints.

variables.scss is imported multiple times, probably the cause of our issue.

[jelle@t14s][~/projects/cockpit/dark-theme]%git grep variables.scss
pkg/Makefile.am:        pkg/lib/patternfly/patternfly-overrides-variables.scss \
pkg/lib/_global-variables.scss:@import "@patternfly/patternfly/base/patternfly-variables.scss";
pkg/lib/cockpit-components-table.scss:@use "@patternfly/patternfly/base/patternfly-variables.scss";
pkg/lib/ct-card.scss:@use "_global-variables.scss" as *;
pkg/systemd/logs.scss:@import "global-variables.scss";
pkg/systemd/services/service-details.scss:@use "_global-variables.scss";
pkg/users/users.scss:@import "global-variables.scss";
pkg/users/users.scss:// @import "@patternfly/patternfly/sass-utilities/scss-variables.scss";
webpack.config.js:                // [Redefine grid breakpoints] section in pkg/lib/_global-variables.scss for more details

[jelle@t14s][~/projects/cockpit/dark-theme]%git grep patternfly-variables
pkg/lib/_global-variables.scss:@import "@patternfly/patternfly/base/patternfly-variables.scss";
pkg/lib/cockpit-components-table.scss:@use "@patternfly/patternfly/base/patternfly-variables.scss";

Upstream issue: https://github.com/patternfly/patternfly/issues/4511

We can't use @use to resolve this issue https://github.com/patternfly/patternfly/issues/4519

This most likely blocks https://github.com/cockpit-project/cockpit/pull/17319

[garrett]

Breakpoints are separate from this problem. This problem is with :root {} PF varibles being imported several times.

node_modules/@patternfly/patternfly/base/patternfly-variables.scss contains:

@import "../sass-utilities/all";
@import "variables";

We actually want the SCSS variables when importing patternfly-variables.scss

• global-variables
  • patternfly-variables
  • sass-utilities/all
    • div
    • functions
    • colors
    • scss-variables
    • mixins
    • placeholders
  • variables

I think where we call global-variables, we really want the @patternfly/patternfly/sass-utilities/scss-variables.scss

The main problem here is that we're importing things that are importing CSS where they're supposed to import SASS variables only.

[jelly]

We can't import scss-variables.scss as it depends on $pf-color-white

Module build failed (from ./node_modules/mini-css-extract-plugin/dist/loader.js):
ModuleBuildError: Module build failed (from ./node_modules/sass-loader/dist/cjs.js):
SassError: Undefined variable.
   ╷
26 │ $pf-global--BackgroundColor--100: $pf-color-white !default;
   │                                   ^^^^^^^^^^^^^^^
   ╵
  node_modules/@patternfly/patternfly/sass-utilities/scss-variables.scss 26:35  @import
  pkg/lib/_global-variables.scss 1:9                                            @use
  pkg/lib/ct-card.scss 1:1                                                      @use
  pkg/users/users.scss 1:1                                                      root stylesheet
    at processResult (/home/jelle/projects/cockpit/dark-theme/node_modules/webpack/lib/NormalModule.js:758:19)
    at /home/jelle/projects/cockpit/dark-theme/node_modules/webpack/lib/NormalModule.js:860:5
    at /home/jelle/projects/cockpit/dark-theme/node_modules/loader-runner/lib/LoaderRunner.js:400:11
    at /home/jelle/projects/cockpit/dark-theme/node_modules/loader-runner/lib/LoaderRunner.js:252:18
    at context.callback (/home/jelle/projects/cockpit/dark-theme/node_modules/loader-runner/lib/LoaderRunner.js:124:13)
    at Object.loader (/home/jelle/projects/cockpit/dark-theme/node_modules/sass-loader/dist/index.js:69:5)
    at runNextTicks (node:internal/process/task_queues:60:5)
    at process.processImmediate (node:internal/timers:442:9)

2 ERRORS in child compilations (Use 'stats.children: true' resp. '--stats-children' for more details)
webpack 5.72.0 compiled with 4 errors in 13093 ms

[jelly]

pkg/users/authorized-keys-panel.jsx should be ported to PF4 JSX components instead of using PF html, that would also help out a bit.

[jelly]

So the easiest package to look at is probably networkmanager, the firewall.css file has a duplicate :root {} which is almost the same except:

patternfly-fonts-fake-path comes from pkg/lib/patternfly/patternfly-4-cockpit.scss ./assets/fonts comes from @patternfly/patternfly/base/patternfly-variables.css

[jelle@t14s][~/projects/cockpit/pf-css-removal]%git grep patternfly-4-cockpit.scss pkg/networkmanager
pkg/networkmanager/app.jsx:import '../lib/patternfly/patternfly-4-cockpit.scss';
pkg/networkmanager/firewall.jsx:import '../lib/patternfly/patternfly-4-cockpit.scss';

So app.jsx is for the network page and firewall.jsx for the firewall page these also have a separate html file so that should be fine.

So the question is, does this create one :root {}

/* Set fake font and icon path variables */
$pf-global--font-path: "patternfly-fonts-fake-path";
$pf-global--fonticon-path: "patternfly-icons-fake-path";
$pf-global--disable-fontawesome: true !default; // Disable Font Awesome 5 Free

@import "@patternfly/patternfly/patternfly-base.scss";

So I tried to remove the scss import in firewall.jsx but alas we still get two :root {} definitions. I did managed to get rid of the duplicate :root {} by removing cockpit-componets-table.jsx which imports @patternfly/patternfly/base/patternfly-variables.css.

diff --git a/pkg/networkmanager/firewall.jsx b/pkg/networkmanager/firewall.jsx
index bd9042679..052c72185 100644
--- a/pkg/networkmanager/firewall.jsx
+++ b/pkg/networkmanager/firewall.jsx
@@ -38,7 +38,7 @@ import {
 import { ExclamationCircleIcon } from '@patternfly/react-icons';

 import firewall from "./firewall-client.js";
-import { ListingTable } from 'cockpit-components-table.jsx';
+// import { ListingTable } from 'cockpit-components-table.jsx';
 import { ModalError } from "cockpit-components-inline-notification.jsx";
 import { EmptyStatePanel } from "cockpit-components-empty-state.jsx";
 import { FirewallSwitch } from "./firewall-switch.jsx";
@@ -46,8 +46,6 @@ import { FirewallSwitch } from "./firewall-switch.jsx";
 import { superuser } from "superuser";
 import { WithDialogs, DialogsContext } from "dialogs.jsx";

-import "./networking.scss";
-
 const _ = cockpit.gettext;

 superuser.reload_page_on_change();
@@ -203,38 +201,6 @@ function ZoneSection(props) {
             </CardTitle>
             { !firewall.readonly && <CardActions className="zone-section-buttons">{addServiceAction}{deleteButton}</CardActions> }
         </CardHeader>
-        {props.zone.services.length > 0 &&
-        <CardBody className="contains-list">
-            <ListingTable columns={[{ title: _("Service"), props: { width: 40 } }, { title: _("TCP"), props: { width: 30 } }, { title: _("UDP"), props: { width: 30 } }, { title: "", props: { width: 10 } }]}
-                          id={props.zone.id}
-                          aria-label={props.zone.id}
-                          variant="compact"
-                          emptyCaption={_("There are no active services in this zone")}
-                          rows={
-                              props.zone.services.map(s => {
-                                  if (s in firewall.services) {
-                                      return serviceRow({
-                                          key: firewall.services[s].id,
-                                          service: firewall.services[s],
-                                          onRemoveService: service => props.onRemoveService(props.zone.id, service),
-                                          onEditService: service => props.onEditService(props.zone, firewall.services[service]),
-                                          readonly: firewall.readonly,
-                                      });
-                                  } else {
-                                      return null;
-                                  }
-                              }).concat(
-                                  props.zone.ports.length > 0
-                                      ? portRow({
-                                          key: props.zone.id + "-ports",
-                                          zone: props.zone,
-                                          readonly: firewall.readonly
-                                      })
-                                      : [])
-                                      .filter(Boolean)}
-
-            />
-        </CardBody>}
     </Card>;
 }

Note that this is no issue in apps, as it has no cockpit-components-table.jsx import.

So I think the PR of @garrett should fix this issue as it https://github.com/cockpit-project/cockpit/issues/17556#issuecomment-1184518556

I did not recompile all of our packages but a bunch to see how bad it is:

grep -cr "\-\-pf-global--font-path" dist/*/*.css | grep -v base1

dist/apps/apps.css:1
dist/kdump/kdump.css:1
dist/kdump/test-config-client.css:0
dist/networkmanager/firewall.css:1
dist/networkmanager/network.css:2
dist/networkmanager/test-utils.css:0
dist/packagekit/updates.css:2
dist/selinux/selinux.css:2
dist/sosreport/sosreport.css:2
dist/static/login.css:0
dist/storaged/storage.css:2
dist/storaged/test-util.css:0
dist/systemd/hwinfo.css:2
dist/systemd/logs.css:2
dist/systemd/overview.css:1
dist/systemd/services.css:2
dist/systemd/terminal.css:1
dist/tuned/performance.css:0

[jelly]

Checking our pf-global-font-path includes again with our latest main, I get:

[jelle@t14s][~/projects/cockpit/main]%zgrep -c "\-\-pf-global--font-path" dist/*/*.css.gz

dist/apps/apps.css.gz:1
dist/kdump/kdump.css.gz:1
dist/metrics/index.css.gz:1
dist/networkmanager/firewall.css.gz:1
dist/networkmanager/network.css.gz:1
dist/packagekit/updates.css.gz:1
dist/playground/exception.css.gz:1
dist/playground/index.css.gz:1
dist/playground/journal.css.gz:1
dist/playground/metrics.css.gz:1
dist/playground/pkgs.css.gz:1
dist/playground/plot.css.gz:0
dist/playground/react-patterns.css.gz:1
dist/playground/service.css.gz:1
dist/playground/speed.css.gz:1
dist/playground/test.css.gz:1
dist/playground/translate.css.gz:1
dist/selinux/selinux.css.gz:1
dist/shell/index.css.gz:1
dist/sosreport/sosreport.css.gz:1
dist/storaged/storage.css.gz:1
dist/systemd/hwinfo.css.gz:1
dist/systemd/logs.css.gz:1
dist/systemd/overview.css.gz:1
dist/systemd/services.css.gz:1
dist/systemd/terminal.css.gz:1
dist/tuned/performance.css.gz:0
dist/users/users.css.gz:1

So I wonder if this issue is actually resolved?

[jelly]

Looking at our css with css-purge

[jelle@t14s][~/projects/cockpit/webpack-profile]%npx css-purge -v -i dist/selinux/selinux.css -o selinux.css
Input - CSS File : dist/selinux/selinux.css
Process - CSS
Process - Rules - Remove Comment
Process - Rules - Group Duplicate Rule : .pf-c-divider
Process - Rules - Group Duplicate Rule : .pf-c-divider::after
Process - Rules - Group Duplicate Rule : .pf-c-alert
Process - Rules - Group Duplicate Rule : .pf-c-alert__title
Process - Rules - Group Duplicate Rule : .pf-l-flex
Process - Rules - Group Duplicate Rule : .pf-c-page__sidebar.pf-m-light
Process - Rules - Group Duplicate Rule : .pf-c-page__sidebar.pf-m-light
Process - Declaration - Group Duplicate Declarations : .pf-c-page__sidebar.pf-m-light - --pf-global--Color--100
Process - Declaration - Group Duplicate Declarations : .pf-c-page__sidebar.pf-m-light - --pf-global--Color--200
Process - Declaration - Group Duplicate Declarations : .pf-c-page__sidebar.pf-m-light - --pf-global--BorderColor--100
Process - Declaration - Group Duplicate Declarations : .pf-c-page__sidebar.pf-m-light - --pf-global--primary-color--100
Process - Declaration - Group Duplicate Declarations : .pf-c-page__sidebar.pf-m-light - --pf-global--link--Color
Process - Declaration - Group Duplicate Declarations : .pf-c-page__sidebar.pf-m-light - --pf-global--link--Color--hover
Process - Declaration - Group Duplicate Declarations : .pf-c-page__sidebar.pf-m-light - --pf-global--BackgroundColor--100
Process - Rules - Group Duplicate Rule : .pf-c-page__header-tools-group, .pf-c-page__header-tools-item

Note, that's just a snippet from the output.

[jelle@t14s][~/projects/cockpit/webpack-profile]%grep .pf-c-page__sidebar.pf-m-light dist/selinux/selinux.css
.pf-c-page__sidebar.pf-m-light {
.pf-c-page__sidebar.pf-m-light, .pf-t-light {
.pf-c-page__sidebar.pf-m-light {
.pf-c-page__sidebar.pf-m-light {

The first and last declarations look the same to me.

14332   │ :where(.pf-theme-dark) .pf-c-drawer.pf-m-inline, :where(.pf-theme-dark) .pf-c-drawer.pf-m-static {
14333   │   --pf-c-drawer__panel--BackgroundColor: var(--pf-global--BackgroundColor--100);
14334   │ }
14335   │ .pf-c-page__sidebar.pf-m-light {
14336   │   --pf-global--Color--100: var(--pf-global--Color--dark-100);
14337   │   --pf-global--Color--200: var(--pf-global--Color--dark-200);
14338   │   --pf-global--BorderColor--100: var(--pf-global--BorderColor--dark-100);
14339   │   --pf-global--primary-color--100: var(--pf-global--primary-color--dark-100);
14340   │   --pf-global--link--Color: var(--pf-global--link--Color--dark);
14341   │   --pf-global--link--Color--hover: var(--pf-global--link--Color--dark--hover);
14342   │   --pf-global--BackgroundColor--100: var(--pf-global--BackgroundColor--light-100);
14343   │ }

14499   │ .pf-c-page__sidebar.pf-m-light, .pf-t-light {
14500   │   --pf-global--Color--100: var(--pf-global--Color--dark-100);
14501   │   --pf-global--Color--200: var(--pf-global--Color--dark-200);
14502   │   --pf-global--BorderColor--100: var(--pf-global--BorderColor--dark-100);
14503   │   --pf-global--primary-color--100: var(--pf-global--primary-color--dark-100);
14504   │   --pf-global--link--Color: var(--pf-global--link--Color--dark);
14505   │   --pf-global--link--Color--hover: var(--pf-global--link--Color--dark--hover);
14506   │   --pf-global--BackgroundColor--100: var(--pf-global--BackgroundColor--light-100);
14507   │ }

14808   │ .pf-c-search-input__menu.pf-c-panel {
14809   │   position: absolute;
14810   │   width: 100%;
14811   │ }
14812   │
14813   │ .pf-c-page__sidebar.pf-m-light {
14814   │   --pf-global--Color--100: var(--pf-global--Color--dark-100);
14815   │   --pf-global--Color--200: var(--pf-global--Color--dark-200);
14816   │   --pf-global--BorderColor--100: var(--pf-global--BorderColor--dark-100);
14817   │   --pf-global--primary-color--100: var(--pf-global--primary-color--dark-100);
14818   │   --pf-global--link--Color: var(--pf-global--link--Color--dark);
14819   │   --pf-global--link--Color--hover: var(--pf-global--link--Color--dark--hover);
14820   │   --pf-global--BackgroundColor--100: var(--pf-global--BackgroundColor--light-100);
14821   │ }

15251   │ .pf-c-page__sidebar.pf-m-light {
15252   │   color: var(--pf-global--Color--100);
15253   │   --pf-c-page__sidebar--BackgroundColor: var(--pf-c-page__sidebar--m-light--BackgroundColor);
15254   │   border-right: var(--pf-c-page__sidebar--m-light--BorderRightWidth) solid var(--pf-c-page__sidebar--m-light--BorderRightColor);
15255   │ }

The SELinux page has a minimal set of imports, one of them is @use "page" which imports page.scss which imports @import "@patternfly/patternfly/components/Page/page.scss";

@patternfly/react-styles/css/components/Page/page.css:.pf-c-page__sidebar.pf-m-light {
@patternfly/react-styles/css/components/Page/page.css:.pf-c-page__sidebar.pf-m-light {

So that explains the two last imports. Would the other import come from using the <Page> component?

[garrett]

The SELinux page has a minimal set of imports, one of them is @use "page" which imports page.scss which imports @import "@patternfly/patternfly/components/Page/page.scss";

Probably due to rewriting breakpoint variables in SASS?

Can WebPack let us just rewrite the px values for @media.*-width: .*px after compilation? It'd be less hacky than trying to work around it in CSS, really, as we import both SCSS and (already compiled) CSS, and not everything is doing the right thing right now.

[jelly]

The SELinux page has a minimal set of imports, one of them is @use "page" which imports page.scss which imports @import "@patternfly/patternfly/components/Page/page.scss";

Probably due to rewriting breakpoint variables in SASS?

For that we have global-variables right?

Can WebPack let us just rewrite the px values for @media.*-width: .*px after compilation? It'd be less hacky than trying to work around it in CSS, really, as we import both SCSS and (already compiled) CSS, and not everything is doing the right thing right now.

[garrett]

For that we have global-variables right?

Exactly. It's only used when it's included (not used) and then a file comes after it.

This is why there's so much breakpoint mess. We do not have a way to alter breakpoints on files that are included via React only nor do we for any statically included CSS. Or even SCSS that's included without the import.

Ideally, we'd inject the changed variables in PatternFly itself and rebuild PatternFly with it. Otherwise, the next best thing would be to just build everything and adjust the specific breakpoint variables in the resulting CSS afterward. But we're doing neither of these things.

(And we do absolutely need to adjust the breakpoints, as PatternFly is incorrectly setting breakpoints when you have iframes with navigation in the mix, which is what we do. They're assuming breakpoints are for whole documents, not partials in iframes like what we have.)

[garrett]

Both @jelly and I worked on making this less of a problem.

There's still an issue.

The best approach would be to redo the way we do breakpoints, then this should hopefully not be an issue afterward. Related: https://github.com/cockpit-project/cockpit/issues/17629