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

Improve the menubar accessibility #465

Merged
merged 20 commits into from
Nov 28, 2022
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
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: 1 addition & 1 deletion .github/workflows/license-header.yml
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ jobs:
id: changed-files
uses: tj-actions/[email protected]
with:
base_sha: 'HEAD~1'
since_last_remote_commit: "true"
sha: 'HEAD'

- name: Push fixes
Expand Down
16 changes: 16 additions & 0 deletions examples/example-menubar/index.html
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
{
"name": "@lumino/example-menubar",
"version": "0.1.0-alpha.6",
"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
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;
121 changes: 121 additions & 0 deletions examples/example-menubar/src/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
// 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 button = document.createElement('button');
button.innerHTML = 'A button you can tab to out of the menubar';
node.appendChild(button);
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();

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);

const article = new Article();

appLayout.addWidget(skipLink);
appLayout.addWidget(menubar);
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
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
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
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/*"]
}
36 changes: 28 additions & 8 deletions packages/widgets/src/menubar.ts
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,11 @@ export class MenuBar extends Widget {
// Update the active index.
this._activeIndex = value;

// Update the focus index.
if (value !== -1) {
this._focusIndex = value;
}

// Update focus to new active index
if (
this._activeIndex >= 0 &&
Expand Down Expand Up @@ -378,7 +383,7 @@ export class MenuBar extends Widget {
*/
protected onActivateRequest(msg: Message): void {
if (this.isAttached) {
this.node.focus();
this.activeIndex = 0;
}
}

Expand All @@ -389,6 +394,10 @@ export class MenuBar extends Widget {
let menus = this._menus;
let renderer = this.renderer;
let activeIndex = this._activeIndex;
let focusIndex =
this._focusIndex >= 0 && this._focusIndex < menus.length
? this._focusIndex
: 0;
let content = new Array<VirtualElement>(menus.length);
for (let i = 0, n = menus.length; i < n; ++i) {
let title = menus[i].title;
Expand All @@ -399,6 +408,7 @@ export class MenuBar extends Widget {
content[i] = renderer.renderItem({
title,
active,
focusable: i === focusIndex,
onfocus: () => {
this.activeIndex = i;
}
Expand All @@ -417,17 +427,18 @@ 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 do not trap the tab key.
if (kc === 9) {
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.

This introduces an inconsistency that makes me uncomfortable. The active index gets reset when you tab out of the menubar but not when you click out. On the example page, if you tab to the File menu, then click on the button, the File menu stays shaded. But if you tab to the File menu, then tab to the button, the File menu loses its shading.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can live with this inconsistency for now, but we will need to address it in the future.

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 @@ -652,7 +663,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 @@ -727,6 +737,7 @@ export class MenuBar extends Widget {
}

private _activeIndex = -1;
private _focusIndex = 0;
scmmmh marked this conversation as resolved.
Show resolved Hide resolved
private _forceItemsPosition: Menu.IOpenOptions;
private _menus: Menu[] = [];
private _childMenu: Menu | null = null;
Expand Down Expand Up @@ -772,6 +783,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 focusable: boolean;

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

Expand Down Expand Up @@ -808,7 +824,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.focusable ? '0' : '-1',
onfocus: data.onfocus,
...aria
},
this.renderIcon(data),
this.renderLabel(data)
);
Expand Down Expand Up @@ -941,8 +963,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