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

Jump to bottom

ObjectTree/List: Add ariaRootRole option. Addresses: #65939 #67092

Merged
merged 2 commits into from
Jan 25, 2019
Merged

ObjectTree/List: Add ariaRootRole option. Addresses: #65939 #67092

merged 2 commits into from
Jan 25, 2019
+22 −13

Conversation

cleidigh
Copy link
Contributor

@joaomoreno
@roblourens

Addresses: #65939

  • Add new enum EListAriaRootRole for ariaRootRole option (mixed feelings about 'E' prefix)
// Default tree/list root role 'tree' does not support interactive element labeling, 'form' does
export enum EListAriaRootRole {
	TREE = 'tree',		// default tree structure role
	FORM = 'form'		// unstructured - allows interactive elements within tree/list structure
}
  • Use EListAriaRootRole.TREE as default role, no need to change other clients
  • Update settingsTree/settingsEditor2 to use EListAriaRootRole.FORM
  • Handle the two valid roles in listWidget
  • Remove all aria attribute manipulation in Settings Editor

Note:

Reader behavior related to this issue has changed with different versions of readers, Electron
and Chromium. It it appears from testing (both NVDA & Voiceover) that the readers
will ignore the tree related attributes if the root role != 'tree'. I therefore decided
NOT to alter any application logic for those attributes: item role, aria-level, aria-posinset, aria-setsize

I think this is the conservative approach and the minimal amount of changes. I could see
needing to change the item role in the future possibly, but for now works.

Tested:

  • Windows 10, NVDA 2018.4.1 - latest
  • OS X Mojave, Voiceover

@cleidigh cleidigh added accessibility Keyboard, mouse, ARIA, vision, screen readers (non-specific) issues settings-editor VS Code settings editor issues list-widget List widget issues labels Jan 25, 2019
@cleidigh cleidigh added this to the December/January 2019 milestone Jan 25, 2019
@cleidigh cleidigh mentioned this pull request Jan 25, 2019
Closed
joaomoreno
joaomoreno requested changes Jan 25, 2019
View reviewed changes
Copy link
Member

@joaomoreno joaomoreno left a comment

Choose a reason for hiding this comment

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

@cleidigh Love this PR. I'm glad you convinced me about your authorship of this. I have only minor change requests. Leaving the rest to @roblourens

src/vs/base/browser/ui/list/listWidget.ts Outdated
export interface IListOptions<T> extends IListStyles {
readonly identityProvider?: IIdentityProvider<T>;
readonly dnd?: IListDragAndDrop<T>;
readonly enableKeyboardNavigation?: boolean;
readonly keyboardNavigationLabelProvider?: IKeyboardNavigationLabelProvider<T>;
readonly ariaRootRole?: EListAriaRootRole;
Copy link
Member

Choose a reason for hiding this comment

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

You can just call it ariaRole.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

src/vs/base/browser/ui/list/list.ts Outdated
@@ -62,6 +62,12 @@ export interface IIdentityProvider<T> {
getId(element: T): { toString(): string; };
}

// Default tree/list root role 'tree' does not support interactive element labeling, 'form' does
export enum EListAriaRootRole {
Copy link
Member

Choose a reason for hiding this comment

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

ListAriaRole would be enough here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@cleidigh
Copy link
Contributor Author

cleidigh commented Jan 25, 2019 •
edited
Loading

@cleidigh Love this PR. I'm glad you convinced me about your authorship of this. I have only minor change requests. Leaving the rest to @roblourens

Muito obrigado ;-) @joaomoreno I really enjoy working on this with you guys especially for such a great project !
As I have offered to @bpasero & @roblourens if you guys have any special projects, issues or even experimental research, I would be glad to give it a try. I like getting deeper into the CodeBase 🔢

@roblourens roblourens self-assigned this Jan 25, 2019
roblourens
roblourens reviewed Jan 25, 2019
View reviewed changes
src/vs/workbench/parts/preferences/browser/settingsTree.ts
@@ -1292,11 +1287,14 @@ export class SettingsTree extends ObjectTree<SettingsTreeElement> {
) {
const treeClass = 'settings-editor-tree';

// Must use role = EListAriaRootRole.FORM for interactive elements in settingsTree
Copy link
Member

Choose a reason for hiding this comment

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

I think this comment isn't necessary

Copy link
Member

Choose a reason for hiding this comment

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

I'll just merge and delete it :)

roblourens
roblourens approved these changes Jan 25, 2019
View reviewed changes
Copy link
Member

@roblourens roblourens left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good to me

@roblourens roblourens merged commit 6eefb6d into microsoft:master Jan 25, 2019
roblourens added a commit that referenced this pull request Jan 25, 2019
@roblourens
Tweaks from #67092
db771d0
@cleidigh
Copy link
Contributor Author

Thanks Guys

@cleidigh cleidigh deleted the tree-aria-root-role/add branch January 26, 2019 01:17
@dbaeumer
Copy link
Member

Verified in code.

@dbaeumer dbaeumer added verified Verification succeeded and removed verified Verification succeeded labels Jan 31, 2019
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@roblourens roblourens roblourens approved these changes

@joaomoreno joaomoreno joaomoreno requested changes

Assignees

@joaomoreno joaomoreno

@roblourens roblourens

@cleidigh cleidigh

Labels
accessibility Keyboard, mouse, ARIA, vision, screen readers (non-specific) issues list-widget List widget issues settings-editor VS Code settings editor issues
Projects
None yet
Milestone
December/January 2019
Development

Successfully merging this pull request may close these issues.
4 participants
@cleidigh @dbaeumer @joaomoreno @roblourens