Bug 118104 – Web Inspector: AX: Add container ARIA roles (toolbar, main, labeled regions, etc.) so the layout is more discoverable to screen reader users

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[James Craig]

Web Inspector: AX: Add container ARIA roles (toolbar, main, labeled regions, etc.) so the layout is more discoverable to screen reader users

[Radar WebKit Bug Importer]

<rdar://problem/14283900>

[James Craig]

Created attachment 205582 [details]
patch

[Timothy Hatcher]

Comment on attachment 205582 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=205582&action=review

Thanks for helping with the AX stuff!

> Source/WebInspectorUI/UserInterface/NavigationBar.js:26
> +WebInspector.NavigationBar = function(element, navigationItems, optRole, optLabel) {

We don't prefix optional arguments. WebKit also does not like abbreviations.

> Source/WebInspectorUI/UserInterface/NavigationItem.js:26
> +WebInspector.NavigationItem = function(identifier, optRole, optLabel) {

Ditto

> Source/WebInspectorUI/UserInterface/NavigationSidebarPanel.js:42
> +    this.element.setAttribute("role", "group");
> +    this.element.setAttribute("aria-label", displayName);

This should probially be moved to SidebarPanel.js so it is set for all SidebarPanels, which would include the DetailsSidebarPanels on the right.

> Source/WebInspectorUI/UserInterface/ButtonNavigationItem.js:26
> +WebInspector.ButtonNavigationItem = function(identifier, toolTipOrLabel, image, imageWidth, imageHeight, suppressEmboss, optRole) {

Ditto

> Source/WebInspectorUI/UserInterface/ButtonNavigationItem.js:35
> +    this._element.setAttribute("role", (optRole || "button"));

No need for ().

> Source/WebInspectorUI/UserInterface/Main.js:146
> +    var toolbarElement = document.getElementById("toolbar");
> +    toolbarElement.setAttribute("role", "toolbar");
> +    this.toolbar = new WebInspector.Toolbar(toolbarElement);

A new line between the last two lines would help readability. Or you could do this:

this.toolbar = new WebInspector.Toolbar(document.getElementById("toolbar"));
this.toolbar.element.setAttribute("role", "toolbar");

But why not make the Toolbar class set this in the constructor by default?

> Source/WebInspectorUI/UserInterface/Main.js:152
> +    contentElement.setAttribute("aria-label", WebInspector.UIString("Content"));

Does this match Cocoa and NSWindow?

> Source/WebInspectorUI/UserInterface/Main.js:177
> +    var sidebarElement = document.getElementById("details-sidebar");
> +    sidebarElement.setAttribute("role", "group");
> +    sidebarElement.setAttribute("aria-label", WebInspector.UIString("Details"));
> +    this.rightSidebar = this.detailsSidebar = new WebInspector.Sidebar(sidebarElement, WebInspector.Sidebar.Sides.Right);

I think this would read better if you did:

this.rightSidebar = this.detailsSidebar = new WebInspector.Sidebar(document.getElementById("details-sidebar"), WebInspector.Sidebar.Sides.Right);
this.detailsSidebar.element.setAttribute("role", "group");
this.detailsSidebar.element.setAttribute("aria-label", WebInspector.UIString("Details"));

Though, I think Sidebar's constructor should set the role to "group". And the constructor could have a label parameter like you did for the other classes.

[James Craig]

(In reply to comment #3)

> > Source/WebInspectorUI/UserInterface/Main.js:146
> > +    var toolbarElement = document.getElementById("toolbar");
> > +    toolbarElement.setAttribute("role", "toolbar");
> > +    this.toolbar = new WebInspector.Toolbar(toolbarElement);
> 
> A new line between the last two lines would help readability. Or you could do this:
> 
> this.toolbar = new WebInspector.Toolbar(document.getElementById("toolbar"));
> this.toolbar.element.setAttribute("role", "toolbar");
> 
> But why not make the Toolbar class set this in the constructor by default?

I started out that way, but the Toolbar class is used for more controls than the main toolbar, where the ARIA toolbar role would not be appropriate. I'll update the class to use the optional argument pattern, and comment it so it's more clear.

> > Source/WebInspectorUI/UserInterface/Main.js:152
> > +    contentElement.setAttribute("aria-label", WebInspector.UIString("Content"));
> 
> Does this match Cocoa and NSWindow?

Not sure I understand this question. The equivalent panel in Xcode is just labeled using the filename of the current xcodeproj. I used the "Content" string because this section can contain files, inspector details, log statements, etc. Does that answer your question?

> > Source/WebInspectorUI/UserInterface/Main.js:177
> > +    var sidebarElement = document.getElementById("details-sidebar");
> > +    sidebarElement.setAttribute("role", "group");
> > +    sidebarElement.setAttribute("aria-label", WebInspector.UIString("Details"));
> > +    this.rightSidebar = this.detailsSidebar = new WebInspector.Sidebar(sidebarElement, WebInspector.Sidebar.Sides.Right);
> 
> I think this would read better if you did:
> 
> this.rightSidebar = this.detailsSidebar = new WebInspector.Sidebar(document.getElementById("details-sidebar"), WebInspector.Sidebar.Sides.Right);
> this.detailsSidebar.element.setAttribute("role", "group");
> this.detailsSidebar.element.setAttribute("aria-label", WebInspector.UIString("Details"));
> 
> Though, I think Sidebar's constructor should set the role to "group". And the constructor could have a label parameter like you did for the other classes.

Good idea. I'll make that and the other changes.

[Timothy Hatcher]

Toolbar class is only used for the main toolbar.

[James Craig]

(In reply to comment #5)
> Toolbar class is only used for the main toolbar.

Ah yeah, I was thinking of the NavigationBar superclass. I'll move this to Toolbar.

[Timothy Hatcher]

Toolbar is a subclass of NavigationBar, so you can just have the Toolbar constructor pass the role tool the NavigationBar constructor.

[James Craig]

Created attachment 205617 [details]
patch with review feedback

[Timothy Hatcher]

Comment on attachment 205617 [details]
patch with review feedback

View in context: https://bugs.webkit.org/attachment.cgi?id=205617&action=review

> UserInterface/SidebarPanel.js:42
> +    if (role) 
> +        this._element.setAttribute("role", role);
> +    else 
> +        this._element.setAttribute("role", "group");

this._element.setAttribute("role", role || "group");

> UserInterface/ButtonNavigationItem.js:39
> +    if (role) 
> +        this._element.setAttribute("role", role);
> +    else 
> +        this._element.setAttribute("role", "button");

this._element.setAttribute("role", role || "button");

> UserInterface/Main.js:172
> +    this.rightSidebar = this.detailsSidebar = new WebInspector.Sidebar(document.getElementById("details-sidebar"), WebInspector.Sidebar.Sides.Right, "group", WebInspector.UIString("Details"));

Could pass null for the role. Why not give the navigationSidebar a label too?

[James Craig]

Comment on attachment 205617 [details]
patch with review feedback

Gonna get those last couple changes in an updated patch.

[Timothy Hatcher]

You can just land them when you commit too.

[James Craig]

Created attachment 205622 [details]
patch with review feedback

[WebKit Commit Bot]

Comment on attachment 205622 [details]
patch with review feedback

Rejecting attachment 205622 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 205622, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
oardManager.js
|===================================================================
|--- UserInterface/DashboardManager.js	(revision 152084)
|+++ UserInterface/DashboardManager.js	(working copy)
--------------------------
No file to patch.  Skipping patch.
1 out of 1 hunk ignored
patching file ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Timothy Hatcher']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.appspot.com/results/997168

[James Craig]

Haven't seen that error before. Going to apply the patch clean checkout to see if this was a conflict with another commit, or if this is a commit bot bug.

[James Craig]

Created attachment 205627 [details]
patch with corrected paths

Previous patch was generated from a subdirectory, so the paths were wrong.

[WebKit Commit Bot]

Comment on attachment 205627 [details]
patch with corrected paths

Clearing flags on attachment: 205627

Committed r152141: <http://trac.webkit.org/changeset/152141>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.