[IRN-5039] BottomSheet supports aria-label #3427

steviehailey-skyscanner · 2024-05-08T09:14:15Z

Description

Previously BottomSheet consumers had to provide an aria-labelledBy to be attached to the dialog element but it is rare that they know the relevant title element id to provide.

Now BottomSheet consumers can provide either:
a. aria-labelledBy - if they are not providing a string title and want to reference an element within the content as a label
b. aria-label - if they would like to provide an a11y string (e.g. same value as title or a more accessible equivalent)

Screenshot - BottomSheet consumer can now control aria-label on dialog element

Remember to include the following changes:

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
Tests
Storybook examples created/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

steviehailey-skyscanner · 2024-05-08T09:15:09Z

examples/bpk-component-bottom-sheet/examples.tsx

@@ -146,7 +146,7 @@ BottomSheetContainerProps,
            Open bottom sheet
          </BpkButtonV2>
          <BpkBottomSheet
-            ariaLabelledby='test'


Make the default demo more of a valid example - there was no #test element. Now screen reader reads the a11y title.

steviehailey-skyscanner · 2024-05-08T09:15:39Z

packages/bpk-component-bottom-sheet/src/BpkBottomSheet.tsx

@@ -31,9 +31,8 @@ import STYLES from './BpkBottomSheet.module.scss';

 const getClassName = cssModules(STYLES);

