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

Conversation

scmmmh
Copy link
Contributor

@scmmmh scmmmh commented Nov 28, 2022

This is an extension of my previous work on improving the MenuBar keyboard navigation ARIA compliance. This should now implement all the required keyboard interaction patterns. I've not created any test cases for this yet, but that is the next step.

@gabalafou If you have time to have a look, that would be great. Then I'll do some more work on getting some test-cases set up for this.

@fcollonval fcollonval added enhancement New feature or request accessibility Addresses accessibility needs labels Nov 29, 2022
@krassowski
Copy link
Member

Probably worth reviewing this PR with comments from the previous one: #465

@afshin afshin self-requested a review January 11, 2023 18:53
Copy link
Contributor

@gabalafou gabalafou left a comment

Choose a reason for hiding this comment

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

I'm loving these UX improvements!

Sorry it took me so long to get around to reviewing this PR. I never saw the notification from my at-mention.

This PR is in draft mode because... you're planning on adding tests?


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

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;

Comment on lines +242 to +247
this.activeIndex = ArrayExt.findFirstIndex(
this._items,
Private.canActivate,
0,
this._items.length
);
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
);

Comment on lines +254 to +259
this.activeIndex = ArrayExt.findFirstIndex(
this._items,
Private.canActivate,
this._items.length,
0
);
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
);

* 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.

Comment on lines +350 to +362
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();
}
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.

Comment on lines +104 to +133
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'
);
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();

@@ -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.

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

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

@scmmmh
Copy link
Contributor Author

scmmmh commented Jan 19, 2023

Thanks for the comments. I haven't had any time to progress this and won't get around to replying until next week, so no worries about the delay.

As you said, the PR is in draft form, because I have no tests yet.

@gabalafou
Copy link
Contributor

@scmmmh would you mind if I take over this PR?

@scmmmh
Copy link
Contributor Author

scmmmh commented Jun 1, 2023

Yes please. It has been hovering over my head, but I just haven't been able to allocate time to it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility Addresses accessibility needs enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants