Make commit reordering keyboard accessible#17671
Conversation
…ms in a flat list
It was too annoying specially with screen readers 😞
For some reason, NVDA doesn't like "optional rendering" for this element, otherwise it doesn't read the message when the element is added
niik
left a comment
There was a problem hiding this comment.
Gave this a quick first look. I'll take another look tomorrow but here are some mostly nitpicky requests for changes. Overall I'm pumped about this!
There was a problem hiding this comment.
I tested it on macOS and Windows with and without screen readers. It works so seamlessly. :)
I think your tooltip/popover idea for showing visual instructions to our non screenreader and particularly mouse users would be a great iteration to add in another PR. :)
I was thinking maybe the tooltip/popover could also have a hint about drag/drop for our mouse users? That way we don't enable mouse placement in the keyboard mode and muddy the keyboard experience up for keyboard only/screen reader users.
I have thought about that for cherry-picking and squashing too.. Since drag and drop is so undiscoverable, it might be nice to have some sort of call out in the dialogs when it is invoked by the context menu. We have that sort of thing in the let's get started page for being able to drag repos onto the app.. tho we have much more real estate there for that.
Co-authored-by: Markus Olsson <j.markus.olsson@gmail.com>
Co-authored-by: Markus Olsson <j.markus.olsson@gmail.com>
Co-authored-by: Markus Olsson <j.markus.olsson@gmail.com>
Co-Authored-By: Markus Olsson <634063+niik@users.noreply.github.com>
|
In the end I decided to add the tooltip/popover with the hint to this PR, I think it's a nice addition and it wasn't too hard to implement:
|
tidy-dev
left a comment
There was a problem hiding this comment.
This works great! ✨ Thank you for all your hard work on this!
We have chatted in slack and team sync about iterating on the popover design to include keyboard shortcut visuals and that it needs to not be a role of dialog since it is not something to interact with/grab the users focus. When the final iteration is done, be good to run by accessibility design to make sure there isn't any other gotcha with this since I would call it custom ui - not an established pattern.
Just chatted with Markus and he's happy to merge this PR after having @tidy-dev's approval!
This comment was marked as spam.
This comment was marked as spam.
|
@sergiou87 Thanks a lot for this. |
xref. https://github.com/github/accessibility-audits/issues/5715
Description
This PR makes changes to our flat lists to support reordering commits using the keyboard. Now users can reorder a commit by right-click on the commits they want to move in the history, and then clicking on
Reorder commitsin the context menu presented.After that, the list will enter in a "keyboard insertion mode" where they can use the up and down arrow keys to decide where the commits will be moved to, and then press Enter to confirm the operation (or Escape to cancel it).
Alternatively, while in this mode users can also use their mouse to click on the position where they want their commits.The ability to use the mouse in this mode was removed because it felt too annoying, specially with screen readers.Other improvements to implement in different PRs:
Screenshots
CleanShot.2023-11-07.at.13.17.49.mp4
CleanShot.2023-11-07.at.13.20.02.mp4
Release notes
Notes: [Improved] Reordering commits is now keyboard accessible