RadioControl: label radio group using fieldset and legend #64582
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +41 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
ciampo
added
[Type] Enhancement
ciampo
commented
Aug 17, 2024
Comment on lines
92
to
101
| // Apply the same styles that would be applied to | ||
| // ".block-editor-block-inspector .components-base-control" | ||
| // (see packages/block-editor/src/components/block-inspector/style.scss) | ||
| .wp-block-latest-posts__post-content-radio { | ||
| margin-bottom: #{ $grid-unit-30 }; | ||
|
|
||
| &:last-child { | ||
| margin-bottom: $grid-unit-10; | ||
| } | ||
| } |
There was a problem hiding this comment.
These changes were necessary because previously the margin was applied based on the components-base-control classname. With the changes that this PR applies to RadioControl, the component does not get the components-base-control classname applied. (see related #64526)
ciampo
commented
Aug 17, 2024
There was a problem hiding this comment.
Those style overrides aren't needed anymore after the recent changes in RadioControl to spacing and the added support for individual option help text.
The component should render as expected:
ciampo
commented
Aug 17, 2024
There was a problem hiding this comment.
Those style overrides aren't needed anymore after the recent changes in RadioControl to spacing and the added support for individual option help text.
The component should render as expected:
(PS you need to activate an older theme like TwentyWentyTwelve to show the "Format" button in the post tab)
ciampo
commented
Aug 17, 2024
There was a problem hiding this comment.
Those style overrides aren't needed anymore after the recent changes in RadioControl to spacing and the added support for individual option help text.
The component should render as expected:
ciampo
commented
Aug 17, 2024
There was a problem hiding this comment.
Those style overrides aren't needed anymore after the recent changes in RadioControl to spacing and the added support for individual option help text.
The component should render as expected:
|
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. |
tyxla
reviewed
Aug 19, 2024
Comment on lines
+145
to
+151
| <StyledHelp | ||
| __nextHasNoMarginBottom | ||
| id={ generateHelpId( id ) } | ||
| className="components-base-control__help" | ||
| > | ||
| { help } | ||
| </StyledHelp> |
There was a problem hiding this comment.
All this may be a good opportunity to export and reuse directly from BaseControl.
There was a problem hiding this comment.
StyledHelp is already being imported from BaseControl (it's just not exported publicly, hence why we're not using BaseControl.StyledHelp).
I'm hesitant to make changes to BaseControl, because I believe we would soon like to take a good look at the component and potentially work on a new version (see #60836)
There was a problem hiding this comment.
Sure, but this entire help part is just duplicated from BaseControl and we could easily reuse. No strong feelings if you are hesitant to touch it though.
There was a problem hiding this comment.
The part that we're not really extracting and re-using from BaseControl is the logic to generate an id for the help text, and use it for the aria-describedby attribute on the fieldset — but it's quite a standard practice, and since we're already going quite custom with fieldset and legend, I don't think that putting time into this could bring much benefit at this point in time.
| hideLabelFromVision={ hideLabelFromVision } | ||
| help={ help } | ||
| className={ clsx( className, 'components-radio-control' ) } | ||
| className={ clsx( 'components-radio-control', className ) } |
There was a problem hiding this comment.
Reordering the classnames now can lead to 3rd party code specificity issues where selectors aren't specific enough. Should we do it separately and do some research/testing?
There was a problem hiding this comment.
AFAIK, the order declared in the class attribute has no effect — although I'm happy to revert the change and preserve the original order.]
In general, I hope we can assume that it's the consumer's duty to make sure they write robust CSS (ie. not using the private.components-radio-control class name, and providing good specificity instead of relying on classname order), and that we can make such changes without worrying about potential consequences 😅
There was a problem hiding this comment.
I agree with you, mostly. However, my recommendation was exactly that to bringe the gap between "hope" and "confirm" we'll need some testing, and that can also happen in another PR 😉
There was a problem hiding this comment.
FWIW, seconding that changing class name order like this will not affect anything, regardless of how the consumer CSS is written or loaded.
Comment on lines
+2
to
+4
| border: 0; | ||
| margin: 0; | ||
| padding: 0; |
There was a problem hiding this comment.
I was surprised we don't have an underlying component to cover a fieldset
There was a problem hiding this comment.
I don't think we do, also because fieldset is very vanilla (it basically renders a transparent box), and all of its functionality is about semantics.
We could definitely consider making it more central in a hypothetical future version of BaseControl (#60836)
mirka
approved these changes
Aug 20, 2024
There was a problem hiding this comment.
Accessibility tree looks correct 👍 As I understand, ARIA descriptions on group role elements will not be read out by screen readers (at least in VoiceOver or JAWS) in a way that's clearly associated with the group, but just in the flow of content. I'm assuming that is generally better for UX, as it would be tedious for the description to be read out for each interactive element in the group.
And great to see all the style hacks removed 🎉
There was a problem hiding this comment.
You'll want to add these styles to the editor.scss file, as it's only needed in the editor (style.scss is also loaded on the front end).
There was a problem hiding this comment.
Good point, will move before merging
|
Forgot to mention, I also checked the editor-side changes 👍 |
ciampo
force-pushed
the
fix/radio-control-fieldset
branch
from
8691756
to
d924114
Compare
To clarify the screen readers expected behavior and actual behavior: |
bph
pushed a commit
to bph/gutenberg
that referenced
this pull request
Aug 31, 2024
…64582) * Swap BaseControl for fieldset + legend * Add help text * Use group help text to describe the fieldset instead of each individual option * CHANGELOG * Re-apply ID and classname * Render visually hidden label as div * Fix spacing in latest post block * Increase margin-top override specificity * Remove redundant styles * Use legend even when the label is visually hidden * Test that group labelling works as expected also when label is visually hidden * typo * Revert to original classname order * Move styles from styles.scss to editor.scss --- Co-authored-by: ciampo <mciampini@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org> Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: afercia <afercia@git.wordpress.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Fixes #52890
Related to #63751 (comment)
Changes how radio inputs are grouped and labelled in
RadioControl, switching tofieldset+legendWhy?
The previous approach was incorrect and inaccessible (see #52890)
How?
BaseControlfor a customfieldset+legendimplementation. Visual styles have been preserved thanks to usingStyledLabelandStyledHelpcomponents fromBaseControl;fieldset) instead of each optionNext steps
Consider replacing custom implementation in the post visibility UI
Testing Instructions
RadioControlexamples in Storybookfieldset+legendhelpprop) is not used to describe each option, but is instead used to describe the group itself;trunk.components-base-controlanymore, see Normalize block inspector controls spacing #64526 as related PR)Screenshots or screencast
No visible changes.