Skip to content

fix(pagination):improve focus style to meet WCAG 2.2 AA#1612

Merged
dancormier merged 2 commits intodevelopfrom
STACKS-544/focus-style-pagination
Jan 26, 2024
Merged

fix(pagination):improve focus style to meet WCAG 2.2 AA#1612
dancormier merged 2 commits intodevelopfrom
STACKS-544/focus-style-pagination

Conversation

@dancormier
Copy link
Copy Markdown
Contributor

@dancormier dancormier commented Jan 25, 2024

Addresses STACKS-544

Technical notes

You'll notice that this PR achieves the focus style mainly with box-shadow:

   box-shadow: inset 0 0 0 var(--su-static2) var(--theme-secondary-400), inset 0 0 0 var(--su-static4) var(--black-050);
    …
   outline: var(--su-static2) solid transparent;

This is because box-shadow tends to align more consistently. Additionally, a transparent outline is used to ensure that an outline doesn't render when expected but can still be rendered in forced-color modes (see https://blogs.windows.com/msedgedev/2020/09/17/styling-for-windows-high-contrast-with-new-standards-for-forced-colors/#gist105426765).

@netlify
Copy link
Copy Markdown

netlify Bot commented Jan 25, 2024

Deploy Preview for stacks ready!

Name Link
🔨 Latest commit 5c9661f
🔍 Latest deploy log https://app.netlify.com/sites/stacks/deploys/65b3e335b3e96c0008a33912
😎 Deploy Preview https://deploy-preview-1612--stacks.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@dancormier dancormier marked this pull request as ready for review January 25, 2024 21:31
Copy link
Copy Markdown
Contributor

@giamir giamir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Great work 🎉

Copy link
Copy Markdown
Collaborator

@CGuindon CGuindon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Visually, everything looks good, from other examples I'm seeing online I think we need Focus to land on the selected/active page number as well. In the preview, it skips over page 1 when I tab through the elements.

The current page is indicated by aria-current="page".

Other resource

Unless I'm missing something...

@dancormier
Copy link
Copy Markdown
Contributor Author

Visually, everything looks good, from other examples I'm seeing online I think we need Focus to land on the selected/active page number as well. In the preview, it skips over page 1 when I tab through the elements.

The current page is indicated by aria-current="page".

Other resource

Unless I'm missing something...

I've added the focus style on the pagination item with .is-selected, but it would only be focused if it were rendered as a link. We've historically omitted the link on the active element, though I can change that (it would only change that in the docs and consumers of the component would need to update if that want to match that). I'll update the docs to include a linked selected pagination item.

image

@dancormier dancormier requested a review from CGuindon January 26, 2024 16:56
@dancormier
Copy link
Copy Markdown
Contributor Author

@CGuindon I've update the pagination docs to include a linked selected pagination item and I've added the visually hidden "page" text inspired by the W3C design system:

For all pagination links excluding the current page, page is added to provide additional context to the link wording for users of Assistive Technology.

@CGuindon
Copy link
Copy Markdown
Collaborator

@dancormier Would it be possible to add a test for this? So that any pagination buttons that are missing this piece in core would then show up in our automated tests list of issues with exact locations? (or am I dreaming to big)

Copy link
Copy Markdown
Collaborator

@CGuindon CGuindon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My question about the test is separate from the original ticket — everything from the requirements LGTM

@dancormier
Copy link
Copy Markdown
Contributor Author

@dancormier Would it be possible to add a test for this? So that any pagination buttons that are missing this piece in core would then show up in our automated tests list of issues with exact locations? (or am I dreaming to big)

Dreaming big is good! That said, I think automated accessibility testing may catch failures like aria-current="page" missing, but I'm not 100% sure about that. As far as using an anchor for the current page, I don't believe that's an explicit requirement for WCAG AA (just a decent practice) so automated tests for sure wouldn't call that out.

It's pretty hard to enforce our components to be marked up exactly as we'd want in our consumer code unless they're using something like a razor or svelte component that handles the markup for the consumer. At some point, we'll probably want to work with Core to help design their accessibility tests broadly which may help with this, but I think this will probably need to be manual for the moment.

@dancormier dancormier merged commit b84ac3a into develop Jan 26, 2024
@dancormier dancormier deleted the STACKS-544/focus-style-pagination branch January 26, 2024 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants