backpack icon indicating copy to clipboard operation
backpack copied to clipboard

Luna 1326: Changes BpkSlider base library for mobile accessibility

Open LouiseReid opened this issue 2 years ago • 16 comments

Base library updated to radux-ui/react-slider which allows for screen reader swipe interaction on mWeb.

All props of previous react-slider library have been changed to ensure 99% backwards compatibility however ariaLabel is now a required prop

The Problem we had - swiping does not move thumb

https://github.com/Skyscanner/backpack/assets/28352715/bc461f79-5f5a-4cf8-8de6-2dfa8e10f9af

With new base library - note this is using the dist build from this PR in banana

https://github.com/Skyscanner/backpack/assets/28352715/5e46ba44-16bd-427f-bb1f-8d0277fffa80

Breaking change

  • ariaLabel is now a required prop
  • className prop removed

Remember to include the following changes:

  • [x] Ensure the PR title includes the name of the component you are changing so it's clear in the release notes for consumers of the changes in the version e.g [KOA-123][BpkButton] Updating the colour
  • [ ] README.md (If you have created a new component)
  • [ ] Component README.md
  • [x] Tests
  • [x] Storybook examples created/updated
  • [ ] Type declaration (.d.ts) files updated
  • [ ] For breaking changes or deprecating components/properties, migration guides added to the description of the PR. If the guide has large changes, consider creating a new Markdown page inside the component's docs folder and link it here

LouiseReid avatar Apr 16 '24 15:04 LouiseReid

Browser support

If this is a visual change, make sure you've tested it in multiple browsers.

Generated by :no_entry_sign: dangerJS against 7ee61056effa1333d5cb9f7c593ce39864d0244b

github-actions[bot] avatar Apr 16 '24 15:04 github-actions[bot]

Visit https://backpack.github.io/storybook-prs/3368 to see this build running in a browser.

github-actions[bot] avatar Apr 16 '24 15:04 github-actions[bot]

Visit https://backpack.github.io/storybook-prs/3368 to see this build running in a browser.

github-actions[bot] avatar Apr 16 '24 15:04 github-actions[bot]

Visit https://backpack.github.io/storybook-prs/3368 to see this build running in a browser.

github-actions[bot] avatar Apr 17 '24 09:04 github-actions[bot]

Visit https://backpack.github.io/storybook-prs/3368 to see this build running in a browser.

github-actions[bot] avatar Apr 17 '24 09:04 github-actions[bot]

Thanks @LouiseReid for the PR this is interesting this issue came up as we recently completed an audit for this component based on the old library and there were no screenreader issues coming. So this PR comes as a shock as its not working when it was passed in audits

olliecurtis avatar Apr 18 '24 08:04 olliecurtis

Thanks @LouiseReid for the PR this is interesting this issue came up as we recently completed an audit for this component based on the old library and there were no screenreader issues coming. So this PR comes as a shock as its not working when it was passed in audits

Its strange as the keyboard arrow keys for navigation work on desktop which is what I believe should also power the mobile screen reader swipe but they don't

LouiseReid avatar Apr 18 '24 08:04 LouiseReid

Thanks @LouiseReid for the PR this is interesting this issue came up as we recently completed an audit for this component based on the old library and there were no screenreader issues coming. So this PR comes as a shock as its not working when it was passed in audits

Its strange as the keyboard arrow keys for navigation work on desktop which is what I believe should also power the mobile screen reader swipe but they don't

Gotcha I see, now trying again on some different devices!

Could you also check the storybook for this? https://backpack.github.io/storybook-prs/3368/?path=/story/bpk-component-slider--visual-test

I just tried on Chrome on Android and its not working as expected, its the same behaviour of not slidiing but this time no highlighting of the slider controls

olliecurtis avatar Apr 18 '24 08:04 olliecurtis

Visit https://backpack.github.io/storybook-prs/3368 to see this build running in a browser.

github-actions[bot] avatar Apr 18 '24 10:04 github-actions[bot]

Will pause this just now. I've raised an issue on the source repo - https://github.com/radix-ui/primitives/issues/2850

Will give that a couple of weeks then we can decided next steps - nothing, finding another lib or merging as is to at least fix for iOS

LouiseReid avatar Apr 18 '24 13:04 LouiseReid

@olliecurtis I've hit a bit of a wall with trying to get this working with Android talkback. I propose we merge as is at the next appropriate time given it enhances iOS and does nothing detrimental to android

LouiseReid avatar May 03 '24 10:05 LouiseReid

Visit https://backpack.github.io/storybook-prs/3368 to see this build running in a browser.

github-actions[bot] avatar May 03 '24 10:05 github-actions[bot]

Visit https://backpack.github.io/storybook-prs/3368 to see this build running in a browser.

github-actions[bot] avatar May 06 '24 11:05 github-actions[bot]

Adding DON'T MERGE label so we don't merge until the point of next major and batch together

olliecurtis avatar May 06 '24 11:05 olliecurtis

Also to check do we need to have a migration guide

No, its been make backward compatible matching the api of the old library with the only breaking change being the need to now send an array of aria labels (previously was also array but not required).

We could also make that an optional prop as to not be a breaking change but really aria labels and aria value text values should really be given for a11y

LouiseReid avatar May 06 '24 12:05 LouiseReid

Visit https://backpack.github.io/storybook-prs/3368 to see this build running in a browser.

github-actions[bot] avatar May 06 '24 12:05 github-actions[bot]

Visit https://backpack.github.io/storybook-prs/3368 to see this build running in a browser.

github-actions[bot] avatar May 09 '24 09:05 github-actions[bot]