RadioControl: label radio group using fieldset and legend #64582
Conversation
|
Size Change: +41 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
ciampo
added
[Type] Enhancement
| // 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)
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:
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)
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:
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. |
| <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.
| 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)
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: |
…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>
vcanales
added
Backport to WP Minor Release
|
Developers may be using this component in a block sidebar. This component is no longer Code to reproduce this issuewp.blocks.registerBlockType( 'test/test', {
apiVersion: 3,
title: 'Test Block',
edit: () => {
return wp.element.createElement(
'div',
wp.blockEditor.useBlockProps(),
wp.element.createElement( 'div', null, 'Test Block' ),
wp.element.createElement(
wp.editor.InspectorControls,
null,
wp.element.createElement(
wp.components.PanelBody,
{
title: 'Test',
},
wp.element.createElement( wp.components.RadioControl, {
label: 'Radio 1',
options: [
{
label: 'Option 1',
value: 'option-1',
},
{
label: 'Option-2',
value: 'option-2',
},
],
} ),
wp.element.createElement( wp.components.RadioControl, {
label: 'Radio 1',
options: [
{
label: 'Option 1',
value: 'option-1',
},
{
label: 'Option-2',
value: 'option-2',
},
],
} )
)
)
);
},
} );
Until margin-free styles become the standard, how about adding fallback styles like the following: .block-editor-block-inspector {
.components-radio-control {
&:where(:not(:last-child)) {
margin-bottom: $grid-unit-20;
}
}
} |
Good catch. But I'm hesitant to add a fallback at this stage, because any extender who may have already addressed this in the RC (e.g. with a flex gap) could see it break again 🥲 I slightly lean towards leaving this as is. What do you think? |
|
If extenders were to restore the spacing using gaps or something, I don't think the fallback margin would break the layout too much. I think the 16px increase in spacing is acceptable. On the other hand, for extenders that don't address this issue, there will be no spacing at all, which might be considered a broken layout. By the way, is the fallback margin for Update: Might be worth considering fixing this in a minor release (6.7.1). |
You're right, that makes sense. Let's ship the fallback then.
In any case I'm not planning on applying the new spacing solution without explicit opt-in from the extender, so a fallback margin will be needed regardless of this matter. |
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.