Repository navigation
Conversation
|
Size Change: 0 B Total Size: 7.91 MB |
|
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. |
| ref={ ref } | ||
| className={ clsx( styles.chip, className ) } | ||
| { ...restProps } | ||
| aria-label={ chipAriaLabel } |
There was a problem hiding this comment.
Tecnically we're adding an aria-label to a plain div, but the ARIA generic role prohibits naming, and ARIA in HTML prohibits aria-label on roles that cannot be named.
Should we give the default chip a nameable role or element, maybe group or something else?
| const removeAriaLabel = | ||
| removeAriaLabelProp ?? | ||
| ( typeof children === 'string' | ||
| ? sprintf( | ||
| /* translators: %s: chip label. */ | ||
| __( 'Remove %s' ), | ||
| children | ||
| ) | ||
| : __( 'Remove' ) ); |
There was a problem hiding this comment.
Previously we documented removeLabel as the remove button's accessible label. This PR changes it to tooltip-only, so existing code still compiles but gets a different accessible name. For example, removeLabel="Delete Apple" can produce a Delete Apple tooltip and a button named Remove, which can be problematic.
Should we removeLabel as the fallback when removeAriaLabel is absent? Something like
removeAriaLabelProp ??
removeLabelProp ??
( chipAriaLabel
? sprintf( __( 'Remove %s' ), chipAriaLabel )
: __( 'Remove' ) );This would preserve existing consumers, while letting an explicit chip label provide the default remove-button context.
Feel free to ignore if we don't care as much about introducing these small regressions in existing usage
| <Combobox.ChipWithRemove | ||
| key={ item.id } | ||
| removeLabel={ `Remove ${ item.value }` } | ||
| removeAriaLabel={ `Remove ${ item.value }` } |
There was a problem hiding this comment.
Should we test a value that is different from the default? Otherwise the test would still pass even when the prop is ignored
|
Closing in favor of #82863. |
What?
Improves accessible naming for
Combobox.ChipWithRemovechips and their remove buttons.Why?
When virtual focus moves to a chip via arrow keys, screen readers were announcing the chip name followed by "Remove", because the nested remove button label was being aggregated into the chip's accessible name. Separately, every remove button shared the same generic "Remove" accessible name when activated directly.
This follows the pattern used in Base UI's Combobox multiple-select example, which sets an explicit
aria-labelon the chip.How?
aria-labelon the chip from stringchildrenwhen no explicit label is passed.childrenas a translatedRemove %s.Remove, since the chip name is already visible beside the X.Tooltip+Button+Icondirectly, rather than changingIconButton's one-label contract.removeLabel(tooltip) andremoveAriaLabel(accessible name) props onChipWithRemove.chipsContentwith non-string chip content should pass explicitaria-labelandremoveAriaLabel.Testing Instructions