WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10k stars 4.02k forks source link

Add stylelint rule to prevent usage of the `*-reverse` CSS flex direction values #63048

Open afercia opened 6 days ago

afercia commented 6 days ago

Description

SimilarlySimilarly to https://github.com/WordPress/gutenberg/issues/61241 / https://github.com/WordPress/gutenberg/pull/61243 the CSS flex row-reverse and column-reverse values for flex-direction introduce a mismatch between visual/reading and DOM order.

For more details about why this is a problem for accessibility and the very limited cases where these values can be used, please see https://github.com/WordPress/gutenberg/issues/61241.

To prevent future usages of these values I'd like to porpose to add a new stylelint rule.

Currently, there are 5 occurrences of -reverse; in the *.scss files in the codebase:

packages/block-editor/src/hooks/block-hooks.scss:
  7:        flex-direction: row-reverse;

packages/block-editor/src/hooks/use-editor-wrapper-styles.native.scss:
  8:    flex-direction: column-reverse;

packages/block-library/src/form-input/style.scss:
  27:       flex-direction: row-reverse;

packages/block-library/src/media-text/style.native.scss:
  10:   flex-direction: row-reverse;
  17:       flex-direction: column-reverse;

All these occurrences should be double checked and if they do alter visual/DOM order, the implementation should be changed.

Step-by-step reproduction instructions

Screenshots, screen recording, code snippet

No response

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

afercia commented 6 days ago

I was expecting this to be as simple as adding the property / value to the existing declaration-property-value-disallowed-list rule this way:

"declaration-property-value-disallowed-list": [
    {
        "/.*/": [ "/--wp-components-color-/" ],
        "flex-direction": "/-reverse/"
    },
    {
        "message": "some message"
    }
],

That doesn't work.

After some research and trials, I discovered a potential issue with the regex for 'any property' /.*/. When changing that for testing purposes to any other CSS property name, it works as expected. For example replacing the regex with opacity will 'work':

"declaration-property-value-disallowed-list": [
    {
        "opacity": [ "/--wp-components-color-/" ],
        "flex-direction": "/-reverse/"
    },
    {
        "message": "some message"
    }
],

Of course, the check for --wp-components-color- would not work any longer. I'm wondering if there are known issues with a regex for the CSS property names like /.*/ as that seems to prevent other properties/values to be added to this rule.

@mirka any clue? When you have a chance 🙇🏽

A quick workaround would be using declaration-property-value-allowed-list instead and list the allowed values.

mirka commented 6 days ago

It stops at the first possible match, so we need to swap the order:

    {
        "flex-direction": "/-reverse/",
        "/.*/": [ "/--wp-components-color-/" ]
    },

And we'll probably want to convert the .stylelintrc.json to a .stylelintrc.js at this point, so we can configure the message as a function to show different warning messages for each violation. It looks like we'll also need to update our stylelint dependency to achieve that. Our repo is at stylelint 14.2, whereas the feature we need is available from 14.5.0.

afercia commented 5 days ago

so we can configure the message as a function to show different warning messages for each violation

Yes that would be nice to have. For now, I reversed the approach and I'm using the allowed list so that I can also use a specific message.

Edit: I will file a new issue to keep track of it.