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

Added tests to check for aria labels and roles for accessibility #15137

Merged
merged 32 commits into from Sep 29, 2023

Conversation

m158261
Copy link
Contributor

@m158261 m158261 commented Sep 19, 2023

References

Related PR - #9622

Code changes

Added tests to check aria labels and roles are present for users with screen readers. There a mostly around sidebar widgets. Some of the tests are run in Galata due to restrictions with rendering extension packages.

User-facing changes

None

Backwards-incompatible changes

None

@jupyterlab-probot
Copy link

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link: Binder

@m158261 m158261 changed the title Added test to check for aria labels and roles Added test to checks for aria labels and roles for accessibility Sep 26, 2023
@m158261 m158261 marked this pull request as ready for review September 26, 2023 10:46
@m158261 m158261 changed the title Added test to checks for aria labels and roles for accessibility Added tests to checks for aria labels and roles for accessibility Sep 26, 2023
@m158261 m158261 changed the title Added tests to checks for aria labels and roles for accessibility Added tests to check for aria labels and roles for accessibility Sep 26, 2023
@krassowski krassowski added this to the 4.1.0 milestone Sep 27, 2023
packages/application/test/shell.spec.ts Outdated Show resolved Hide resolved
Comment on lines 32 to 41
'notebook actions'
);
expect(widget.toolbar.node.getAttribute('role')).toEqual('navigation');
});

it('content should have an aria-label of notebook content and a role of region', () => {
const content = new Widget();
const widget = new MainAreaWidget({ content });
expect(widget.content.node.getAttribute('aria-label')).toEqual(
'notebook content'
Copy link
Member

Choose a reason for hiding this comment

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

This test demonstrated incorrect behaviour (not every main area widget is a notebook, for example a text editor is not a notebook, csv viewer is not a notebook etc), see #13045. I am not sure what the default for a main area widget should be but my expectation is that it would not be "notebook". Also, is the toolbar really navigation? Shouldn't it be a toolbar role?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can change the navigation role to toolbar. As the notebook content label still has an outstanding issue, I could either remove the test or leave it to be updated when that issue is worked on?

Copy link
Member

Choose a reason for hiding this comment

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

Let's keep the test, but:

  • mark it with test.failing as xfail for now
  • add a comment on the test pointing to issue
  • once this PR is merged, comment on the issue that there is now a test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes made.

Copy link
Member

@krassowski krassowski left a comment

Choose a reason for hiding this comment

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

Thank you @m158261!

@@ -46,7 +46,7 @@ export class MainAreaWidget<T extends Widget = Widget>
content.node.setAttribute('role', 'region');
content.node.setAttribute('aria-label', trans.__('notebook content'));
const toolbar = (this._toolbar = options.toolbar || new ReactiveToolbar());
toolbar.node.setAttribute('role', 'navigation');
toolbar.node.setAttribute('role', 'toolbar');
Copy link
Member

Choose a reason for hiding this comment

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

For context, the toolbar role is expected to have arrow key navigation (https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/toolbar_role) which is being implemented in #15021.

@krassowski krassowski merged commit fcce3ce into jupyterlab:main Sep 29, 2023
76 of 80 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants