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

Improve the menubar accessibility #465

Merged
merged 20 commits into from
Nov 28, 2022
Merged

Improve the menubar accessibility #465

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
601c14b
Bring keyboard handling closer in line with WCAG2.1
scmmmh Nov 14, 2022
41bcc6f
Code cleanup
scmmmh Nov 14, 2022
29d2e8b
Fix focusing the menubar on event
scmmmh Nov 14, 2022
4231171
Make the tests check that the activeElement is a descendant
scmmmh Nov 14, 2022
588348d
More work on the tests
scmmmh Nov 14, 2022
ad90278
Fixed the focus / tab tests
scmmmh Nov 14, 2022
fc2c5f9
Added missing test for the space activation
scmmmh Nov 14, 2022
f4d6525
Cleanup the example
scmmmh Nov 14, 2022
0720b26
Switch to the ARIA menubar example interaction pattern
scmmmh Nov 16, 2022
4fe3e21
Set the activeIndex in onActivateRequest
scmmmh Nov 16, 2022
480267b
Remove old code
scmmmh Nov 16, 2022
8eb88b2
Code cleanup
scmmmh Nov 16, 2022
44fe991
Remove test for scenario that cannot occur
scmmmh Nov 16, 2022
0a5ebb4
Remember the last focused menu item
scmmmh Nov 16, 2022
8e6551e
Test fix
scmmmh Nov 16, 2022
ff5d5f1
Fix license header job
fcollonval Nov 16, 2022
cdc8143
Update examples/example-menubar/package.json
scmmmh Nov 21, 2022
7ef2870
Small rewordings
scmmmh Nov 22, 2022
acac897
Small doc changes
scmmmh Nov 22, 2022
89c792a
Widgets API update
scmmmh Nov 24, 2022
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
16 changes: 16 additions & 0 deletions examples/example-menubar/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
<!--
~ Copyright (c) Jupyter Development Team.
~ Distributed under the terms of the Modified BSD License.
-->

<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="UTF-8">
<link href="https://maxcdn.bootstrapcdn.com/font-awesome/4.2.0/css/font-awesome.min.css" rel="stylesheet">
<script type="text/javascript" src="build/bundle.example.js"></script>
<title>MenuBar Example</title>
</head>
<body>
</body>
</html>
21 changes: 21 additions & 0 deletions examples/example-menubar/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
{
"name": "@lumino/example-menubar",
"version": "0.9.0-alpha.6",
scmmmh marked this conversation as resolved.
Show resolved Hide resolved
"private": true,
"scripts": {
"build": "tsc && rollup -c",
"clean": "rimraf build"
},
"dependencies": {
"@lumino/default-theme": "^1.0.0-alpha.6",
"@lumino/messaging": "^2.0.0-alpha.6",
"@lumino/widgets": "^2.0.0-alpha.6"
scmmmh marked this conversation as resolved.
Show resolved Hide resolved
},
"devDependencies": {
"@rollup/plugin-node-resolve": "^13.3.0",
"rimraf": "^3.0.2",
"rollup": "^2.77.3",
"rollup-plugin-styles": "^4.0.0",
"typescript": "~4.7.3"
}
}
8 changes: 8 additions & 0 deletions examples/example-menubar/rollup.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
/*
* Copyright (c) Jupyter Development Team.
* Distributed under the terms of the Modified BSD License.
*/

import { createRollupConfig } from '../../rollup.examples.config';
const rollupConfig = createRollupConfig();
export default rollupConfig;
125 changes: 125 additions & 0 deletions examples/example-menubar/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
// Copyright (c) Jupyter Development Team.
// Distributed under the terms of the Modified BSD License.

import { CommandRegistry } from '@lumino/commands';
import { Menu, MenuBar, PanelLayout, Widget } from '@lumino/widgets';

import '../style/index.css';

/**
* Wrapper widget containing the example application.
*/
class Application extends Widget {
constructor() {
super({ tag: 'main' });
}
}

/**
* Skip link to jump to the main content.
*/
class SkipLink extends Widget {
/**
* Create a HTMLElement that statically links to "#content".
*/
static createNode(): HTMLElement {
const node = document.createElement('a');
node.setAttribute('href', '#content');
node.innerHTML = 'Skip to the main content';
node.classList.add('lm-example-skip-link');
return node;
}

constructor() {
super({ node: SkipLink.createNode() });
}
}

