Skip to content
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

Conversation

AlexVelezLl
Copy link
Member

@AlexVelezLl AlexVelezLl commented May 9, 2024

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

  • Run Kolibri and check that all places where KTabsList is used continue to work correctly.
    • Coach Reports
    • Coach Groups Reports
    • Coach Lessons Reports
    • Coach Quizzes Reports
    • Coach Learners Reports
    • Coach Plan
    • Create new Quiz Sections Tabs
  • Check that the list of tabs works well in the new Quiz Management
  • Check keyboard navigation
  • Check the responsivenees

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@github-actions github-actions bot added APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) DEV: frontend SIZE: medium labels May 9, 2024
@AlexVelezLl AlexVelezLl requested a review from MisRob May 10, 2024 12:38
@@ -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",
Copy link
Member

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.

Copy link
Member

@nucleogenesis nucleogenesis left a 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());
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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.

@MisRob MisRob requested a review from radinamatic May 15, 2024 04:18
@radinamatic
Copy link
Member

I think I am using the latest asset, but not seeing any tab overflow in Firefox on Ubuntu 20.04... 😕
Chrome seems OK:

Firefox Chrome
no-overflow overflow-chrome

@AlexVelezLl
Copy link
Member Author

Hi @radinamatic. Do you see any error logs in the browser console?

@nucleogenesis
Copy link
Member

@radinamatic @AlexVelezLl I tested this locally and replicated the issue when running the devserver -- both in Chrome and in Firefox.

However, when I ran yarn install --force and restarted my server, the changes worked properly. Seems that the cached KDS was being used rather than the newly updated commit specified in the package.json.

Once I did that, the tabs worked correctly in both Firefox and Chromium

@radinamatic
Copy link
Member

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 ... button with the overflow tabs.

2024-05-21_06-25-33

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 Tab key, and then use Arrow-up and Arrow-down to select other tabs. In my eyes, this is a deal-breaker.

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

@AlexVelezLl
Copy link
Member Author

Thanks @radinamatic!

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 Tab key, and then use Arrow-up and Arrow-down to select other tabs. In my eyes, this is a deal-breaker.

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:

  • When the user is focusing the last element visible in the list and presses Right and/or Up arrows it will automatically open the dropdown menu and focus the first element, and the user can continue navigating with Right/Up arrows.
  • When the user reaches the last element on the dropdown menu and presses Right and/or Up arrows, It will return to the first element visible in the list.
  • The same happens with Left/Down arrows.

This way we have a focus trap between the visible list and the dropdownMenu.

@AlexVelezLl
Copy link
Member Author

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

@radinamatic
Copy link
Member

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 EXE asset to run it with NVDA, thank you! 🙏🏽

@AlexVelezLl
Copy link
Member Author

Hi @radinamatic I have pushed the changes. Can you give it a try please =)

@AlexVelezLl
Copy link
Member Author

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:

[ ] Mark the "currently selected section" w/ a $themeTokens.primary underline if it is in the drop-down menu.

image

@radinamatic
Copy link
Member

As I was afraid, the current implementation breaks the tab pattern: notice how only tabs 1 through 5 are announced as such by NVDA, even though I added 12 sections, and there should, technically, be 12 tabs to navigate through. Sections 6 through 12 are announced as buttons inside the menu, and this disconnect from the rest is totally confusing for someone using the screen reader.

tab-overflow-NVDA

My other concern about the user not being able to distinguish which section is currently selected also still stands. I was not able to see the underline in the overflow menu (not sure if it's actually implemented in this latest asset, tested in Firefox on Windows 10), but even with it, in my judgement we cannot expect the user to keep clicking the button which opens the overflow menu every time just to make sure they are editing the section they want to edit... 😳 There must be a clear visual awareness, and that means the tab title with corresponding focus outline in the foreground, which tells the user where they are at each moment.

@AlexVelezLl
Copy link
Member Author

AlexVelezLl commented May 23, 2024

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:
image

but even with it, in my judgement we cannot expect the user to keep clicking the button which opens the overflow menu every time just to make sure they are editing the section they want to edit... 😳

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.

@nucleogenesis
Copy link
Member

@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 overflow:hidden property? I wouldn't expect it to specifically not read things that are hidden by the overflow property because they're still rendered :-/

I'm not sure the best way to indicate the "current selection" in the drop-down context.

Two thoughts:

  • The selected section in the list has a background color (w/ proper accessible contrast)
  • Perhaps we can scroll to the "currently selected" so that it is always visible upon opening the dropdown

@AlexVelezLl
Copy link
Member Author

Hi @radinamatic! I have pushed some more changes. I tried something different:

  • Now for visible tabs, NVDA should be aware of the correct number of the total number of tabs.
  • For menu items, I have changed the role of them to be "tab", so NVDA was in fact announcing that tab, but it wasnt announcing the "tab X of Y" for these menu items. So, a workaround I found was to add it by myself with aria-roledescription. The bad part of this is that we will need to add this string for translation, but I dont know if this will be compatible with others screen readers, but I havent found any other workarounds yet. If we dont want this, the best option I would find is just remove the aria-roledescription and stay with the screen reader just saying the tab name without the "X of Y"
  • I have tried with aria-labelledby and aria-owns but without sucess.

@nucleogenesis No, the reason why NVDA is not recognizing them is because they are visibility: hidden. A way I found to keep them invisible, but NVDA being able to recognize them is to set its opacity: 0 and set its z-index: -1 so it is not visible and unclickable.

@radinamatic
Copy link
Member

@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.

tab-overflow-NVDA-new

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 < and > buttons on the sides for the mouse and touch users, and the keyboard users will be able to freely navigate with the known arrow keys interaction through the uninterrupted list of tabs to navigate through.

sliding-tab-list

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! 💯 🌟

@AlexVelezLl
Copy link
Member Author

Oh sliders definitely sound much better for overflowed tabs haha. All right on, Ill close related PRs. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) DEV: frontend SIZE: medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Quiz Creation: Replace TabsWithOverflow with KListWithOverflow
4 participants