@W-12627096@ Improve accessibility of variation attribute swatches #1587
Conversation
The swatches are also links, which accomplishes the same thing.
Using React.Children + cloneElement is not recommended because it's weird and confusing.
Signed-off-by: Will Harney <[email protected]>
| onKeyDown={onKeyDown} | ||
| // To mimic the behavior of native radio inputs, only the selected input should be | ||
| // tabbable. (The rest are selectable via arrow keys.) | ||
| tabindex={selected ? 0 : -1} |
There was a problem hiding this comment.
Aside: the native behavior fields awkward in terms of accessibility since the non-selected options are not tabable
There was a problem hiding this comment.
It makes sense if you think of the focused component as the radio group, rather than the individual radio button. Compare with a dropdown, where tab moves focus on/off the <select>, while arrow keys are how you choose an <option>.
| const intl = useIntl() | ||
| const swatches = values.map(({href, name, image, value, orderable}) => { | ||
| /** Mimic the behavior of native radio inputs by using arrow keys to select prev/next value. */ | ||
| const onKeyDown = (evt) => { |
There was a problem hiding this comment.
why don't we put this as a prop for the Swatch-group component letting user to choose to enable/disable this behavior? if we have it here, it looks like it is only being used by ProductView
There was a problem hiding this comment.
One goal of this PR is to update the SwatchGroup component so that it does not modify its children. (i.e. it just renders {children} without changing the props.)
My solution here is definitely not the proper React solution, but I'm not sure the best way to implement this. The two main constraints are:
- (as mentioned)
SwatchGrouprenders its children without modification - Using arrow keys when a swatch is focused changes the selected swatch.
There was a problem hiding this comment.
Should this be the designed behavior for SwatchGroup? For example, if we have to use SwatchGroup somewhere else, using SwatchGroup would create them same accessibility issue, right? 🤔 .
Should we consider rewrite SwatchGroup to support accessibility instead of creating a "Wrapped" component like this one to patch the issue?
There was a problem hiding this comment.
Good call. I've moved the logic back into the Swatch and deleted the wrapper component.
| @@ -108,7 +110,7 @@ describe('Swatch Component', () => { | |||
| const history = createMemoryHistory() | |||
| history.push('/en-GB/swatch-example?color=JJ2XNXX') | |||
|
|
|||
| render( | |||
| renderWithProviders( | |||
There was a problem hiding this comment.
Why do we need to use renderWithProviders? I think it was intentional to not use the it since there is not need to render other providers to test this component.
There was a problem hiding this comment.
Test fails without the providers. With these changes it needs the IntlProvider.
|
@wjhsf I checked out the changes localhost, it seems like the variants are not tablable, and when non variant is selected, using arrow keys are also not working, it will scroll the page. It only works when I use my mouse to click on the selection, then i can use arrow keys to move the variant selection. This seems like less accessible then before imo because I have to use my mouse to locate the variant buttons before I can use the arrow keys |
Is that the case when viewing this product? I can tab / arrow just fine... 🤔 |
|
@wjhsf that is when a variant has been selected, your url has pid, when you navigate from PLP to PDP, there is not variant select yet, so it is impossible to select the variant on that screen unless you use mouse. It was tablable on previous implementation |
Thanks for catching that! I've pushed a fix. |
|
Took a look at the associated ticket (W-12627096) and there's a recommended fix to add |
Signed-off-by: Will Harney <[email protected]>
There was a problem hiding this comment.
@wjhsf Looking at the screenshot linked in the ticket seems that the a11y issue is related to the size refinements buttons of the PLP not having the role HTML attribute.
Why are we not making changes to that file to include therole="switch" attribute?
Because I accidentally improved the accessibility of the wrong swatches. 🤦 This PR improves the accessibility of an area unrelated to the ticket. The root issue of the ticket is that we use buttons to select filters, but don't have any way for a screen reader to tell the user "this filter is not selected; click this button to add it" or "this filter is selected; click this button to remove it". I addressed that issue in #1607 (which is linked to a different ticket that I was working on that touches the same area). My fix is a bit different, it changes the labels rather than updating the ARIA attributes, but it achieves the same goal. |
Description
The implementation of the swatches is weird. It's buttons that act like links that act like radio inputs. Ideally, I would have liked to refactor this to use native radio inputs, because that would be simpler and more semantically correct. However, I couldn't get our current logic ("selected value" derived from the URL) to work nicely with Chakra's radio hooks/components (which, btw, are not particularly accessible!), so I gave up on that approach and just added missing accessibility features to the existing solution. Namely, I made tab navigation skip non-selected radio buttons and added the ability to select options using the arrow keys (see native behavior here). This seems to be as close to the native behavior as we can reasonably get, without doing a complete refactor of the component.
I also pulled the component out into a new file, because the indentation was getting excessive.
Types of Changes
Changes
How to Test-Drive This PR
Checklists
General
Accessibility Compliance
You must check off all items in one of the follow two lists:
or...
Localization