-export type Props = {
+interface CommonProps {


Extract out the non-a11y props so we can conditionally allow either label or labelledBy...

steviehailey-skyscanner · 2024-05-08T09:15:53Z

packages/bpk-component-bottom-sheet/src/BpkBottomSheet.tsx

-  title = '',
-  wide = false
-}: Props) => {
+export type Props = CommonProps & ({ ariaLabelledby: string } | { ariaLabel: string; });


Ensure we get one aria prop or the other

steviehailey-skyscanner · 2024-05-08T09:16:27Z

packages/bpk-component-bottom-sheet/src/BpkBottomSheet.tsx

@@ -92,7 +93,7 @@ const BpkBottomSheet = ({
  return <BpkBreakpoint query={BREAKPOINTS.ABOVE_MOBILE}>
    {(isAboveMobile: boolean) =>
      <BpkDialogWrapper
-        ariaLabelledby={ariaLabelledby}
+        {...props}


Pass down the a11y props (we don't know which one we'll have but TS assures we'll have one)

steviehailey-skyscanner · 2024-05-08T09:16:35Z

packages/bpk-react-utils/src/BpkDialogWrapper/BpkDialogWrapper.tsx

 };

+export type Props = CommonProps & ({ ariaLabelledby: string } | { ariaLabel: string; });;


Same pattern as above

steviehailey-skyscanner · 2024-05-08T09:16:56Z

packages/bpk-react-utils/src/BpkDialogWrapper/BpkDialogWrapper.tsx

@@ -135,6 +135,11 @@ export const BpkDialogWrapper = (props: Props) => {
    };
  }, [id, isOpen, onClose, closeOnEscPressed, closeOnScrimClick]);

+  const ariaProps = {


Extract whichever aria prop has been provided to add the the dialog element

github-actions · 2024-05-08T09:17:30Z

	Warnings
⚠️	Package source files (e.g. `packages/package-name/src/Component.js`) were updated, but snapshots weren't. Have you checked that the tests still pass?
⚠️	Package source files (e.g. `packages/package-name/src/Component.tsx`) were updated, but type files weren't. Have you checked that no types have changed?

Browser support

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

Generated by 🚫 dangerJS against aaba15a

github-actions · 2024-05-08T09:17:51Z

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

anambl · 2024-05-08T10:58:23Z

packages/bpk-component-bottom-sheet/src/BpkBottomSheet.tsx

-}: Props) => {
+export type Props = CommonProps & ({ ariaLabelledby: string } | { ariaLabel: string; });
+
+const BpkBottomSheet = (props: Props) => {


why not keep it as it was previously and destructure it here? 🤔

This is a limitation of the mutually exclusive aria props unfortunately. TS won't allow destructuring of those fields because it knows one will not be there so by keeping the props intact we can safely reference or pass them on (subject to performing checks for existence to keep TS happy).

In this component we just pass whichever we have by spreading the props

In BpkDialogWrapper we have an example of checking and using a specific one here: https://github.com/Skyscanner/backpack/pull/3427/files#diff-8ae3b880e3772a84f85517514640da0930f295cffbdfd531fcf063a29ba84606R139

I see, but we could also destructure the props here and spread the aria props rather than explicitly destructure them? Something like

const BpkBottomSheet = ({actionText = '', children, etc, ...ariaProps}) => {...}

?
In which case we'd also pass whichever we have

Ah, yeah we could use that syntax to keep the destructure - will push an update 🚀

steviehailey-skyscanner · 2024-05-08T15:58:31Z

packages/bpk-react-utils/src/BpkDialogWrapper/BpkDialogWrapper-test.tsx

+        </BpkDialogWrapper>
+      );
+
+      expect(container.querySelector('dialog[aria-labelledby="my-title-id"]')).toBeInTheDocument();


Check aria correctly applied
(I noticed most other tests use snapshots but this feels more explicit on what we're looking for while still capturing a low level of DOM detail for the relevant parts)

steviehailey-skyscanner · 2024-05-08T15:58:59Z

packages/bpk-react-utils/src/BpkDialogWrapper/accessibility-test.tsx

+    expect(results).toHaveNoViolations();
+  });
+
+  it('should not have programmatically-detectable accessibility issues using airaLabel', async () => {


Additional axe check for alternative aria

github-actions · 2024-05-08T16:01:04Z

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

steviehailey-skyscanner · 2024-05-09T09:17:16Z

packages/bpk-component-bottom-sheet/src/BpkBottomSheet-test.tsx


 import BpkBottomSheet from './BpkBottomSheet';
 // mock breakpoint to always match
 jest.mock('../../bpk-component-breakpoint/src/useMediaQuery', () => jest.fn(() => true));
 describe('BpkBottomSheet', () => {
+
+  const props = {


Extract out some common props to reduce repetition and let the cases focus on the props of interest.

steviehailey-skyscanner · 2024-05-09T09:17:49Z

packages/bpk-component-bottom-sheet/src/BpkBottomSheet-test.tsx

        wide
      >
        Bottom Sheet content
      </BpkBottomSheet>
    );
    expect(asFragment()).toMatchSnapshot();
  });
+  it('renders correctly with ariaLabelledBy prop', () => {


New cases to explicitly check the propagation of the aria props (based on the DialogWrapper ones)

github-actions · 2024-05-09T09:18:30Z

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

anambl · 2024-05-09T10:19:13Z

packages/bpk-react-utils/src/BpkDialogWrapper/BpkDialogWrapper.tsx

 };

+export type Props = CommonProps & ({ ariaLabelledby: string } | { ariaLabel: string; });;


I think there's an extra ;?

anambl · 2024-05-09T10:19:42Z

packages/bpk-react-utils/src/BpkDialogWrapper/BpkDialogWrapper.tsx

@@ -177,6 +182,6 @@ export const BpkDialogWrapper = (props: Props) => {
          </div>
        </dialog>
      </CSSTransition>
-    </div>
+    </div >


Is this an automatic linting change?

Not sure what happened there - the linter is happy with or without the space so will remove 😆

anambl · 2024-05-09T10:20:22Z

packages/bpk-component-bottom-sheet/src/BpkBottomSheet.tsx

-  title = '',
-  wide = false
-}: Props) => {
+export type Props = CommonProps & ({ ariaLabelledby: string } | { ariaLabel: string; });


Interesting, I thought interfaces could only be extended and & wouldn't work on them

Yeah there is some subtlety there, you can mention interfaces when using intersection to define a type (you just can define an interface using intersection).

https://www.typescriptlang.org/docs/handbook/2/everyday-types.html#differences-between-type-aliases-and-interfaces

github-actions · 2024-05-09T10:37:46Z

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

github-actions · 2024-05-09T10:41:28Z

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

anambl · 2024-05-09T14:16:12Z

packages/bpk-component-bottom-sheet/src/BpkBottomSheet-test.tsx

+    const { container } = render(
+      <BpkBottomSheet
+        {...props}
+        ariaLabel='my a11y title'


I'm guessing this will take precedence since it's added after ariaLabelledBy? 🤔

I have done some testing in Safari with VoiceOver and if you give it a valid ariaLabelledBy then it will use that. If the labelledBy was missing or invalid then it falls back to the aria-label instead.

anambl · 2024-05-09T14:16:27Z

packages/bpk-component-bottom-sheet/src/BpkBottomSheet-test.tsx

+    const { container } = render(
+      <BpkBottomSheet
+        {...props}
+        ariaLabelledby='my-bottomsheet-title-id'


it's already part of props so I don't think we need it here?

Yeah I we could avoid it but I thought it was nice to be explicit when we are asserting based on that property?

anambl · 2024-05-09T14:18:47Z

packages/bpk-react-utils/src/BpkDialogWrapper/BpkDialogWrapper-test.tsx

+      const { container } = render(
+        <BpkDialogWrapper
+          {...props}
+          ariaLabelledby='my-title-id'


Same here, it's already declared in props

anambl · 2024-05-09T14:23:05Z

It all looks great to me!
What happens if you don't pass neither or if you pass both aria props? For the first, I assume a TS error, but I don't think it's possible to unit test that 😅 And for the 2nd, the latter takes precedence? 🤔

steviehailey-skyscanner · 2024-05-09T14:42:23Z

It all looks great to me! What happens if you don't pass neither or if you pass both aria props? For the first, I assume a TS error, but I don't think it's possible to unit test that 😅 And for the 2nd, the latter takes precedence? 🤔

Indeed TS will prevent the former but if someone ignored the TS error then things function but you get a degraded screenreader experience because it often describes the "Close button" before describing what the dialog is for. For the later case, it seems that labelledBy is used in favour of label if someone did provide both which seems reasonable.

github-actions · 2024-05-13T07:33:33Z

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

* [IRN-5039] BottomSheet supports aria-label * Add tests for modal wrapper * Add bottom sheet aria tests * PR feedback

[IRN-5039] BottomSheet supports aria-label

b0d7afd

steviehailey-skyscanner commented May 8, 2024

View reviewed changes

anambl reviewed May 8, 2024

View reviewed changes

Add tests for modal wrapper

a2be832

steviehailey-skyscanner commented May 8, 2024

View reviewed changes

steviehailey-skyscanner added the minor Non breaking change label May 9, 2024

Add bottom sheet aria tests

5ab7cae

steviehailey-skyscanner commented May 9, 2024

View reviewed changes

steviehailey-skyscanner marked this pull request as ready for review May 9, 2024 09:31

anambl reviewed May 9, 2024

View reviewed changes

PR feedback

33f2e46

Merge branch 'main' into IRN-5039

5d316f9

anambl reviewed May 9, 2024

View reviewed changes

Merge branch 'main' into IRN-5039

aaba15a

anambl approved these changes May 13, 2024

View reviewed changes

anambl merged commit b0093c8 into main May 13, 2024
9 checks passed

anambl deleted the IRN-5039 branch May 13, 2024 07:55

FireRedNinja pushed a commit that referenced this pull request May 22, 2024

[IRN-5039] BottomSheet supports aria-label (#3427)

74a9396

* [IRN-5039] BottomSheet supports aria-label * Add tests for modal wrapper * Add bottom sheet aria tests * PR feedback

KathyWang0208 pushed a commit that referenced this pull request May 27, 2024

[IRN-5039] BottomSheet supports aria-label (#3427)

fa49fed

* [IRN-5039] BottomSheet supports aria-label * Add tests for modal wrapper * Add bottom sheet aria tests * PR feedback

KathyWang0208 pushed a commit that referenced this pull request May 27, 2024

[IRN-5039] BottomSheet supports aria-label (#3427)

3f409cb

* [IRN-5039] BottomSheet supports aria-label * Add tests for modal wrapper * Add bottom sheet aria tests * PR feedback

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[IRN-5039] BottomSheet supports aria-label #3427

[IRN-5039] BottomSheet supports aria-label #3427

steviehailey-skyscanner commented May 8, 2024 •

edited

steviehailey-skyscanner May 8, 2024

steviehailey-skyscanner May 8, 2024

steviehailey-skyscanner May 8, 2024

steviehailey-skyscanner May 8, 2024

steviehailey-skyscanner May 8, 2024

steviehailey-skyscanner May 8, 2024

github-actions bot commented May 8, 2024 •

edited

github-actions bot commented May 8, 2024

anambl May 8, 2024

steviehailey-skyscanner May 8, 2024 •

edited

anambl May 9, 2024

steviehailey-skyscanner May 9, 2024

steviehailey-skyscanner May 8, 2024 •

edited

steviehailey-skyscanner May 8, 2024

github-actions bot commented May 8, 2024

steviehailey-skyscanner May 9, 2024

steviehailey-skyscanner May 9, 2024

github-actions bot commented May 9, 2024

anambl May 9, 2024

anambl May 9, 2024

steviehailey-skyscanner May 9, 2024

anambl May 9, 2024

steviehailey-skyscanner May 9, 2024

github-actions bot commented May 9, 2024

github-actions bot commented May 9, 2024

anambl May 9, 2024

steviehailey-skyscanner May 9, 2024

anambl May 9, 2024

steviehailey-skyscanner May 9, 2024

anambl May 9, 2024

anambl commented May 9, 2024

steviehailey-skyscanner commented May 9, 2024

github-actions bot commented May 13, 2024

		};

		export type Props = CommonProps & ({ ariaLabelledby: string } \| { ariaLabel: string; });;

[IRN-5039] BottomSheet supports aria-label #3427

[IRN-5039] BottomSheet supports aria-label #3427

Conversation

steviehailey-skyscanner commented May 8, 2024 • edited

Description

Screenshot - BottomSheet consumer can now control aria-label on dialog element

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 8, 2024 • edited

Browser support

github-actions bot commented May 8, 2024

Choose a reason for hiding this comment

steviehailey-skyscanner May 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steviehailey-skyscanner May 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 9, 2024

github-actions bot commented May 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anambl commented May 9, 2024

steviehailey-skyscanner commented May 9, 2024

github-actions bot commented May 13, 2024

steviehailey-skyscanner commented May 8, 2024 •

edited

github-actions bot commented May 8, 2024 •

edited

steviehailey-skyscanner May 8, 2024 •

edited

steviehailey-skyscanner May 8, 2024 •

edited