Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add overflow checks to code blocks to dynamically remove tabindex #4034

Merged
merged 1 commit into from
Sep 19, 2024

Conversation

querkmachine
Copy link
Member

The recent audit noted a usability issue for users who navigated using the Tab key, wherein they would be made to stop at every code block as they all have the tabindex attribute set. #4007

We set tabindex intentionally so that long lines of code can overflow the container and be scrolled by using the keyboard's arrow keys. We want to maintain this ability (removing it entirely would be a likely violation of Success Criterion 2.1: Keyboard Accessible) but would like to limit it only to blocks that need to scroll.

Changes

  • Adds a data-module attribute to code blocks processed by Markdown.
  • Adds JavaScript to attach a ResizeObserver to each code block.
    • When the page is initially loaded, or the elements are resized, checks if the content of the code block overflows the container.
    • If so, it adds the tabindex attribute. Otherwise, it removes it.

Thoughts

This is only one possible solution to this issue, if we opt to implement a 'fix' at all.

Personally, I'm a little iffy on how necessary it is. Having some code blocks focusable and some not, contextual depending upon things like font size and screen width, feels inconsistent and confusing to a keyboard navigation user—potentially being more detrimental to their experience than having each code block just be focusable.

@querkmachine querkmachine requested a review from a team August 12, 2024 17:49
@querkmachine querkmachine self-assigned this Aug 12, 2024
Copy link

netlify bot commented Aug 12, 2024

You can preview this change here:

Name Link
🔨 Latest commit c9c2da0
🔍 Latest deploy log https://app.netlify.com/sites/govuk-design-system-preview/deploys/66ea9afdf00004000886da88
😎 Deploy Preview https://deploy-preview-4034--govuk-design-system-preview.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.

@querkmachine querkmachine marked this pull request as ready for review August 12, 2024 17:59
Copy link
Contributor

@owenatgov owenatgov left a comment

Choose a reason for hiding this comment

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

One pithy comment. This is looking great and working great 💪🏻

owenatgov
owenatgov previously approved these changes Aug 15, 2024
Copy link
Contributor

@owenatgov owenatgov left a comment

Choose a reason for hiding this comment

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

😎

@owenatgov owenatgov dismissed their stale review August 15, 2024 10:02

Dismissing due to some chat happening outside this PR.

@owenatgov
Copy link
Contributor

Just adding some thoughts on if this is worth doing or not, which I didn't address in my original review.

Here are 2 quotes from @selfthinker on this issue. Firstly on if this is a failure or not:

disagree that this is a WCAG fail, definitely not 2.4.3 as the order is fine, although agree it is confusing when there is nothing to interact with;
suspect it's there due to long lines being cut off (e.g. when zooming in) to make it possible for keyboard only users to scroll through and read the text

specialists agree it's not a WCAG fail, but is a usability issue and a minor accessibility issue

...and if we should accet DAC's proposed solution to completely remove tabindex="0":

no, as removing this would fail 1.4.4, 1.4.10 and 2.1.1

it's worth noting that Firefox make overflowing elements focusable by default for years and Chrome will start doing that soon, probably in version 128

Based on these quotes, I personally think this is fine as an enhancement. The specific points that stand out to me from what Anika's said:

  1. The accessibilty specialists at GDS agee it's a 'minor accessibilty issue', so there's value in providing something to bypass it as a potential problem
  2. Firefox already provides this as a enhancement out of the box

I take your point on this potentially being confusing for keyboard users but I wonder if there's enough info in a given context for users to figure it out ie: they're reading page content and tabbing as they go, they notice an overflowing code block, tab and reach it, realising it's scrollable. I can't corroberate that, it's just a feeling. Maybe we could explore making the scroll controls more prominent or adding something to pompt users that overflowing code blocks are interactable?

@querkmachine querkmachine force-pushed the code-blocks-focus branch 2 times, most recently from affb7fb to cd0c517 Compare August 16, 2024 07:55
@selfthinker
Copy link
Contributor

selfthinker commented Aug 28, 2024

I like that this solution is basically implementing what some browser vendors have already implemented.

We've decided to ask DAC about this solution when we have the followup meeting with them.
They might be able to share with us how their keyboard-only testers would find using this solution.

@selfthinker
Copy link
Contributor

We've had a meeting with DAC today. They like this solution.
They said when something receives focus it should be actionable. And the scrollbar makes it clear that it's actionable as scrolling itself is an action. When it doesn't have a scrollbar (like it is without this change when on a code block that doesn't need it) it's confusing.

I suggest to merge this (after the conflicts are resolved).

Checks if the content of a code block is overflowing or not, if not, it removes the `tabindex` attribute that makes the block focusable
@owenatgov
Copy link
Contributor

One more thing I forgot to add: Once this is merged, I think we're good to write up a final explainer on #4007 and close it. To summarise: this solution doesn't follow DAC's recommendation to the letter but both us and DAC are happy with it so we feel this is done.

@owenatgov owenatgov merged commit 6680415 into main Sep 19, 2024
15 checks passed
@owenatgov owenatgov deleted the code-blocks-focus branch September 19, 2024 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants