Skip to content

Resolving some accessibility issues #6719

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

Merged
jtpio merged 6 commits into from
Feb 9, 2023
Merged

Resolving some accessibility issues #6719

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
dba0b92
Uses different ID for the running-session widgets, the one in main vi…
brichet Jan 6, 2023
13b3c17
Append kernel logo image only if necessary
brichet Feb 8, 2023
1d28d19
Add title to the button which close sidebar
brichet Feb 8, 2023
5038d6d
Add landmarks roles
brichet Feb 8, 2023
993e942
Fix UI-test
brichet Feb 9, 2023
8e51cd4
lint
brichet Feb 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/application/src/panelhandler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

@jtpio jtpio Feb 9, 2023

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

@brichet brichet Feb 9, 2023

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

@jtpio jtpio Feb 9, 2023

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

@jtpio jtpio Feb 9, 2023

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

@jtpio jtpio Feb 9, 2023

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.


const icon = new Widget({ node: closeButton });
this._panel.addWidget(icon);
this._panel.addWidget(this._widgetPanel);
Expand Down
5 changes: 5 additions & 0 deletions packages/application/src/shell.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,11 @@ export class NotebookShell extends Widget implements JupyterFrontEnd.IShell {
const menuWrapper = (this._menuWrapper = new Panel());

this._topHandler.panel.id = 'top-panel';
this._topHandler.panel.node.setAttribute('role', 'banner');
this._menuHandler.panel.id = 'menu-panel';
this._menuHandler.panel.node.setAttribute('role', 'navigation');
this._main.id = 'main-panel';
this._main.node.setAttribute('role', 'main');

this._spacer = new Widget();
this._spacer.id = 'spacer-widget';
Expand All @@ -63,7 +66,9 @@ export class NotebookShell extends Widget implements JupyterFrontEnd.IShell {
const rightHandler = this._rightHandler;

leftHandler.panel.id = 'jp-left-stack';
leftHandler.panel.node.setAttribute('role', 'complementary');
rightHandler.panel.id = 'jp-right-stack';
rightHandler.panel.node.setAttribute('role', 'complementary');

// Hide the side panels by default.
leftHandler.hide();
Expand Down
7 changes: 6 additions & 1 deletion packages/notebook-extension/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,26 +141,31 @@ const kernelLogo: JupyterFrontEndPlugin<void> = {

const node = document.createElement('div');
const img = document.createElement('img');
node.appendChild(img);

const onChange = async () => {
const current = shell.currentWidget;
if (!(current instanceof NotebookPanel)) {
return;
}

if (!node.hasChildNodes()) {
node.appendChild(img);
}

await current.sessionContext.ready;
current.sessionContext.kernelChanged.disconnect(onChange);
current.sessionContext.kernelChanged.connect(onChange);

const name = current.sessionContext.session?.kernel?.name ?? '';
const spec = serviceManager.kernelspecs?.specs?.kernelspecs[name];
if (!spec) {
node.childNodes[0].remove();
return;
}

const kernelIconUrl = spec.resources['logo-64x64'];
if (!kernelIconUrl) {
node.childNodes[0].remove();
return;
}

Expand Down
2 changes: 1 addition & 1 deletion packages/tree-extension/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

@jtpio jtpio Feb 8, 2023

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

running.title.label = trans.__('Running');
running.title.icon = runningIcon;
nbTreeWidget.addWidget(running);
Expand Down
4 changes: 3 additions & 1 deletion ui-tests/test/tree.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,9 @@ test('should update url when navigating in filebrowser', async ({
test('Should activate file browser tab', async ({ page, tmpPath }) => {
await page.goto(`tree/${tmpPath}`);
await page.click('text="Running"');
await expect(page.locator('#main-panel #jp-running-sessions')).toBeVisible();
await expect(
page.locator('#main-panel #jp-running-sessions-tree')
).toBeVisible();

await page.menu.clickMenuItem('View>File Browser');
await expect(page.locator('#main-panel #filebrowser')).toBeVisible();
Expand Down