Fixed tab index order for tab press #755

zubairshakoorarbisoft · 2025-03-13T10:06:44Z

Description

On the Posts page, the keyboard focus was restricted to only the pre-selected option within the filter, and it was not moving to other non-selected options.

Steps to reproduce:

Open the page: [Posts Page](https://apps.sandbox.openedx.edly.io/discussions/course-v1:OpenedX+DemoX+DemoCourse/posts)
Select either "My Posts", "All Posts", or "Learners".
Access the filter icon.
Notice that the keyboard focus remains on the pre-selected option and does not move to other non-selected options.

Related to overhangio/tutor-indigo#146

How Has This Been Tested?

I set up Tutor locally with the Indigo Plugin enabled (which is enabled by default). I followed the above steps to reproduce the issue, inspected the HTML on the frontend, and identified where the adjustment was needed. After applying the fix, the keyboard now correctly navigates through both selected and non-selected options using the tab key.

Screenshots/sandbox (optional):

Sandbox Link
Taiga Ticket (If someone have access)

Screenshots

Here is the screen where only selected options were keyboard accessible, but after making the fix it should be accessible for all the options.

Merge Checklist

If your update includes visual changes, have they been reviewed by a designer? Send them a link to the Sandbox, if applicable.
Is there adequate test coverage for your changes?

Post-merge Checklist

Deploy the changes to prod after verifying on stage or ask @openedx/edx-infinity to do it.
🎉 🙌 Celebrate! Thanks for your contribution.

brian-smith-tcril

Thanks for looking into this! I left a comment with a small question.

brian-smith-tcril · 2025-05-20T16:58:20Z

src/discussions/learners/learner/LearnerFilterBar.jsx

@@ -28,10 +28,10 @@ const ActionItem = React.memo(({
    style={{ cursor: 'pointer' }}
    aria-checked={value === selected}
    // eslint-disable-next-line jsx-a11y/no-noninteractive-tabindex
-    tabIndex={value === selected ? '0' : '-1'}
+    tabIndex={'0'}


I checked the git blame and it looks like this line was added for #483

Can you confirm the issue that PR was fixing is still fixed with this change?

Yes, I’ve verified that the issue addressed in PR #483 is still resolved with the current change.
Thanks @brian-smith-tcril for pointing it out and for your thorough review!

You should not have a tabindex on a Label, because it is a non-interactive role. This will throw a new error in automated testing.

open-craft-grove · 2025-05-21T00:39:45Z

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

awais-ansari · 2025-05-21T08:02:58Z

The required checks are failing. Other than that, the changes look good to me.

brian-smith-tcril

These changes do feel like an improvement to me compared to what is currently on master.

I did notice one thing though.

In my testing I wasn't able to select filters using the keyboard (pressing space or enter when a filter was focused did nothing). I think the inability to focus on non-selected filters on master is hiding the inability to interact with the filters using the keyboard. Since the filters behave similarly to radio buttons, having nothing happen when pressing space or enter on a focused filter when it is selected is "correct" behavior.

wittjeff

I can give you more precise recommendations if you want to discuss.

wittjeff · 2025-05-27T12:44:20Z

src/discussions/learners/learner/LearnerFilterBar.jsx

@@ -28,10 +28,10 @@ const ActionItem = React.memo(({
    style={{ cursor: 'pointer' }}
    aria-checked={value === selected}
    // eslint-disable-next-line jsx-a11y/no-noninteractive-tabindex
-    tabIndex={value === selected ? '0' : '-1'}
+    tabIndex={'0'}


You should not have a tabindex on a Label, because it is a non-interactive role. This will throw a new error in automated testing.

wittjeff · 2025-05-27T12:47:29Z

src/discussions/learners/learner/LearnerFilterBar.jsx

  >
    <Icon src={Check} className={classNames('text-success mr-2', { invisible: value !== selected })} />
-    <Form.Radio id={id} className="sr-only sr-only-focusable" value={value} tabIndex="0">
+    <Form.Radio id={id} className="sr-only sr-only-focusable" value={value} tabIndex="-1">


You should not have a tabindex on a input type="radio". The radiogroup will take focus as a single tab stop, and the selection within the group may be changed via arrow keys (assuming all items have the same name attribute ).

If you want to wrap a label with an icon over a sr-only input which will take focus (which is not the technique used by the Paragon radio buttons and checkboxes, btw), you can use css::focuswithin on the wrapping Label. to change the visible focus styling.

wittjeff · 2025-05-27T12:50:10Z

src/discussions/posts/post-filter-bar/PostFilterBar.jsx

@@ -42,10 +42,10 @@ export const ActionItem = React.memo(({
    style={{ cursor: 'pointer' }}
    aria-checked={value === selected}


A label should never have an aria-checked attribute. The Form.Radio should have a regular checked value when appropriate.

wittjeff · 2025-05-27T12:58:04Z

The sandbox link does not show the associated UI. I'll give better feedback if you can show it live.

brian-smith-tcril · 2025-05-27T13:40:08Z

The sandbox link does not show the associated UI. I'll give better feedback if you can show it live.

https://app.pr-755-220000.sandboxes.opencraft.hosting/discussions/course-v1:demo+Course+1/posts and click

brian-smith-tcril · 2025-05-27T13:46:33Z

@wittjeff The reason I approved this PR is not because it is perfect from an a11y stand point, but because it is a marked improvement over the existing behavior on `master.

In the following screen recordings I am hitting tab repeatedly until I get to the end, and then hitting shift+tab. Using the arrow keys to select options does not work on either master or this PR.

`master`

Screencast.From.2025-05-27.09-41-43.mp4

This PR

Screencast.From.2025-05-27.09-42-05.mp4

wittjeff · 2025-05-27T14:44:37Z

The sandbox link does not show the associated UI. I'll give better feedback if you can show it live.

https://app.pr-755-220000.sandboxes.opencraft.hosting/discussions/course-v1:demo+Course+1/posts and click

I found it, thanks!

wittjeff · 2025-05-27T14:45:30Z

@wittjeff The reason I approved this PR is not because it is perfect from an a11y stand point, but because it is a marked improvement over the existing behavior on `master.

In the following screen recordings I am hitting tab repeatedly until I get to the end, and then hitting shift+tab. Using the arrow keys to select options does not work on either master or this PR.

master

Screencast.From.2025-05-27.09-41-43.mp4

This PR

Screencast.From.2025-05-27.09-42-05.mp4

It seems like this would be better implemented as a variation on the existing Paragon radio group, with a custom Icon used to indicated checked state.

fixed tab index for tab press

9a84415

zubairshakoorarbisoft mentioned this pull request May 7, 2025

[EPIC] Accessibility compliance overhangio/tutor-indigo#146

Open

brian-smith-tcril reviewed May 20, 2025

View reviewed changes

brian-smith-tcril added the create-sandbox open-craft-grove should create a sandbox environment from this PR label May 20, 2025

brian-smith-tcril approved these changes May 21, 2025

View reviewed changes

awais-ansari self-requested a review May 26, 2025 06:45

wittjeff suggested changes May 27, 2025

View reviewed changes

		@@ -42,10 +42,10 @@ export const ActionItem = React.memo(({
		style={{ cursor: 'pointer' }}
		aria-checked={value === selected}

Fixed tab index order for tab press #755

Are you sure you want to change the base?

Fixed tab index order for tab press #755

Conversation

zubairshakoorarbisoft commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How Has This Been Tested?

Screenshots/sandbox (optional):

Screenshots

Merge Checklist

Post-merge Checklist

Uh oh!

brian-smith-tcril left a comment

Choose a reason for hiding this comment

Uh oh!

brian-smith-tcril May 20, 2025

Choose a reason for hiding this comment

Uh oh!

zubairshakoorarbisoft May 20, 2025

Choose a reason for hiding this comment

Uh oh!

wittjeff May 27, 2025

Choose a reason for hiding this comment

Uh oh!

open-craft-grove commented May 21, 2025

Uh oh!

awais-ansari commented May 21, 2025

Uh oh!

brian-smith-tcril left a comment

Choose a reason for hiding this comment

Uh oh!

wittjeff left a comment

Choose a reason for hiding this comment

Uh oh!

wittjeff May 27, 2025

Choose a reason for hiding this comment

Uh oh!

wittjeff May 27, 2025

Choose a reason for hiding this comment

Uh oh!

wittjeff May 27, 2025

Choose a reason for hiding this comment

Uh oh!

wittjeff May 27, 2025

Choose a reason for hiding this comment

Uh oh!

wittjeff commented May 27, 2025

Uh oh!

brian-smith-tcril commented May 27, 2025

Uh oh!

brian-smith-tcril commented May 27, 2025

master

This PR

Uh oh!

wittjeff commented May 27, 2025

Uh oh!

wittjeff commented May 27, 2025

master

This PR

Uh oh!

Uh oh!

zubairshakoorarbisoft commented Mar 13, 2025 •

edited

Loading

`master`

`master`