Bug 118104 - Web Inspector: AX: Add container ARIA roles (toolbar, main, labeled regions, etc.) so the layout is more discoverable to screen reader users
Summary: Web Inspector: AX: Add container ARIA roles (toolbar, main, labeled regions, ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Craig
URL:
Keywords: InRadar
Depends on:
Blocks: 118105
  Show dependency treegraph
 
Reported: 2013-06-26 17:15 PDT by James Craig
Modified: 2013-06-27 17:18 PDT (History)
6 users (show)

See Also:


Attachments
patch (12.55 KB, patch)
2013-06-27 02:42 PDT, James Craig
timothy: review-
Details | Formatted Diff | Diff
patch with review feedback (13.74 KB, patch)
2013-06-27 10:06 PDT, James Craig
timothy: review+
jcraig: commit-queue-
Details | Formatted Diff | Diff
patch with review feedback (14.95 KB, patch)
2013-06-27 12:02 PDT, James Craig
timothy: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
patch with corrected paths (15.79 KB, patch)
2013-06-27 13:27 PDT, James Craig
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Craig 2013-06-26 17:15:21 PDT
Web Inspector: AX: Add container ARIA roles (toolbar, main, labeled regions, etc.) so the layout is more discoverable to screen reader users
Comment 1 Radar WebKit Bug Importer 2013-06-26 17:15:36 PDT
<rdar://problem/14283900>
Comment 2 James Craig 2013-06-27 02:42:58 PDT
Created attachment 205582 [details]
patch
Comment 3 Timothy Hatcher 2013-06-27 03:43:52 PDT
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.
Comment 4 James Craig 2013-06-27 09:05:46 PDT
(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.
Comment 5 Timothy Hatcher 2013-06-27 09:12:49 PDT
Toolbar class is only used for the main toolbar.
Comment 6 James Craig 2013-06-27 09:37:13 PDT
(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.
Comment 7 Timothy Hatcher 2013-06-27 09:50:33 PDT
Toolbar is a subclass of NavigationBar, so you can just have the Toolbar constructor pass the role tool the NavigationBar constructor.
Comment 8 James Craig 2013-06-27 10:06:17 PDT
Created attachment 205617 [details]
patch with review feedback
Comment 9 Timothy Hatcher 2013-06-27 10:33:00 PDT
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?
Comment 10 James Craig 2013-06-27 11:37:03 PDT
Comment on attachment 205617 [details]
patch with review feedback

Gonna get those last couple changes in an updated patch.
Comment 11 Timothy Hatcher 2013-06-27 11:39:56 PDT
You can just land them when you commit too.
Comment 12 James Craig 2013-06-27 12:02:47 PDT
Created attachment 205622 [details]
patch with review feedback
Comment 13 WebKit Commit Bot 2013-06-27 12:11:12 PDT
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
Comment 14 James Craig 2013-06-27 13:07:56 PDT
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.
Comment 15 James Craig 2013-06-27 13:27:51 PDT
Created attachment 205627 [details]
patch with corrected paths

Previous patch was generated from a subdirectory, so the paths were wrong.
Comment 16 WebKit Commit Bot 2013-06-27 17:17:58 PDT
Comment on attachment 205627 [details]
patch with corrected paths

Clearing flags on attachment: 205627

Committed r152141: <http://trac.webkit.org/changeset/152141>
Comment 17 WebKit Commit Bot 2013-06-27 17:18:01 PDT
All reviewed patches have been landed.  Closing bug.