/**
* A Widget containing some content to provide context example.
*/
class Article extends Widget {
/**
* Create the content structure.
*/
static createNode(): HTMLElement {
const node = document.createElement('div');
node.setAttribute('id', 'content');
node.setAttribute('tabindex', '-1');
const h1 = document.createElement('h1');
h1.innerHTML = 'MenuBar Example';
node.appendChild(h1);
const label = document.createElement('label');
label.appendChild(
document.createTextNode('A textarea to demonstrate the tab handling.')
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add some more prose explaining the inclusion of the textarea on the page? I'm afraid that in the future without a better explanation, it's not going to be quite clear why the textarea is on the page, or what purpose it serves.

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'll add a comment. It is basically there so that there is some content that can be tabbed to. Otherwise the tab out of the menubar takes you into the browser UI and that can often be a bit harder to see where it has gone (I guess an a11y issue in itself).

);
const textarea = document.createElement('textarea');
textarea.setAttribute('autocomplete', 'off');
label.appendChild(textarea);
node.appendChild(label);
return node;
}

constructor() {
super({ node: Article.createNode() });
}
}

/**
* Helper Function to add menu items.
*/
function addMenuItem(
commands: CommandRegistry,
menu: Menu,
command: string,
label: string,
log: string
): void {
commands.addCommand(command, {
label: label,
execute: () => {
console.log(log);
}
});
menu.addItem({
type: 'command',
command: command
});
}

/**
* Create the MenuBar example application.
*/
function main(): void {
const app = new Application();
const appLayout = new PanelLayout();
app.layout = appLayout;

const skipLink = new SkipLink();
appLayout.addWidget(skipLink);

const menubar = new MenuBar();
const commands = new CommandRegistry();

const fileMenu = new Menu({ commands: commands });
fileMenu.title.label = 'File';
addMenuItem(commands, fileMenu, 'new', 'New', 'File > New');
addMenuItem(commands, fileMenu, 'open', 'Open', 'File > Open');
addMenuItem(commands, fileMenu, 'save', 'Save', 'File > Save');
menubar.addMenu(fileMenu);

const editMenu = new Menu({ commands: commands });
editMenu.title.label = 'Edit';
addMenuItem(commands, editMenu, 'cut', 'Cut', 'Edit > Cut');
addMenuItem(commands, editMenu, 'copy', 'Copy', 'Edit > Copy');
addMenuItem(commands, editMenu, 'paste', 'Paste', 'Edit > Paste');
menubar.addMenu(editMenu);
appLayout.addWidget(menubar);

const article = new Article();
appLayout.addWidget(article);

fcollonval marked this conversation as resolved.
Show resolved Hide resolved
Widget.attach(app, document.body);
}

window.onload = main;
30 changes: 30 additions & 0 deletions examples/example-menubar/style/content.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*
Copyright (c) Jupyter Development Team.
Distributed under the terms of the Modified BSD License.
*/

.lm-example-skip-link {
position: absolute;
left: -1000px;
top: -1000px;
}

.lm-example-skip-link:focus {
position: fixed;
left: 50%;
top: 0;
z-index: 10;
padding: 0.5rem 1rem;
box-shadow: 0 0 5px #000;
border-bottom-left-radius: 0.2rem;
border-bottom-right-radius: 0.2rem;
background: #fff;
}

textarea {
display: block;
}

#content {
padding: 0 1rem;
}
19 changes: 19 additions & 0 deletions examples/example-menubar/style/index.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
/*
Copyright (c) Jupyter Development Team.
Distributed under the terms of the Modified BSD License.
*/
@import '@lumino/default-theme/style/index.css';
@import './content.css';

body {
display: flex;
flex-direction: column;
position: absolute;
top: 0;
left: 0;
right: 0;
bottom: 0;
margin: 0;
padding: 0;
overflow: hidden;
}
17 changes: 17 additions & 0 deletions examples/example-menubar/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
{
"compilerOptions": {
"declaration": false,
"noImplicitAny": true,
"noEmitOnError": true,
"noUnusedLocals": true,
"strictNullChecks": true,
"sourceMap": true,
"module": "ES6",
"moduleResolution": "node",
"target": "ES2018",
"outDir": "./build",
"lib": ["DOM", "ES2018"],
"types": []
},
"include": ["src/*"]
}
43 changes: 36 additions & 7 deletions packages/widgets/src/menubar.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,9 @@ export class MenuBar extends Widget {
event.preventDefault();
event.stopPropagation();
break;
case 'focus':
this._evtFocus(event as FocusEvent);
break;
}
}

Expand All @@ -359,6 +362,7 @@ export class MenuBar extends Widget {
this.node.addEventListener('mousemove', this);
this.node.addEventListener('mouseleave', this);
this.node.addEventListener('contextmenu', this);
this.contentNode.addEventListener('focus', this);
}

