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

Add Accessibility API #10961

Merged
merged 6 commits into from
Apr 11, 2022
Merged

Add Accessibility API #10961

merged 6 commits into from
Apr 11, 2022

Conversation

sgraband
Copy link
Contributor

@sgraband sgraband commented Mar 30, 2022

What it does

Add AccessibilityInformation interface.
Add optional accessibilityInformation to StatusBarItem and TreeItem.
Also added the type to TimelineItem, but will create a follow-up ticket for loading the information into the DOM.
Proposed API:
Add StatusBarItemOptions (containing accessibilityInformation).
Provide window.createStatusBarItem(StatusBarItemOptions).

Update createStatusBarItem handler to handle StatusBarItems.
Added aria-labels for default StatusBarItems.

Resolves #9965.

Contributed on behalf of STMicroelectronics.

How to test

StatusBarItems:
Check one of the StatusBarItems, like the Toggle Bottom Panel, in the Accessibilty tab of the Chrome DevTools. The aria-label should be recognized correctly.

TreeItems:
To simplify the testing the tree-view-sample vscode-extension could be used. Just add:

treeItem.accessibilityInformation = {
	label: 'somValue',
	role: 'someValue'
};

here. And add the extension to Theia. The jsonOutline view tree should then also display the correct label and role.

Review checklist

Reminder for reviewers

@JonasHelming JonasHelming requested a review from msujew March 30, 2022 19:46
@msujew msujew added accessibility issues related to accessbilitiy vscode issues related to VSCode compatibility labels Mar 30, 2022
CHANGELOG.md Outdated Show resolved Hide resolved
packages/plugin/src/theia-proposed.d.ts Outdated Show resolved Hide resolved
packages/plugin/src/theia-proposed.d.ts Outdated Show resolved Hide resolved
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Looking like a good start already :) I was planning on doing some work on a11y as well, but the less for me to do there the better 👍

I have some minor code suggestions, in addition to what I already said regarding the proposed API stuff (see comment). I haven't tested the changes yet, will do so later this week.

@sgraband sgraband force-pushed the 9965 branch 2 times, most recently from c38e015 to 34cc6f4 Compare April 6, 2022 07:24
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

@sgraband it seems as though the ariaLabel is not respected when a plugin defines it when creating the StatusBarItem. It always ends up using the tooltip for such a case.

You can confirm with status-item-0.0.1.zip, I create the item like so (when triggering the StatusBar Item: Create command):

let disposable = vscode.commands.registerCommand('status-item.create', () => {
    const item = vscode.window.createStatusBarItem();
    item.name = 'Test Item';
    item.text = 'Test Text';
    item.accessibilityInformation = { label: 'Test Label', role: 'button' };
    item.show();
});

The same is also true for role.

packages/core/src/browser/status-bar/status-bar.tsx Outdated Show resolved Hide resolved
@vince-fugnitto
Copy link
Member

@sgraband I have the following commit that handles the accessibilityInformation for the StatusBarItem that you can take a look at. It does not require a change to createStatusBarItem as it follows the design of other attributes.

I also confirmed it works with different use-cases using status-item-0.0.1.zip

@sgraband
Copy link
Contributor Author

sgraband commented Apr 7, 2022

Good catch and thank you for the commit @vince-fugnitto! I also tested it with the extension you provided and everything seems to work like intended now.

packages/core/src/browser/status-bar/status-bar.tsx Outdated Show resolved Hide resolved
packages/core/src/browser/status-bar/status-bar.tsx Outdated Show resolved Hide resolved
packages/plugin/src/theia.d.ts Outdated Show resolved Hide resolved
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

I'll let @msujew complete the review since part of the changes are from the commit I proposed 👍

packages/core/src/browser/status-bar/status-bar.tsx Outdated Show resolved Hide resolved
@msujew msujew self-requested a review April 8, 2022 22:55
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

LGTM!

  • Tree elements display the correct accessibility information
  • Status items display the correct accessibility information

sgraband and others added 5 commits April 11, 2022 15:48
Add AccessibilityInformation interface.
Add optional accessibilityInformation to StatusBarItem and TreeItem.
Proposed API:
Add StatusBarItemOptions (containing accessibilityInformation).
Provide window.createStatusBarItem(StatusBarItemOptions).

Update createStatusBarItem handler to handle StatusBarItems.

Contributed on behalf of STMicroelectronics.
Contributed on behalf of STMicroelectronics.
Contributed on behalf of STMicroelectronics

Co-authored-by: vince-fugnitto <[email protected]>
Contributed on behalf of STMicroelectronics
And add accessibility type to common package.

Contributed on behalf of STMicroelectronics.
@sgraband
Copy link
Contributor Author

Thanks for the review @msujew. I added two accessibilityInformations to the Notification and Toggle Bottom panel status item. I also added the AccessibilityInformation type to the common package like @vince-fugnitto suggested.

@msujew msujew merged commit 64d193d into eclipse-theia:master Apr 11, 2022
@vince-fugnitto vince-fugnitto added this to the 1.25.0 milestone Apr 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility issues related to accessbilitiy vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Accessibility API
3 participants