Allow PostCSS Plugin “Px to Rem” in core for Olivero theme

+ 1

Generally speaking, I'm fine with this, but I'm curious if it's out of the realm of possibility to use 10px as the HTML element base font instead of 16px? In that case 10px = 1rem -- 15px = 1.5rem, etc... Becomes way less confusing that way, and you don't need any extra post processing. I would imagine, there's be a bit of work to go adjust font sizes elsewhere to adjust though.

+1

+1 this plugin was helpful during my contributions to Olivero in contrib, would be great to also have access to it in Core.

+1
Even if I'm used to writing rem. I think that could be a nice addition to ease the theming process.

+1

Could someone write initial patch for this to start discussing the configuration and see how it works with Claro? We have to make sure that we take into account that some properties defining hairline elements should continue using px values (for example borders). This has been documented here https://www.drupal.org/docs/8/modules/claro/claro-css-coding-standards#s....

+1

I think we should expand the configuration by adding media queries to the converted list. We also need to convert at least width, height, min-width, min-height, margin, padding, right, left, top, bottom, border-radius and box-shadow properties. Maybe it would be easier to use asterisk and then negate border and outline properties?

1. +++ b/core/themes/claro/css/base/print.css
   @@ -48,15 +48,15 @@
   +    border-width: 0.0625rem;
   

   This should be added to the negated properties list.

2. +++ b/core/themes/claro/css/components/accordion.css
   @@ -13,28 +13,28 @@
   +  border-radius: 0.125rem;
   

   I'm wondering if we should set minPixelValue to something like 2px or 3px to avoid other hairline elements from being converted

Looks much better! We should manually test Claro and add dependency evaluation for the new library.

+++ b/core/themes/claro/css/components/system-status-counter.css
@@ -45,7 +45,7 @@
+  background-size: 1.5625rem;

Still pondering if background-size and background should be excluded too 🤔

Since this is already how Claro is implemented, I would recommend moving the discussion about that to a follow-up. We could implement this patch based on the current Claro implementation without causing any changes, and make changes after we have agreed on how this should work in a follow-up. Any thoughts?

Fonts are an area that I always need to look up. Best advice I could see is:
https://www.24a11y.com/2019/pixels-vs-relative-units-in-css-why-its-stil...

Webaim is always useful:
https://webaim.org/techniques/fonts/#font_size

Then you've got opinions like this about ditching REM's entirely & going back to px:
https://mindtheshift.wordpress.com/2015/04/02/r-i-p-rem-viva-css-referen...

There is more digging to be done, particularly if we're looking at browser default sizes vs page zoom (CTRL++).

Re. #19:

My main thought is that when people change their browser's font-size, they're only trying to get the browser to display larger fonts, not zoom the entire page

That's a VERY unsafe guess. I see the point that you may not want some layout whitespace to grow, but you can't easily make that assumption on behalf of users.

There are definitely some non-font things which users will want to zoom. Here are some scenarios:

1. Borders and outlines which convey state (e.g. focus), or help to distinguish content.
   1. Link underlines are essential to distinguish content and identify behaviour. These are variously implemented as CSS text-decoration (which scales as text); OR using CSS borders, box-shadows, or something else (which won't necessarily scale as text).
   2. Borders in tables help users to track along the same row or column. If the text scales, but the separator lines don't, then they won't be effective for helping users with low vision.
2. Images (icons, etc) which convey information, or aid the understanding of information.
   1. Message type. If the text scales, but your warning-triangle doesn't, then it won't help someone with low vision understand that this is a warning.
   2. Styled faux-checkboxes, (or other inputs). If the text label scales, but a checkbox doesn't, then it won't help low-vision users to understand the state of the checkbox.
   3. An image which marks a required field (commonly an asterisk). A real text asterisk will scale as text, but a PNG of an asterisk won't scale as text. Low vision users need the required-field indicator to scale too.
3. Clickable targets. If text scales, but an icon-only button doesn't, then it won't help a user with motor impairments to hit the target reliably.
4. Line spacing is important for readability. The CSS line-height property scales as text, but other white space might not. For example, if you rely on CSS padding to space table rows apart. Margins between paragraphs and list-items should also scale.

I'd say if you find any of these things, which fail to grow in any of our supported browsers, then it should definitely be classed as a bug in Drupal. (This list isn't exhaustive.)

