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

[DO NOT MERGE] Test new KDS tabs components in Coach Reports #10109

Closed
wants to merge 2 commits into from

Conversation

MisRob
Copy link
Member

@MisRob MisRob commented Feb 13, 2023

@github-actions github-actions bot added APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) SIZE: large labels Feb 13, 2023
@MisRob MisRob requested a review from radinamatic February 13, 2023 17:12
@pcenov
Copy link
Member

pcenov commented Feb 15, 2023

Hi @radinamatic and @MisRob, I was able to identify the following differences with latest develop:

  1. The gray line under the tabs is no longer being displayed
  2. There is less space between the text 'View reports for your learners and class materials' and the tabs
  3. When navigating by using the keyboard the tab order is not the same, skips from 'Lessons' to the 'Status' drop-down.

Here's a short video comparison:

2023-02-15_17-07-06.mp4

@radinamatic
Copy link
Member

When navigating by using the keyboard the tab order is not the same, skips from 'Lessons' to the 'Status' drop-down.

That is what is supposed to happen, we had it not properly coded until now 😉
You can use arrow keys to navigate between tabs to select and open another one.

@MisRob
Copy link
Member Author

MisRob commented Feb 15, 2023

Thank you, @pcenov, I will fix visual regressions (1) and (2). Regarding (3), just to add to what @radinamatic has mentioned already, here's the expected navigation by keyboard described in more detail https://www.w3.org/WAI/ARIA/apg/patterns/tabs/ ("Keyboard Interaction" section) if that'd be helpful.

@radinamatic
Copy link
Member

radinamatic commented Feb 16, 2023

@MisRob So far it's looking very good, even though I haven't finished testing all the nooks and crannies 😉

One thing I noticed is that when user has finished tabbing through the focusable elements in one panel, the next Tab press takes them outside the tablist, and since in our case there's nothing else below in the page, it returns the focus to the browser tab 🤔

tabs-exit

For some reason I was under the impression that they should move to the next tab, but there's no prescribed flow in the pattern specs you cited above, although I must admit the phrases like this do not help me much understand what is actually recommended... 😒

When the tab list contains the focus, moves focus to the next element in the page tab sequence outside the tablist, which is the tabpanel unless the first element containing meaningful content inside the tabpanel is focusable.

@MisRob
Copy link
Member Author

MisRob commented Feb 22, 2023

Thank you @radinamatic

I see two things in your previous comment

(1) "when user has finished tabbing through the focusable elements in one panel, the next Tab press takes them outside the tablist"

I too didn't notice any guidelines in regard to this behavior. I think you have a better grasp on what would feel more intuitive to keyboard users so feel free to suggest different behavior and I will implement it if you think it'd be better.

Regarding

(2)

When the tab list contains the focus, moves focus to the next element in the page tab sequence outside the tablist, which is the tabpanel unless the first element containing meaningful content inside the tabpanel is focusable.

My understanding is that this is not related to (1), but rather to another similar note from this page

When the tabpanel does not contain any focusable elements or the first element with content is not focusable, the tabpanel should set tabindex="0" to include it in the tab sequence of the page.

This, to me, seems to describe two scenarios:
(A) the tabpanel <div> contains a focusable element, e.g. <a> link
(B) the tabpanel <div> doesn't contain any focusable element, e.g. there's just <p> paragraph

Now, let's say the focus is on the tab list. When I click TAB, in (A) the focus moves to <a>, whereas in (B) it moves to the tabpanel <div> (thanks to it having tabindex="0" in this particular case)

This is how I implemented it, however, it's true that some wording in the guidelines in this regard is indeed confusing. Please let me know if this behavior makes sense to you.

@MisRob
Copy link
Member Author

MisRob commented Feb 22, 2023

@radinamatic Besides the conversation from the comment above, do you have any other notes?

@radinamatic
Copy link
Member

This is how I implemented it, however, it's true that some wording in the guidelines in this regard is indeed confusing.

