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

ARIA MenuBar keyboard interaction improvements #477

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all 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
36 changes: 36 additions & 0 deletions examples/example-menubar/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,42 @@ function main(): void {
addMenuItem(commands, fileMenu, 'new', 'New', 'File > New');
addMenuItem(commands, fileMenu, 'open', 'Open', 'File > Open');
addMenuItem(commands, fileMenu, 'save', 'Save', 'File > Save');

const recentMenu = new Menu({ commands: commands });
recentMenu.title.label = 'Open Recent';
addMenuItem(
commands,
recentMenu,
'file1',
'File1.txt',
'File > Open Recent > File1.txt'
);
addMenuItem(
commands,
recentMenu,
'file2',
'File2.md',
'File > Open Recent > File2.md'
);
addMenuItem(
commands,
recentMenu,
'file3',
'File3.xml',
'File > Open Recent > File3.xml'
);
addMenuItem(
commands,
recentMenu,
'file4',
'File4.txt',
'File > Open Recent > File4.txt'
);
Comment on lines +104 to +133
Copy link
Contributor

Choose a reason for hiding this comment

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

Readability:

Suggested change
const recentMenu = new Menu({ commands: commands });
recentMenu.title.label = 'Open Recent';
addMenuItem(
commands,
recentMenu,
'file1',
'File1.txt',
'File > Open Recent > File1.txt'
);
addMenuItem(
commands,
recentMenu,
'file2',
'File2.md',
'File > Open Recent > File2.md'
);
addMenuItem(
commands,
recentMenu,
'file3',
'File3.xml',
'File > Open Recent > File3.xml'
);
addMenuItem(
commands,
recentMenu,
'file4',
'File4.txt',
'File > Open Recent > File4.txt'
);
const recentMenu = createRecentSubmenu();

fileMenu.addItem({
type: 'submenu',
submenu: recentMenu
});

menubar.addMenu(fileMenu);

const editMenu = new Menu({ commands: commands });
Expand Down
83 changes: 76 additions & 7 deletions packages/widgets/src/menu.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import {
VirtualDOM,
VirtualElement
} from '@lumino/virtualdom';
import { MenuBar } from './menubar';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import { MenuBar } from './menubar';
import type { MenuBar } from './menubar';

This lets the code reader know that we're not actually creating MenuBar instances in this file, just using it for type definitions.


import { Widget } from './widget';

Expand Down Expand Up @@ -234,6 +235,30 @@ export class Menu extends Widget {
return this._items;
}

/**
* Activate the first selectable item in the menu.
*/
activateFirstItem(): void {
this.activeIndex = ArrayExt.findFirstIndex(
this._items,
Private.canActivate,
0,
this._items.length
);
Comment on lines +242 to +247
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
this.activeIndex = ArrayExt.findFirstIndex(
this._items,
Private.canActivate,
0,
this._items.length
);
this.activeIndex = ArrayExt.findFirstIndex(
this._items,
Private.canActivate
);

}

/**
* Activate the first selectable item in the menu.
*/
activateLastItem(): void {
this.activeIndex = ArrayExt.findFirstIndex(
this._items,
Private.canActivate,
this._items.length,
0
);
Comment on lines +254 to +259
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
this.activeIndex = ArrayExt.findFirstIndex(
this._items,
Private.canActivate,
this._items.length,
0
);
this.activeIndex = ArrayExt.findLastIndex(
this._items,
Private.canActivate
);

}

/**
* Activate the next selectable item in the menu.
*
Expand Down Expand Up @@ -319,6 +344,23 @@ export class Menu extends Widget {
}
}

/**
* Close the menu completely.
*/
closeMenu(): void {
// Bail if the menu is not attached.
if (!this.isAttached) {
return;
}

// Cancel the pending timers.
this._cancelOpenTimer();
this._cancelCloseTimer();

// Close the root menu before executing the command.
this.rootMenu.close();
}
Comment on lines +350 to +362
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I'm familiar enough with the Menu class to adequately review this method.


/**
* Add a menu item to the end of the menu.
*
Expand Down Expand Up @@ -502,6 +544,13 @@ export class Menu extends Widget {
}
}

/**
* Set the parent MenuBar, if the Menu is contained within one.
*/
set parentMenuBar(value: MenuBar) {
this._parentMenuBar = value;
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably need to set this to null in the dispose method.

}

/**
* A message handler invoked on a `'before-attach'` message.
*/
Expand Down Expand Up @@ -607,25 +656,44 @@ export class Menu extends Widget {
* This listener is attached to the menu node.
*/
private _evtKeyDown(event: KeyboardEvent): void {
// A menu handles all keydown events.
event.preventDefault();
event.stopPropagation();

// Fetch the key code for the event.
let kc = event.keyCode;

// Enter
if (kc === 13) {
// A menu handles all keydown events, except for tab, which we let propagate.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// A menu handles all keydown events, except for tab, which we let propagate.
// A menu handles all keydown events, except for tab, which we let propagate
// for accessibility, so that users can navigate the UI using only their keyboard.

if (kc === 9) {
this.closeMenu();
return;
}

event.preventDefault();
event.stopPropagation();

// Enter or Space
if (kc === 13 || kc === 32) {
this.triggerActiveItem();
return;
}

// Escape
if (kc === 27) {
this.close();
// If this menu is in a menubar, refocus the menubar.
if (this._parentMenuBar) {
this._parentMenuBar.activate();
}
return;
}

// Home
if (kc === 36) {
this.activateFirstItem();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
this.activateFirstItem();
this.activateFirstItem();
return;

}

// End
if (kc === 35) {
this.activateLastItem();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
this.activateLastItem();
this.activateLastItem();
return;

}

// Left Arrow
if (kc === 37) {
if (this._parentMenu) {
Expand Down Expand Up @@ -938,6 +1006,7 @@ export class Menu extends Widget {
private _items: Menu.IItem[] = [];
private _childMenu: Menu | null = null;
private _parentMenu: Menu | null = null;
private _parentMenuBar: MenuBar | null = null;
private _aboutToClose = new Signal<this, void>(this);
private _menuRequested = new Signal<this, 'next' | 'previous'>(this);
}
Expand Down Expand Up @@ -1175,7 +1244,7 @@ export namespace Menu {
{
className,
dataset,
tabindex: '0',
tabindex: '-1',
Copy link
Contributor

Choose a reason for hiding this comment

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

In other words, the user will navigate to menu items via the left/right arrow keys?

onfocus: data.onfocus,
...aria
},
Expand Down
19 changes: 16 additions & 3 deletions packages/widgets/src/menubar.ts
Original file line number Diff line number Diff line change
Expand Up @@ -383,7 +383,7 @@ export class MenuBar extends Widget {
*/
protected onActivateRequest(msg: Message): void {
if (this.isAttached) {
this.activeIndex = 0;
this.activeIndex = this._tabFocusIndex;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice

}
}

Expand Down Expand Up @@ -446,8 +446,18 @@ export class MenuBar extends Widget {
// Escape
if (kc === 27) {
this._closeChildMenu();
this.activeIndex = -1;
this.node.blur();
return;
}

// Home
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Home
// Home - select (without opening) the first menu in the menubar

if (kc === 36) {
this.activeIndex = 0;
return;
}

// End
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// End
// End - select (without opening) the last menu in the menubar

if (kc === 35) {
this.activeIndex = this._menus.length - 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this is going to play nicely with #489.

cc @steff456

return;
}

Expand Down Expand Up @@ -625,6 +635,9 @@ export class MenuBar extends Widget {
// Swap the internal menu reference.
this._childMenu = newMenu;

// Set the reference to this MenuBar that contains the menu.
this._childMenu.parentMenuBar = this;

// Close the current menu, or setup for the new menu.
if (oldMenu) {
oldMenu.close();
Expand Down