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.

Sign up for GitHub

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

Jump to bottom

Resolving some accessibility issues #6719

Merged
merged 6 commits into from
Feb 9, 2023
Merged

Resolving some accessibility issues #6719

merged 6 commits into from
Feb 9, 2023

Conversation

brichet
Copy link
Collaborator

@brichet brichet commented Feb 8, 2023 •
edited
Loading

This PR addresses some accessibility issues:

  • adds landmarks roles
  • removes duplicate id on running session from side panel and form main area
  • adds title on the sidebar button (a later PR will be necessary for translation)
  • avoids adding kernel logo image if not necessary, to avoid image without title

Related to #6671

@github-actions
Copy link
Contributor

github-actions bot commented Feb 8, 2023

Binder 👈 Launch a Binder on branch brichet/notebook/accessibility

@jtpio jtpio added this to the 7.0 milestone Feb 8, 2023
jtpio
jtpio reviewed Feb 8, 2023
View reviewed changes
packages/tree-extension/src/index.ts
@@ -185,7 +185,7 @@ const notebookTreeWidget: JupyterFrontEndPlugin<INotebookTree> = {

if (manager) {
const running = new RunningSessions(manager, translator);
running.id = 'jp-running-sessions';
running.id = 'jp-running-sessions-tree';
Copy link
Member

Choose a reason for hiding this comment

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

Looks like the following selector should also be updated in the UI tests:

notebook/ui-tests/test/tree.spec.ts

Line 43 in ce9c16a

await expect(page.locator('#main-panel #jp-running-sessions')).toBeVisible();

Copy link
Collaborator Author

@brichet brichet Feb 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @jtpio

jtpio
jtpio reviewed Feb 9, 2023
View reviewed changes
packages/application/src/panelhandler.ts
@@ -90,6 +90,8 @@ export class SidePanelHandler extends PanelHandler {
this.hide();
};
closeButton.className = 'jp-Button jp-SidePanel-collapse';
closeButton.title = 'Collapse side panel';
Copy link
Member

Choose a reason for hiding this comment

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

Should this string be translated?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, but I was thinking doing this in a later PR, as it involves more changes.
The sidepanels are created with the NotebookShell, so (I guess) before the translator.
Currently the NotebookShell does not include a translator, so we should probably add a translator-extension that add a translator to the shell, in order to translate the title.
If you think there is a simpler way maybe we can include it in this PR.

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. Then we can indeed have a look separately and track this in an issue.

Copy link
Member

Choose a reason for hiding this comment

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

Just checked and JupyterLab sets the translator here:

https://github.com/jupyterlab/jupyterlab/blob/e81eb3d5e05e63f162d729066c2f644aa6243a32/packages/translation-extension/src/index.ts#L56-L59

Copy link
Member

Choose a reason for hiding this comment

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

Opened #6722 to track this.

jtpio
jtpio approved these changes Feb 9, 2023
View reviewed changes
Copy link
Member

@jtpio jtpio left a comment

Choose a reason for hiding this comment

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

Thanks!

@jtpio jtpio merged commit 905553e into jupyter:main Feb 9, 2023
@jtpio jtpio mentioned this pull request Feb 9, 2023
Closed
@brichet brichet mentioned this pull request Feb 9, 2023
Merged
@brichet brichet deleted the accessibility branch February 10, 2023 13:54
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jtpio jtpio jtpio approved these changes

Assignees

@brichet brichet

Labels
enhancement
Projects
None yet
Milestone
7.0
Development

Successfully merging this pull request may close these issues.
2 participants
@brichet @jtpio