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
Update KTabsList to support the new overflow handling #12146
Update KTabsList to support the new overflow handling #12146
Conversation
Build Artifacts
|
kolibri/core/package.json
Outdated
@@ -21,7 +21,7 @@ | |||
"js-cookie": "^3.0.5", | |||
"knuth-shuffle-seeded": "^1.0.6", | |||
"kolibri-constants": "0.2.5", | |||
"kolibri-design-system": "4.2.0", | |||
"kolibri-design-system": "https://github.com/AlexVelezLl/kolibri-design-system.git#110b3ecb909aed2015b3cfdf66ed501fb442447e", |
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.
Note that we can merge this PR after a new KDS release is out and this reference will need to be changed back to the npm package before merging. Thank you for preparing it for testing! Just leaving the note here so we don't forget when the time comes.
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.
Looks good on manual testing. I think if I'm understanding the purpose behind adding the setTimeout
it might be worth trying it w/ nextTick
since we use that fairly commonly in the code base -- although I may be missing something there for sure.
With an answer to that question and fixes to the conflicts this should be good to merge!
@@ -551,7 +513,8 @@ | |||
'createSnackbar', | |||
this.sectionDeletedNotification$({ section_title }) | |||
); | |||
this.focusActiveSectionTab(); | |||
// Run after the focus change of KModal destroyed method | |||
setTimeout(() => this.focusActiveSectionTab()); |
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.
Is this doing the same kind of thing you'd do with Vue.$nextTick
?
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 tried even with 4+ consecutive $nextTick calls and all of them were beign called before the setTimeout that excecutes this focus on KModal destroyed mehod :this-is-fine:. Since there we use setTimeout, we need to use setTimeout also here, for the focusActiveSectionTab method to be enqueued after the lastFocus.focus() in KModal.
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.
Ah right on! Thanks for the explanation.
Hi @radinamatic. Do you see any error logs in the browser console? |
@radinamatic @AlexVelezLl I tested this locally and replicated the issue when running the devserver -- both in Chrome and in Firefox. However, when I ran Once I did that, the tabs worked correctly in both Firefox and Chromium |
Not sure what exactly was causing the issue the first time, but when I run the testing VM again today, where the initial asset was installed (since it has not changed in the meantime), this time I do see the However, I have serious concerns regarding the user experience of the current implementation that go beyond the keyboard-navigation. The most important is that it breaks the expected pattern of arrow navigation between tabs. The set that is visible (sections 1 to 6 in the above screenshot) is navigable with arrow left-right keys, but if there are overflow tabs, the only way for user to access the rest of them is to know to switch to Second very relevant UX concern (cc @tomiwaoLE) is that user is not aware which overflow-ed section (tab) they are editing (adding questions or adjusting sections). Look at the above screenshot or the screencast below, how would the user know which section is the current since nothing in the UI seems to change (and we're not even talking keyboard-navigation)? 😕 which-section.mp4 |
Thanks @radinamatic!
What could we do in that case? Would it be ok if we automatically focus the first option of the dropdown menu? Even if we do that we will need to support left/right arrow navigation on the DropdownMenu. Or support Up/Down arrow navigation on the tab list (or both!). So we could have:
This way we have a focus trap between the visible list and the dropdownMenu. |
Hi @radinamatic. This is a proposal to enhance the keyboard navigation (I havent added yet the support to navigate with all left/right/up/down arrows for both components, so in this example I chnge from left/right to up/down, but it can be implemented if it makes sense): Compartir.pantalla.-.2024-05-21.09_53_23.mp4 |
Interaction from the recording looks promising @AlexVelezLl 🙂 , but I'll have to re-test it with the screen reader to confirm that the tabs are still announced properly and in the uninterrupted manner. Let me know when you push the changes so I can grab the |
c00eeef
to
d36b452
Compare
Hi @radinamatic I have pushed the changes. Can you give it a try please =) |
d36b452
to
2c9c193
Compare
As I noticed that this PR will also close #12011, I updated it to meet the second criteria where stated how we can visualize selected tabs that are overflowed:
|
Hi @radinamatic. Do you have any suggestions or any clue about the path we can take to solve the issue? I found it quite difficult to achieve what you mentioned, because the position in the DOM of both the tabs list and the menu are very very far away, and I dont know if there is a way of "interpreting" that the menu items are part of the tabs list. I'm trying to see what can be done, but if you've seen something in your :a11y: experience, or if you've seen this case of "overflowed tabs" somewhere else, some suggestions would be super great. Also, I downloaded the lastest .pex asset and I am able to see the selected section in the menu:
There were some thoughts about that and to try to "reorder", prioritize and show the selected items in an overflowed list, but we will keep this as: Section 1 | Section 2 | Section 3 | Section 12 ( More ) But it can be also confusing, because the user could think that Section 12 will be the 4th to be shown in the exam, but its not, its just reordered to show that it is selected But I think we can create a follow up issue for the visualization of the selected items that are overflowed, because we will probably need to discuss it well, and it will probably take a while. |
@AlexVelezLl is the newer implementation of the overflow handling not rendering the overflow-ing items or are they not being read out by NVDA because they're visually hidden due to the CSS I'm not sure the best way to indicate the "current selection" in the drop-down context. Two thoughts:
|
2c9c193
to
9512385
Compare
Hi @radinamatic! I have pushed some more changes. I tried something different:
@nucleogenesis No, the reason why NVDA is not recognizing them is because they are |
@AlexVelezLl Thank you for investigating this and trying out every possible solution, the effort and persistence are really appreciated! 🤗 I tested the latest asset, and while this time you indeed managed to make it so NVDA would recognize and announce all created tabs (Section 3 of 11 instead of Section 3 of 5 with only 5 visible tabs), there is still a difference in which they are announced that I'm afraid will be confusing. While I did try to research and find the recommended accessible approach for this type of interaction, that was unsuccessful, mostly because we are in the essence trying to implement a new and not standard interaction pattern (and tabbed interfaces are not even recommended in case of many tabs or distinct sections to present to users). We've also had a lengthy conversation with @marcellamaki and @tomiwaoLE on how to approach the concern that it is unclear which tab is currently active and selected when it is inside overflow menu, and the conclusion we arrived at is that the overflow menu interaction is not the best choice. I regret sincerely that we did not realize this before you invested so much effort into implementing this component across all the tabs in the Coach pages 😞 @tomiwaoLE sketched a couple of solutions that in case of the overflow would use the slider paradigm that would have Tomiwa will certainly present a much better looking one than the above 🙂 Again, thank you so much for trying to fit this to serve our needs, I hope the discussions here stay with all of us as a valuable lesson! 💯 🌟 |
Oh sliders definitely sound much better for overflowed tabs haha. All right on, Ill close related PRs. Thanks! |
Summary
This is the corresponding pr to introduce the new KTabsList changes from learningequality/kolibri-design-system#635.
This PR replaces the existing
TabsWithOverflow
with KTabsList.Compartir.pantalla.-.2024-05-09.10_25_55.mp4
Compartir.pantalla.-.2024-05-09.10_28_33.mp4
References
Closes #12011
Reviewer guidance
Testing checklist
PR process
Reviewer checklist
yarn
andpip
)