-
Notifications
You must be signed in to change notification settings - Fork 247
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
Conversation
✅ You can preview this change here:
To edit notification comments on pull requests, go to your Netlify site configuration. |
125cd33
to
6b69f49
Compare
There was a problem hiding this 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 💪🏻
6b69f49
to
883e443
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😎
Dismissing due to some chat happening outside this PR.
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:
...and if we should accet DAC's proposed solution to completely remove
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:
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? |
affb7fb
to
cd0c517
Compare
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. |
We've had a meeting with DAC today. They like this solution. I suggest to merge this (after the conflicts are resolved). |
cd0c517
to
40f902a
Compare
Checks if the content of a code block is overflowing or not, if not, it removes the `tabindex` attribute that makes the block focusable
40f902a
to
c9c2da0
Compare
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. |
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. #4007We 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
data-module
attribute to code blocks processed by Markdown.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.