Problem/Motivation

CSS Logical Properties enable one-line CSS declarations that work in both left-to-right and right-to-left languages.

Instead of writing 

.selector {
  padding-left: 10px;
}

[dir="rtl"] .selector {
  padding-right: 10px;
}

We will now write

.selector {
  padding-inline-start: 10px;
}

Logical properties are supported by all of Drupal 10's supported browsers, and now will ship them without transpiling (as of #3312481: Update core's browserlist).

Steps to reproduce

Proposed resolution

We should enforce the use of logical properties through automation. This will ensure that RTL styles are not neglected, and it will also result in a smaller CSS bundle (as opposed to writing our using the dir attribute).

Remaining tasks

Failing tests:
core/profiles/demo_umami/tests/src/FunctionalJavascript/OpenTelemetryFrontPagePerformanceTest.php
core/profiles/demo_umami/tests/src/FunctionalJavascript/AssetAggregationAcrossPagesTest.php

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#32 3312966-error.png523.06 KBnayana_mvr
#3 3312966-2.patch1.73 KBlongwave

Issue fork drupal-3312966

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mherchel created an issue. See original summary.

longwave made their first commit to this issue’s fork.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Active » Needs review
StatusFileSize
new 3312966-2.patch1.73 KB

There is an npm package that lets us enforce this in Stylelint: https://www.npmjs.com/package/stylelint-use-logical-spec

Running this against core gives 4263 errors and some of these are likely false positives, we might need to break this up into different CSS rules.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Needs work
mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented

Is there a way to only enforce it for CSS properties that are in the diff?

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented

Note that if/when we implement this, we need to allow certain non-logical rules including float and clear to accommodate #3313146: PostCSS Logical not transpiling flow relative properties (e.g. float: inline-start) which are not supported by Chrome and Safari

It looks like stylelint-use-logical-spec supports this (see https://github.com/Jordan-Hall/stylelint-use-logical-spec#except)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

I was looking at using the fixer but unfortunately we fall over on https://github.com/Jordan-Hall/stylelint-use-logical-spec/issues/28

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Related issues: +#3314921: Add CSS logical properties to properties-order within .stylelintrc.json, and do some cleanup of unneeded properties
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

The issue linked in #7 was closed by stylelint-use-logical-spec v5.0.0, so hopefully it's time to revisit this.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

alexpott opened merge request !7761

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Hiding patches.

xjm’s picture

xjm
she/her
Primary language English
commented

I postponed #2485497: Clean up locale CSS in line with our CSS standards on this issue.

Gauravvvv made their first commit to this issue’s fork.

Gauravvvv opened merge request !7903

quietone made their first commit to this issue’s fork.

quietone’s picture

quietone commented
Issue summary: View changes

@Gauravvvv, why did you make a second MR?

I rebased MR 7761. The I ran the automated fixes. There are now 2 failing tests.

core/profiles/demo_umami/tests/src/FunctionalJavascript/OpenTelemetryFrontPagePerformanceTest.php
core/profiles/demo_umami/tests/src/FunctionalJavascript/AssetAggregationAcrossPagesTest.php

nod_’s picture

nod_
Primary language French
Location Lille
commented

It's nice to have a rule and automated fix for this one.

MR needs quite a bit of work (only did 1/3 of the changed files). Essentially we need to make sure that any [dir="rtl"] rule is still needed and remove all the associated /* LTR */ comments in the CSS.

The manual cleanup is needed because if you have

Before auto-fix Fix needed
This code 
.myclass {
  padding-left:10px;/*LTR*/
}
[dir="rtl"] .myclass {
  padding-left:0;
  padding-right:10px;
}
is transformed into 
.myclass {
  padding-inline-start:10px;/*LTR*/
}
[dir="rtl"] .myclass {
  padding-inline:0 10px;
}

Here when in RTL, we add the equivalent of a padding-left, not the wanted padding-right!

And the proper fix is:

.myclass {
  padding-inline-start:10px;
}
nod_’s picture

nod_
Primary language French
Location Lille
commented

seems the config of the lint check is broken

nod_’s picture

nod_
Primary language French
Location Lille
commented

It's easier to revert the last commit, fix the config and rerun the linter

quietone’s picture

quietone commented

@nod_, thanks for the review!

While I don't know CSS very well I accepted the suggestions because they made sense to me. I then tried just a few to see if I truly understand. I look forward to feedback on those few changes.

Still more work to do here.

alexpott changed the visibility of the branch 3312966-enforce-css-logical-properties to hidden.

nod_’s picture

nod_
Primary language French
Location Lille
commented

Thanks, the fix you tried are accurate. One thing is that the last autofix changed all the values for float/border but they are supposed to be excluded from the config. So either we change the config to remove all exceptions, or we change the code to follow the exceptions (ie. revert the last autofix commit).

quietone’s picture

quietone commented

@nod_, I think the choice is yours to make. Which do you prefer?

nod_’s picture

nod_
Primary language French
Location Lille
commented

I pinged folks on slack to get some more views on this. I think I'd rather go all-in we used to have a problem with postcss and the float logical values, it might be fixed by now.

quadrexdev made their first commit to this issue’s fork.

quietone’s picture

quietone commented

Shall we break this into smaller child issues? What is a good way to do that for this type of change?

nod_’s picture

nod_
Primary language French
Location Lille
commented

breaking it down would be first automatic transform of all the css files that do not have a [dir=] rule, since automated replacement is safe.

Then it's a little bit of case by case. Might need to see what's left to know how to split things up.

nayana_mvr made their first commit to this issue’s fork.
Some .css file changes were missing. Added them.

nayana_mvr’s picture

nayana_mvr commented

I think the comment 

/**
     * Chromium and Webkit do not yet support flow relative logical properties,
     * such as float: inline-end. However, PostCSS Logical does not compile this
     * value, so we accommodate by not using these.
     *
     * @see https://caniuse.com/mdn-css_properties_clear_flow_relative_values
     * @see https://github.com/csstools/postcss-plugins/issues/632
     */

added as part of the ticket https://www.drupal.org/project/drupal/issues/3313146 is no longer required since the issue (https://github.com/csstools/postcss-plugins/issues/632) mentioned in that comment is already closed.

quietone’s picture

quietone commented

@nayana_mvr, Can you explain how you found the 'missing ,css files' ?

nayana_mvr’s picture

nayana_mvr commented
StatusFileSize
new 3312966-error.png523.06 KB

There was a pipeline error as shown below,

error

It says .pcss.css file from line no. 273 is not updated. This happens when .css (compiled) file corresponding to .pcss.css file is not updated with the changes. I have faced similar issue before. That’s why I tried the command yarn run build:css in core folder and found that the .css files corresponding to the .pcss.css files shown in the error are not pushed to the branch.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.