Note that some browsers offer more than one zooming method, and you should test them all:

• Font-size settings (global, persistent). These may come from the OS settings, or browser settings.
• Some browsers also have a minimum font-size setting. They partially respect author's size choices, but prevent too small text.
• Text zoom (ad-hoc, remembered per site)
• Full-page zoom. Firefox lets users decide whether to zoom text only, or text and images.
• Others?

User style sheets are worth thinking about too, but they aren't specifically about zooming. (They're also awkward to plan/test for in my view.) Be careful not to make life difficult with CSS !important, or over-use of class/attribute selectors. It's easier to write a user style sheet for HTML element names and well-known pseudo-classes, than have to anticipate a multitude of arbitrary class selectors. Drupal already suffers from this.

Re. #23:

I see you've found the 24a11y article - it's good, and has some excellent demonstrations. I was going to refer you there myself.

I think we should continue the great discussion on where to use REMs. I believe that we could add the plugin based on our previous assumption on how REMs should be used. This issue will move the responsibility to calculate REM values from a developer to build time and could make it easier to make this type of changes globally because we can apply changes based on the plugin configuration.

From the issue summary...

However, REM units are needed for accessibility reasons.

This isn't exactly true. You can use other relative units such as em or percentage (but not viewport-based units on their own).

I've changed "REM units" to "relative units" in the issue summary.

re-rolled.

rerolle patch For 9.0.x

Here's a reroll of #28.

We need review on the added dependency.

Since this is a dev dependency and we're already using similar plugins by the same author it's simple to sign-off on, so removing the Needs release manager review tag. Are there any plans to use this with claro too?

Looks fine to me; we already signed off on PostCSS and this is just one more tool in their box.

However, I need to have a kneejerk response to:

Security policies of the package: Not formally documented. This library is only used in by core developers in development environments.

Thousands of random dev dependencies installed on committers' laptops are right now my preferred best strategy for "If I wanted get control of dozens or hundreds of major projects, how would I do it? I'd put something malicious in a dev dependency, bury it in the dependency tree, and start collecting data. We should always take the step of suggesting the define a security policy, although their response does not need to be blocking if it's not a prod dep.

So if someone could post that suggestion, that'd be great. Recent example: https://github.com/focus-trap/tabbable/issues/136#issuecomment-726308502

Re #37 - the patch updates claro to use it :)

Did not meant to change status, although I would like to see us us ask PostCSS define a security policy (probably not in this particular queue).

I've added the follow-up #3187501: Define how and where to use REM units

I've done a drive through of claro and I've not noticed any regressions.

Committed 7527394 and pushed to 9.2.x. Thanks!
Committed 4073ca3 and pushed to 9.1.x. Thanks!

Crediting RM's for release manager review on the d.o issue.

Backported to 9.1.x as both Claro and Olivero are experimental.

CSPELL: checking all files
/Users/alex/dev/drupal/core/scripts/css/compile.js:10:45 - Unknown word (pxtorem)

diff --git a/core/scripts/css/compile.js b/core/scripts/css/compile.js
index f776c37d39..810e3ea38b 100644
--- a/core/scripts/css/compile.js
+++ b/core/scripts/css/compile.js
@@ -7,6 +7,7 @@ const postcssImport = require('postcss-import');
 const postcssHeader = require('postcss-header');
 const postcssUrl = require('postcss-url');
 const postcssPresetEnv = require('postcss-preset-env');
+// cspell:ignore pxtorem
 const postcssPixelsToRem = require('postcss-pxtorem');
 
 module.exports = (filePath, callback) => {

Fixed on commit.

There was still the followup I asked for. In the IS of the original issue #3060153: Use PostCSS in core, initially only for Claro they actually defines a private reporting process. So we need to stop ignoring security of dev dependencies and at least take the time to check and get it documented back into our dependency security contacts. (And encourage individual policies to get their security policies defined where they're not.)