@MisRob We are in agreement around the interpretation, and especially about the above confusion...

After digging in the code and checking roles, states and keyboard navigation, I indeed have nothing to remark: you implemented all as per the linked specs, thank you! 🙏🏽 👏🏽

I'm still left with the doubt if the tabbing out of one panel (because there are no more focusable elements in it) which leads the user completely outside of the tabbed interface section (instead of maybe to the following tab in the tablist) is the best user experience, but after a lot of searching I failed to find any recommendation regarding this point. Since is not our place to introduce and establish new things, let's leave it as is.

One last thing that I'm not completely certain of is if we should communicate to the user that there is a page URL and thus the title change when they navigate from one tab to another. Not as much for the keyboard navigation, as the focus is rightfully placed to the newly loaded selected tab/panel, but just for the sake of complete information given to the screenreader user who cannot see that the new page has been loaded. I wonder if it has any value to append the new page title to communicate that...? 🤔

@MisRob
Copy link
Member Author

MisRob commented Feb 23, 2023

Thanks, @radinamatic.

One last thing that I'm not completely certain of is if we should communicate to the user that there is a page URL and thus the title change when they navigate from one tab to another. Not as much for the keyboard navigation, as the focus is rightfully placed to the newly loaded selected tab/panel, but just for the sake of complete information given to the screenreader user who cannot see that the new page has been loaded. I wonder if it has any value to append the new page title to communicate that...?

By this, do you mean adding some kind of visually hidden information? If you could post an example, that'd be helpful, I'm not sure if I can understand correctly.

@radinamatic
Copy link
Member

This is a sample of the NVDA speech output:

Coach reports  tab control
GROUPS  tab  selected  3 of 4
LEARNERS  tab  4 of 4
Coach reports  tab control
LEARNERS  tab  selected  4 of 4

Broken down by keyboard steps:

  1. User arrow-navigated to the Groups tab and pressed the Enter key, NVDA read the tablist label and announced the label of the selected/currently active tab (Groups).
Coach reports  tab control
GROUPS  tab  selected  3 of 4
  1. User then arrow-navigated to the Learners tab, NVDA read it as the tab in focus.
LEARNERS  tab  4 of 4
  1. User pressed the Enter key, NVDA read the tablist label and announced the label of the selected/currently active tab (Learners).
Coach reports  tab control
LEARNERS  tab  selected  4 of 4

Before both steps 1 and 3 there was the URL and page title change, namely:

Step 1.
URL: /coach/#/.../reports/groups
Page title: Groups - < class-name > - Kolibri document

Step 3.
URL: /coach/#/.../reports/learners
Page title: Learners - < class-name > - Kolibri document

For a moment I was considering whether these changes should be communicated to the user, but then realized that it would be way too verbose to hear it after each tab change, so I'm taking it back 🙂

Good to go! 🎉

Copy link
Member

@radinamatic radinamatic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And we have the proper, accessible tab experience in Kolibri! 🎉 🎉 🎉

Excellent job, @MisRob, thank you!!! 🥇 💯 :shipit:

@MisRob
Copy link
Member Author

MisRob commented Feb 24, 2023

Thank you @radinamatic, in any case, thank you for the more detailed explanation of NVDA outputs, it's been helpful for me to understand better what's happening.

@MisRob
Copy link
Member Author

MisRob commented Feb 24, 2023

I'm closing this since it was only for testing. As soon as we merge learningequality/kolibri-design-system#420, I will open a new PR to Kolibri with the correct KDS reference and both Coach tabs updated to use new KDS components. On that opportunity, I will also address @pcenov's feedback

  1. The gray line under the tabs is no longer being displayed
  2. There is less space between the text 'View reports for your learners and class materials' and the tabs

for this is matter of Kolibri code rather than that of KDS.

@MisRob MisRob closed this Feb 24, 2023
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.) SIZE: large
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants