Make resize vertical splitter keyboard accessible

[pksjce]

Closeshttps://github.com/github/primer/issues/2810

Context

Earlier guidance for SplitPageLayout accessibility was to create a visually hidden form with range input to determine the position of the splitter. However this hidden form swallows the focus when tabbing through the page layout.

New

The proposed change here is to follow Window Splitter a11y guidelines and support keyboard navigation on the vertical splitter.

Screen.Recording.2023-11-03.at.8.38.29.pm.mov

Rollout strategy

• Patch release
• Minor release
• Major release; if selected, include a written rollout or migration plan

Testing & Reviewing

Tab through the Resizable Pane storybook under PageLayout.features.stories and navigate to focus on the vertical splitter. Use the left and right arrow keys to move it sideways. Use Voiceover to here the announcement on current pane width updated with every move.

Merge checklist

• Added/updated tests
• Added/updated documentation
• Added/updated previews (Storybook)
• Changes are SSR compatible
• Tested in Chrome
• Tested in Firefox
• Tested in Safari
• Tested in Edge
• Integration tests pass at github/github (Learn more about how to run integration tests)

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

[changeset-bot]

🦋 Changeset detected

Latest commit: 595fd42

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[github-actions]

size-limit report 📦

Path	Size
dist/browser.esm.js	104.34 KB (-0.03% 🔽)
dist/browser.umd.js	104.87 KB (-0.04% 🔽)

[siddharthkp]

Hi!

(Probably an old bug that's only surfacing now) Ran into a strange issue where the position would move more and more every keystroke and found that handleKeyDragEnd is running more and more times with every keystroke 🤔

notice the count for console.log:

events-not-detached.mov

[siddharthkp]

Implementation looks great!

Found one strange bug, other than that, looks good!

[pksjce]

Hi @siddharthkp - Thanks for flagging that! I noticed this bug too. The fix was to remove the event listeners in the useEffect's return function. Seems to be more stable now 👍

[siddharthkp]

Seems to be more stable now 👍

Yes, feels much better now!

[siddharthkp]

Optional side note 1: We change the cursor when dragging with pointer, should we also change it for keyboard or not? 🤔

Feels a tiny bit janky based on your keystrokes, but maybe it's fine. Leaving it up to you

resize-cursor.mov

Optional side note 2: Not for this PR, but a future optimisation that would be nice is to make the resizing incrementally bigger the longer you press an arrow key (and then reset it after cool off period)

[siddharthkp]

Left a few optional comments, approving from my side ✅

Pending:

• add changeset
• check with accessibility team

[pksjce]

Optional side note 1: We change the cursor when dragging with pointer, should we also change it for keyboard or not? 🤔

Feels a tiny bit janky based on your keystrokes, but maybe it's fine. Leaving it up to you

So I thought about this but imagined that if somebody is using only keyboard, they wouldn't use the mouse at all (which I believe gives that feeling.So they wouldn't see a mouse pointer? I'm not sure how usage for this will be like.

Not for this PR, but a future optimisation that would be nice is to make the resizing incrementally bigger the longer you press an arrow key (and then reset it after cool off period)

Great idea! I was kinda bored while testing this and this addition would be perfect. It would be fun to implement. Do you think it might have any accessibility penalty?

Eric is on leave, whom would you recommend I tag for accessibility review?

[siddharthkp]

So I thought about this but imagined that if somebody is using only keyboard, they wouldn't use the mouse at all (which I believe gives that feeling.So they wouldn't see a mouse pointer? I'm not sure how usage for this will be like.

Ah yes, good point. I genuinely don't know if that's the case or not 😇

Eric is on leave, whom would you recommend I tag for accessibility review?

@ichelsea or @TylerJDev maybe?

[ichelsea]

Thanks for looking for ways to continuously improve our components!

I'm not finding any issues with using the splitter with just my keyboard (and I'm also behind the idea to leave the cursor change alone when controlling with just the keyboard).

However, I'm unable to control this splitter component with VoiceOver on Safari or NVDA on Edge. It's read out to me, but the arrow keys just move me to the next element and there isn't an intuitive way to figure out how to use the control with a screen reader. I'm unsure of advice to give on this one as it dips into a more technical area that I'm unfamiliar with, so I'm going to tag in our accessibility engineering team to have a look while Eric is out 🙂.

[kendallgassner]

@ichelsea What you are noticing is that the screen reader uses certain keys, in this case arrow keys, to interact with the virtual buffer. Essentially, the screen reader uses the arrow keys to move around the accessibility tree and not passing those key events back to the javascript.

⚠️ I do not advise trying to override how the screen reader operates and therefore I do not advise using something like role=application. role=application should be used sparingly and this does not seem like the right use case. Instead, we will likely need to provide a different design, but I think this is a topic that should be discussed in office hours. We had a similar issue in drag and drop so I would love to be a part of the discussion.

And we should involve Tetralogical in this one 🤔 .

Link to reading: Time to revisit accesskey?

[pksjce]

Taking this comment as confirmation to merge this.
https://github.com/github/accessibility/issues/5101#issuecomment-1824232088

@pksjce perfect. confirming that the latest change (with the doubled-up arrow key behaviour and removal of aria-orientation) works great in all tested browser/SR combinations