Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
afercia opened this issue Jul 2, 2024 · 3 comments · Fixed by #63081
Closed

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

afercia opened this issue Jul 2, 2024 · 3 comments · Fixed by #63081
Assignees
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Status] In Progress Tracking issues with work in progress [Type] Code Quality Issues or PRs that relate to code quality

Comments

@afercia
Copy link
Contributor

afercia commented Jul 2, 2024

Description

SimilarlySimilarly to #61241 / #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 #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

  • Run a search in the codebase for the string -reverse;.

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 afercia added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Code Quality Issues or PRs that relate to code quality labels Jul 2, 2024
@afercia afercia self-assigned this Jul 2, 2024
@afercia
Copy link
Contributor Author

afercia commented Jul 2, 2024

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
Copy link
Member

mirka commented Jul 2, 2024

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
Copy link
Contributor Author

afercia commented Jul 3, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Status] In Progress Tracking issues with work in progress [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants