-
-
Notifications
You must be signed in to change notification settings - Fork 127
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
[accessibility] Uses the arrow keys for tab bar navigation #612
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.
Thanks a lot @brichet; I have some questions:
I guess it closes #581, isn't it?
I tested turning on titlesEditable
and this leaves to a conflict. If the user is focused in the input field to edit the title, pressing the arrow actually move the focus to the surrounding tab instead of moving within the input field.
And it is not possible to click on the input field to place the cursor within the existing title string. It basically set the focus on the tab panel - I don't know if this is a consequence of this PR but if you could fix it 😉
I'm wondering how some part of the code behaves if the option allowDeselect
(that actually does not make sense for me...) is true.
I think the two later cases deserve unit tests.
Finally should we implement Home
and End
. That sounds like a simple and good addition.
Delete
could be a good addition in the future. But as the shortcut to close a Tab in JLab is not that one, this will be require more discussion.
event.preventDefault(); | ||
event.stopPropagation(); |
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.
Should the event being stopped even if there is only one element?
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 thinks so, to avoid different behavior depending on the number of elements.
packages/widgets/src/tabbar.ts
Outdated
}; | ||
if (data.tabIndex !== undefined) { | ||
ariaAttributes.tabindex = `${data.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.
In case this it is undefined should it be set to some default value?
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.
Having tabindex="-1"
as default would probably make sense for a tab.
I'm not quite sure about the difference between tabindex="-1"
and tabindex not set.
It seems that the behavior is not the same for input elements and other elements.
In the case of the tab, not setting the tabindex
would make it not focusable with tabulation, nor 'programatically'. I'm not sure how useful it can be...
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'm not quite sure about the difference between
tabindex="-1"
and tabindex not set.
It seems that the behavior is not the same for input elements and other elements.
when tabindex
is not defined, interactive elements automatically receive tab focus, while non-interactive elements don't. so if you wan't to avoid a tab on an interactive widget you'd use tabindex=-1
. if you want to add a non-interactive dom element to the tab order then you'd use tabindex=0
.
the ARIA authoring guide example for this pattern uses button
s for the tab element which means they need control what doesn't get tabbed to; the implementation needs to choose one tab element to have tabindex=0
, or undefined, and the rest are tabindex=-1
. if the tab elements are made of non-interactive elements then the task is to make elements appear in the dom order by adding tabindex=0
to one element.
In the case of the tab, not setting the
tabindex
would make it not focusable with tabulation, nor 'programatically'. I'm not sure how useful it can be...
hopefully folks don't mess with tab order. that should be private. most recommendations say to avoid tabindex>0
.
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.
fixed in 9d395d8
|
Co-authored-by: Frédéric Collonval <fcollonval@users.noreply.github.com>
Co-authored-by: Frédéric Collonval <fcollonval@users.noreply.github.com>
Co-authored-by: Frédéric Collonval <fcollonval@users.noreply.github.com>
Thanks @fcollonval for reviewing it.
I believe so
Fixed in 2bb3466 |
Yes we can implement it easily I think. The Done ✔️ |
I think it should be in a follow up PR. |
Seems to have no effect on the focus |
APG tab pattern marks Home, End, and Delete as optional interactions; there expected behaviors are explained. there is no mention of Shift + Delete to restore a deleted tab, but i think that would be most consistent. |
this issue resolves some concerns i presented in jupyter/notebook#6935 (comment) |
thanks for working on this @brichet it will save a lot of work for keyboard users! is there a way i can test the actual experience from the pull request on github or do i have to build locally? |
The only way I know is to build locally (lumino), link the package in Jupyterlab ( |
Thanks for your feedback. |
i'll try to get a dev environment up to test this. don't know if i can give a thorough review with out testing locally. i'll do my best to find some time. |
@tonyfast there is an indirect easy way to test this using the doc built on the PR: https://lumino--612.org.readthedocs.build/en/612/examples/dockpanel/index.html It contains a dockpanel example that you can use for testing the changes of this PR. |
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.
Thanks @brichet
I will leave it open to give Tony the time to test.
Would you mind opening a follow-up issue because in Firefox (version 114 on Debian 12) when editing a title, Home, End and click do not work for me? But they do on chromium (version 114).
thanks for dropping that test link. here are the things i tested:
i'm finding two issues:
a personal preference would be automatic tab activation rather than manual activation. is this a switch the component could have? this way when i arrow to a tab i see the contents. automatic tab activation would reduce the work a keyboard user would have to do to explore their content. |
Thanks @tonyfast for testing. I'll merge this one as is as it matches the PR description and to open follow-up issues for
|
This PR follows #583 and #590 to use the arrow keys for tab bar navigation.
After this PR:
By default the tab bar elements have
tabindex="-1"
, except for the current one (or the first one) which hastabindex="0"
.When a tab is focused, the arrow keys are used to change the focused tab (left/right for horizontal tab bars, up/down for vertical ones).
Pressing the tabulation again moves the focus to an element other than the tab bar.
Closes #581