-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
feat(web): navigate with keyboard on person page #5486
Conversation
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.
Can you use svelte:document instead please?
web/src/lib/components/faces-page/merge-suggestion-modal.svelte
Outdated
Show resolved
Hide resolved
if (!$showAssetViewer) { | ||
event.stopPropagation(); | ||
switch (event.key) { | ||
case 'Tab': |
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.
Tabbing through focusable elements is something that is built into the browser. We don't need to re-invent the wheel here.
https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/tabindex
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.
Yeah I know but I couldn't find a way to limit the focus to the suggested people only.
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.
tabindex=-1 tells the browser to skip it.
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.
Yeah I tried it but the issue is that you want to navigate only through the suggested people, not all the buttons on the page. Do you have any ideas 🤔 ?
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.
Does using an actual browser model still let you focus on background elements?
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.
What's an actual browser model ?
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.
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.
Mmmh, just tried it, does it work only for dialogs ?
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.
I don't think we want dialogs to display suggested people 😅
I think it is fine to add keyboard shortcuts, but we should do it in a way that doesn't make the site less accessible (which is the outcome of overriding the tab behavior or preventing other default key behaviors) |
I agree 100% with you. But in this case we want to navigate with the tabs and arrow keys which is a little unusual. I don't know what's the good way for that case 😅 |
Can you just use arrow keys and enter instead? |
Deploying with Cloudflare Pages
|
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.
LGTM
Changes made in this PR
With this PR, users can navigate through suggested people with Tab and Arrow keys. On the suggestion merge modal, pressing the Tab key will only switch between the
No
andYes
buttons.Resolves #5400 and #5398