Add stylelint rule to prevent usage of flex-direction reverse values #63081
Conversation
…e refactored later.
afercia
added
[Focus] Accessibility (a11y)
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: +266 B (+0.02%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
|
@WordPress/gutenberg-core any chance you have time for a review? 🙏🏽 |
.stylelintrc.json
Outdated
| "column", | ||
| "inherit", | ||
| "initial", | ||
| "unset" |
There was a problem hiding this comment.
Is there a way to allow variables as values?
.stylelintrc.json
Outdated
| "row", | ||
| "column", | ||
| "inherit", | ||
| "initial", | ||
| "unset" |
There was a problem hiding this comment.
We should be able to do it with regex. I think it might be easier if we just list all the possible reverse values and with a negative regex (untested):
| "row", | |
| "column", | |
| "inherit", | |
| "initial", | |
| "unset" | |
| "/-reverse$/", |
That will also allow using variables as @youknowriad raised above.
There was a problem hiding this comment.
@tyxla sure but in order to do that we should use declaration-property-value-disallowed-list which is already used. It's not clear to me how to add multiple disallowed properties with custom messages under the same Stylelint rule. Any clue?
There was a problem hiding this comment.
add multiple disallowed properties with custom messages under the same Stylelint rule
This is where we need to first convert the config file from JSON to JS, as well as update the Stylelint dependency to a newer version that supports custom message functions for this rule (#63090).
There was a problem hiding this comment.
@tyxla sure but in order to do that we should use
declaration-property-value-disallowed-listwhich is already used. It's not clear to me how to add multiple disallowed properties with custom messages under the same Stylelint rule. Any clue?
Good point, but I think you should be able to do it with declaration-property-value-allowed-list, just need to invert the regex to match anything BUT row-reverse and column-reverse, for example:
"/^(?!(row|column)-reverse).*$/"
Haven't tested it, so let me know if that works for you.
There was a problem hiding this comment.
Otherwise, converting to the JS format makes sense, although it doesn't look like a high-priority task.
I think it needs to be clarified and documented/agreed upon whether it's okay not to add block deprecation to experimental blocks. We are attempting to add a block deprecation to the Form block in #55755. See the discussion starting in this comment for more details. Personally, I think it's out of scope to update the For now, I think it's better to just ignore |
What cases are valid? |
Only the ones where reordering doesn't affect 'meaning and operability'. WCAG 2.2 1.3.2 Meaningful Sequence (Level A) https://www.w3.org/TR/WCAG22/#meaningful-sequence 2.4.3 Focus Order (Level A) https://www.w3.org/TR/WCAG22/#focus-order CSS Flexible Box Layout Module Level 1 |
Thanks @tyxla the regex seems to work nicely. Pushed in the last commit. To test:
Playing devil's advocate: is there any way to catch a variable when the variable resolves to a |
Glad to hear!
Not really - stylelint, like most linters, performs static code analysis and has no way to deal with what's inside the dynamic values. This shouldn't be something to worry about, though; a reverse flex-direction in a variable sounds like a particularly rare scenario we'll likely never see. |
Hopefully 😆 |
|
Merging this PR will implicitly add a rule that says "We don't have to add block deprecation for experimental blocks", but is that okay? The reason I'm so particular about this is that, as far as I know, whenever the save function of a form-related block was updated, a block deprecation was added without exception. And there is still an open PR that has been trying to add a block deprecation for a long time. Personally, I propose to revert the update of the form block's save function from this PR and simply add an ignore style rule. |
|
Thanks @t-hamano. I see a Reverting the update of the form block's save function wouldn't be great because it needs to be fixed anyways, either in this PR or a new one. Open to other thoughts Cc @aristath @carolinan |
|
I think it's OK to ship this PR as long as it includes the appropriate block deprecation. Personally, I'd recommend addressing the block deprecation in a follow-up PR, since the purpose of this PR is to add a stylelint rule. |
|
Thanks @t-hamano. In the next weeks I'll be away and won't have time to look into this. |
|
I'm happy to take over this PR. In order to get this PR ready to ship sooner, would it be ok to revert the changes in the |
|
@t-hamano as long as it gets fixed soon in a follow-up, no objections from me. |
|
I have reverted the changes to Can this PR be shipped? |
Fixes #63048
What?
The
flex-directionCSS property valuesrow-reverseandcolumn-reverseshould not be used when they make visual order and DOM order mismatch in a way that impacts meanind and interaction. That is, in almost all cases they should not be used. See the issue for more details.Why?
For keyboard users and assistive technology users, the visual order and DOM order must match.
How?
declaration-property-value-allowed-listrule forflex-directionallowing only the following values:core/form-inputblock. The checkbox label is no longer swapped via CSS. There's now a condition to render the checkbox and its label in the expected order. This required to modify the edit and save functions of the block. I'm not too concerned about it as this is still flagged as a Gutenberg experiment.Valid cases:
packages/block-library/src/navigation/style.scsspackages/block-library/src/page-list/style.scssThese flex-direction properties use a CSS variable
--navigation-layout-directionthat would be flagged as an error. To my understanding, the variable value is either 'row', 'column' or 'initial' so it's OK.Cases that need refactoring. I will split them in separate issues:
packages/block-editor/src/hooks/block-hooks.scsspackages/block-editor/src/hooks/use-editor-wrapper-styles.native.scsspackages/block-library/src/media-text/style.native.scssTesting Instructions
npm run lint:css.Form and input blocksexperiment.Testing Instructions for Keyboard
Screenshots or screencast