/**
Expand All @@ -370,6 +374,7 @@ export class MenuBar extends Widget {
this.node.removeEventListener('mousemove', this);
this.node.removeEventListener('mouseleave', this);
this.node.removeEventListener('contextmenu', this);
this.contentNode.removeEventListener('focus', this);
this._closeChildMenu();
}

Expand All @@ -378,7 +383,7 @@ export class MenuBar extends Widget {
*/
protected onActivateRequest(msg: Message): void {
if (this.isAttached) {
this.node.focus();
this.contentNode.focus();
}
}

Expand All @@ -399,6 +404,7 @@ export class MenuBar extends Widget {
content[i] = renderer.renderItem({
title,
active,
index: i,
onfocus: () => {
this.activeIndex = i;
}
Expand All @@ -417,17 +423,20 @@ export class MenuBar extends Widget {
// Fetch the key code for the event.
let kc = event.keyCode;

// Do not trap the tab key.
// Reset the active index on tab (but not reverse tab), but do not trap the tab key.
if (kc === 9) {
if (!event.shiftKey) {
this.activeIndex = -1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, even with the comment I'm still not quite sure I get what this line is doing.

I thought if it's not resetting the activeIndex, then I should be able to do the following on the example page:

  1. Tab to the menu bar
  2. Right arrow key over to Edit (it's highlighted now)
  3. Click on the text area input ("Edit" is still highlighted)
  4. Press Shift + Tab and...
    Expected behavior: "Edit" remains highlighted and if I press Enter it should open the Edit menu.
    Actual behavior: the File menu items becomes highlighted and if I press Enter it opens the File menu.

}
return;
}

// A menu bar handles all other keydown events.
event.preventDefault();
event.stopPropagation();

// Enter, Up Arrow, Down Arrow
if (kc === 13 || kc === 38 || kc === 40) {
// Enter, Space, Up Arrow, Down Arrow
if (kc === 13 || kc === 32 || kc === 38 || kc === 40) {
this.openActiveMenu();
return;
}
Expand Down Expand Up @@ -591,6 +600,17 @@ export class MenuBar extends Widget {
}
}

/**
* Handle the `'focus'` event for the menu bar.
*/
private _evtFocus(event: FocusEvent): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we wish to reset the active menu item when the menu bar receives focus?

I would like to make the Lumino menubar behave more like the example menubar in the ARIA Authoring Practices Guide.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is caused by the interpretation of what it means for the "menubar to receive focus", as I put in the main reply. I'll switch to the interpretation used in the example, which will also make the code much simpler.

if (this.activeIndex === -1) {
this.activeIndex = 0;
} else {
this.activeIndex = -1;
}
}

/**
* Open the child menu at the active index immediately.
*
Expand Down Expand Up @@ -652,7 +672,6 @@ export class MenuBar extends Widget {
if (!this._childMenu) {
return;
}

// Remove the active class from the menu bar.
this.removeClass('lm-mod-active');

Expand Down Expand Up @@ -772,6 +791,11 @@ export namespace MenuBar {
*/
readonly active: boolean;

/**
* The index of the item in the list of items.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this description was meant for a different variable? focusable is a boolean, not an index.

While we're at it, could we rename this variable? To be, focusable means "able to be focussed" but I think what this variable really means is more like "should be made focusable" (by setting tabindex to 0).

What do we think about makeFocusable or enableFocus or enableTabIndex or...?

*/
readonly index: number;

readonly onfocus?: (event: FocusEvent) => void;
}

Expand Down Expand Up @@ -808,7 +832,13 @@ export namespace MenuBar {
let dataset = this.createItemDataset(data);
let aria = this.createItemARIA(data);
return h.li(
{ className, dataset, tabindex: '0', onfocus: data.onfocus, ...aria },
{
className,
dataset,
tabindex: data.index === 0 ? '0' : '-1',
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I understand this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only one of the menuitems should be focusable using the tab key (tabindex="0") and following the spec, the first menuitem is set to that and all of the others are set to be focusable via code (tabindex="-1"). In the future, if we want to have the tab key return to the last selected menuitem, then this will need changing to do that.

onfocus: data.onfocus,
...aria
},
this.renderIcon(data),
this.renderLabel(data)
);
Expand Down Expand Up @@ -941,7 +971,6 @@ namespace Private {
content.className = 'lm-MenuBar-content';
node.appendChild(content);
content.setAttribute('role', 'menubar');
node.tabIndex = 0;
scmmmh marked this conversation as resolved.
Show resolved Hide resolved
content.tabIndex = 0;
return node;
}
Expand Down
Loading