Closed Bug 1242694 Opened 5 years ago Closed 4 years ago

[a11y] Add proper tree view semantics and keyboard navigation to inspector markup view.

Categories

(DevTools :: Inspector, defect, P3)

defect

Tracking

(firefox49 fixed)

RESOLVED FIXED
Firefox 49
Tracking Status
firefox49 --- fixed

People

(Reporter: yzen, Assigned: yzen)

References

(Depends on 2 open bugs, Blocks 1 open bug)

Details

(Keywords: access, Whiteboard: [btpp-backlog])

Attachments

(1 file)

Right now the main inspector view is lacking a tree view semantics to represent the DOM tree. A good example of how it is implemented is Firebug (full details here http://getfirebug.com/wiki/index.php/Accessibility).

Among other things we need to:
* starting using outline and outline item roles appropriately for current tag items and their ancestry
* track and expose the tree nesting level correctly (using aria-level)
* track and apply expanded and collapsed states for tree items
Assignee: nobody → yzenevich
Status: NEW → ASSIGNED
Triaging (filter on CLIMBING SHOES).
Priority: -- → P3
Whiteboard: [btpp-backlog]
Here's the win64 build, Marco:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7b8d81818d5
Flags: needinfo?(mzehe)
This looks pretty good, with one problem: The keyboard focus always lands on the first child of the actual tree view item, not the tree view item itself. The result is that NVDA only says "Textframe" when navigating. So, it *should* land on an element like this (got this from NVDA+F1 while navigator object was on the tree item):

Developer info for navigator object:
name: u'<textarea id="bufferInputView1634" style="height: 20px; overflow: hidden;" name="msg" rows="1"></textarea>ev'
role: ROLE_TREEVIEWITEM
states: STATE_SELECTABLE, STATE_SELECTED
isFocusable: False
hasFocus: False
Python object: <NVDAObjects.Dynamic_OutlineItemMozillaIAccessible object at 0x0502EDF0>
Python class mro: (<class 'NVDAObjects.Dynamic_OutlineItemMozillaIAccessible'>, <class 'NVDAObjects.IAccessible.OutlineItem'>, <class 'NVDAObjects.IAccessible.mozilla.Mozilla'>, <class 'NVDAObjects.IAccessible.ia2Web.Ia2Web'>, <class 'NVDAObjects.IAccessible.IAccessible'>, <class 'NVDAObjects.window.Window'>, <class 'NVDAObjects.NVDAObject'>, <class 'baseObject.ScriptableObject'>, <class 'baseObject.AutoPropertyObject'>, <type 'object'>)
description: u''
location: (720, 1679, 1622, 34)
value: None
appModule: <'firefox' (appName u'firefox', process ID 9184) at address 50c2590>
appModule.productName: u'Nightly'
appModule.productVersion: u'48.0a1'
TextInfo: <class 'NVDAObjects.IAccessible.IA2TextTextInfo'>
windowHandle: 3211588L
windowClassName: u'MozillaWindowClass'
windowControlID: 0
windowStyle: 399441920
windowThreadID: 2152
windowText: u'* #accessibility | Mozilla - Nightly'
displayText: u''
IAccessibleObject: <POINTER(IAccessible2) ptr=0xc0099fc at 5754a30>
IAccessibleChildID: 0
IAccessible event parameters: windowHandle=3211588L, objectID=-4, childID=-955
IAccessible accName: u'<textarea id="bufferInputView1634" style="height: 20px; overflow: hidden;" name="msg" rows="1"></textarea>ev'
IAccessible accRole: ROLE_SYSTEM_OUTLINEITEM
IAccessible accState: STATE_SYSTEM_SELECTABLE, STATE_SYSTEM_SELECTED, STATE_SYSTEM_VALID (2097154)
IAccessible accDescription: u''
IAccessible accValue: None
IAccessible2 windowHandle: 3211588
IAccessible2 uniqueID: -955
IAccessible2 role: ROLE_SYSTEM_OUTLINEITEM
IAccessible2 states: IA2_STATE_SELECTABLE_TEXT, IA2_STATE_OPAQUE (5120)
IAccessible2 attributes: u'margin-left:0px;level:30;text-indent:0px;setsize:2;formatting:block;margin-right:0px;xml-roles:treeitem;tag:div;class:tag-line;margin-top:0px;text-align:left;posinset:2;margin-bottom:0px;display:block;'

Keyboard focus instead lands on this item:

Developer info for navigator object:
name: None
role: ROLE_TEXTFRAME
states: 
isFocusable: False
hasFocus: False
Python object: <NVDAObjects.IAccessible.mozilla.Mozilla object at 0x050AA510>
Python class mro: (<class 'NVDAObjects.IAccessible.mozilla.Mozilla'>, <class 'NVDAObjects.IAccessible.ia2Web.Ia2Web'>, <class 'NVDAObjects.IAccessible.IAccessible'>, <class 'NVDAObjects.window.Window'>, <class 'NVDAObjects.NVDAObject'>, <class 'baseObject.ScriptableObject'>, <class 'baseObject.AutoPropertyObject'>, <type 'object'>)
description: u''
location: (-23280, 1684, 25622, 34)
value: None
appModule: <'firefox' (appName u'firefox', process ID 9184) at address 50c2590>
appModule.productName: u'Nightly'
appModule.productVersion: u'48.0a1'
TextInfo: <class 'NVDAObjects.IAccessible.IA2TextTextInfo'>
windowHandle: 3211588L
windowClassName: u'MozillaWindowClass'
windowControlID: 0
windowStyle: 399441920
windowThreadID: 2152
windowText: u'* #accessibility | Mozilla - Nightly'
displayText: u''
IAccessibleObject: <POINTER(IAccessible2) ptr=0xc1507fc at 52806c0>
IAccessibleChildID: 0
IAccessible event parameters: windowHandle=3211588L, objectID=-4, childID=-839
IAccessible accName: None
IAccessible accRole: u'span'
IAccessible accState: STATE_SYSTEM_FLOATING, STATE_SYSTEM_VALID (4096)
IAccessible accDescription: u''
IAccessible accValue: None
IAccessible2 windowHandle: 3211588
IAccessible2 uniqueID: -839
IAccessible2 role: IA2_ROLE_TEXT_FRAME
IAccessible2 states: IA2_STATE_OPAQUE (1024)
IAccessible2 attributes: u'margin-left:0px;text-align:left;text-indent:0px;formatting:block;margin-right:0px;tag:span;class:tag-state theme-selected;margin-top:0px;margin-bottom:0px;display:block;'
Flags: needinfo?(mzehe)
Here're a keyboard interaction guidelines from Marco:

[10:28:15]  <&MarcoZ>	a) Arrows up and down walk among nodes. Right opens, left closes.
[10:28:30]  <&MarcoZ>	b) Enter steps into the selected tree item, putting focus on the tag.
[10:28:44]  <&MarcoZ>	c) Tab moves among the tag and attributes *only for that item, not the whole tree*.
[10:28:49]  <&MarcoZ>	d) Escape backs up.
[10:29:14]  <&MarcoZ>	E) tab and shift tab move at the tree level to the next or previous widget. So one has to start interacting with the individual nodes by enter, escaping out when done.
Hi Marco, I think keyboard a11y should be in fairly good shape. My one concern is if I need to set more presentation roles for all the containers that are present inside each treeitem. Here's the build for Win64:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c83ae37de04c
Flags: needinfo?(mzehe)
(In reply to Yura Zenevich [:yzen] from comment #5)
> Hi Marco, I think keyboard a11y should be in fairly good shape. My one
> concern is if I need to set more presentation roles for all the containers
> that are present inside each treeitem. Here's the build for Win64:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=c83ae37de04c

In terms of tabbing through attributes, there are still nameless text nodes but are activatable via space or return keys. They then become input fields for editing. It is not exactly contentEditable case as they are not editable by default (when starting to tab) and are more like buttons that activate attribute editing. Would you have any suggestions? Thanks
Here's a more polished version, Marco:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0107ddbf28e1
(In reply to Yura Zenevich [:yzen] from comment #6)
> In terms of tabbing through attributes, there are still nameless text nodes
> but are activatable via space or return keys. They then become input fields
> for editing. It is not exactly contentEditable case as they are not editable
> by default (when starting to tab) and are more like buttons that activate
> attribute editing. Would you have any suggestions? Thanks
Yes, plain and simple: Make them a role of "button". :-) You override the name from sub tree with the aria-label for the treeview item itself anyway, so the children might as well be text nodes as well as ARIA buttons. They behave like buttons now, so having them announce as "edit" as is now is irritating.

But aside from that, this now works exactly how I would want it to work! Fantastic job, Yura!

I believe this model can be applied to the other panels that have similar behavior, too, like the CSS rules and other sub panels in that inspector panel.
Flags: needinfo?(mzehe)
Hi Brian, not sure if you're the right person for review, let me know if I need to flip the flag onto someone else. Thanks!
Attachment #8750583 - Flags: review?(bgrinstead) → review?(gl)
Summary: [a11y] Add proper tree view semantics to inspector. → [a11y] Add proper tree view semantics and keyboard navigation to inspector markup view.
Hi Gabriel, would you have some time to take a look at this patch? If not I can forward it to someone else. Thanks!
Flags: needinfo?(gl)
(In reply to Yura Zenevich [:yzen] from comment #12)
> Hi Gabriel, would you have some time to take a look at this patch? If not I
> can forward it to someone else. Thanks!

Sorry for the not getting to this sooner - I was away for onboarding. I will look it later today.
Flags: needinfo?(gl)
Comment on attachment 8750583 [details]
MozReview Request: Bug 1242694 - improving inspector markup view accessibility (semantics and keyboard). r=gl

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51495/diff/1-2/
Updated to make rebase with master and fix tests.
Hi Marco, would you give the latest version a try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=35eb824aa51a
Flags: needinfo?(mzehe)
This latest build still works as expected. I assume the breadcrumb navigation is being dealt with in a separate bug, right? Because in there, left and right don't work, up does not do anything, either, but DownArrow moves focus to the tab list of the sub panels, onto the Rules tab in my case.
Flags: needinfo?(mzehe)
Comment on attachment 8750583 [details]
MozReview Request: Bug 1242694 - improving inspector markup view accessibility (semantics and keyboard). r=gl

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51495/diff/2-3/
Comment on attachment 8750583 [details]
MozReview Request: Bug 1242694 - improving inspector markup view accessibility (semantics and keyboard). r=gl

https://reviewboard.mozilla.org/r/51495/#review50592

::: devtools/client/inspector/markup/markup.js:1948
(Diff revision 2)
> +   */
> +  set canFocus(value) {
> +    if (this._canFocus === value) {
> +      return;
> +    }
> +    this._canFocus = value;

Add a space between 'this._canFocus = value' to improve readability.

::: devtools/client/inspector/markup/markup.js:1964
(Diff revision 2)
> +  /**
> +   * If conatiner and its contents are focusable, exclude them from tab order,
> +   * and, if necessary, remove focus.
> +   */
> +  clearFocus: function () {
> +    if (!this.canFocus) {

Same comment as above, let's add a new line before or after to separate if / while blocks

::: devtools/client/inspector/markup/markup.js:2180
(Diff revision 2)
>  
> +  /**
> +   * Move keyboard focus to a next/previous focusable element inside container
> +   * that is not part of its children (only if current focus is on first or last
> +   * element).
> +   * @param  {DOMNode} current  currently focused element

Add a new line to separate the comment and @param

::: devtools/client/inspector/markup/markup.js:2205
(Diff revision 2)
> +    let {target, keyCode, shiftKey} = event;
> +    let isInput = this.markup._isInputOrTextarea(target);
> +
> +    // Ignore all keystrokes that originated in editors except for when 'Tab' is
> +    // pressed.
> +    if (isInput && keyCode !== Ci.nsIDOMKeyEvent.DOM_VK_TAB) {

I typically see us do event.DOM_VK_123. Can you try to see if we can do that here as well?

::: devtools/client/inspector/markup/test/browser_markup_accessibility_focus_blur.js:11
(Diff revision 2)
> +
> +// Test inspector markup view handling focus and blur when moving between markup
> +// view, its root and other containers, and other parts of inspector.
> +
> +add_task(function* () {
> +  info("Loading the test document and opening the inspector");

Remove this info() since it is repeating what the function call openInspectorForURL is saying.

::: devtools/client/inspector/markup/test/browser_markup_accessibility_focus_blur.js:24
(Diff revision 2)
> +  let rootContainer = markup.getContainer(markup._rootNode);
> +
> +  is(doc.activeElement, doc.body,
> +    "Keyboard focus by default is on document body");
> +
> +  info("Selecting the test span node");

Remove this info()

::: devtools/client/inspector/markup/test/browser_markup_accessibility_focus_blur.js:30
(Diff revision 2)
> +  yield selectNode("span", inspector);
> +
> +  is(doc.activeElement, doc.body,
> +    "Keyboard focus is still on document body");
> +
> +  info("Focusing on the test span node usign 'Return' key");

I think you meant to spell 'using', but it turned out to be 'usign'.

::: devtools/client/inspector/markup/test/browser_markup_accessibility_focus_blur.js:50
(Diff revision 2)
> +  yield selectNode("span", inspector);
> +
> +  is(doc.activeElement, doc.body,
> +    "Keyboard focus should again be on document body");
> +
> +  info("Focusing on the test span node usign 'Space' key");

spelling: usign

::: devtools/client/inspector/markup/test/browser_markup_accessibility_navigation.js:244
(Diff revision 2)
> +
> +let elms = {};
> +let containerID = 0;
> +
> +add_task(function* () {
> +  info("Loading the test document and opening the inspector");

Remove info()

::: devtools/client/inspector/markup/test/browser_markup_accessibility_semantics.js:13
(Diff revision 2)
> +// updated.
> +
> +const TOP_CONTAINER_LEVEL = 3;
> +
> +add_task(function* () {
> +  info("Loading the test document and opening the inspector");

Remove info()

::: devtools/client/inspector/test/head.js:150
(Diff revision 2)
> + * @param {String|NodeFront} selector
> + * @param {InspectorPanel} inspector The current inspector-panel instance.
> + * @return {MarkupContainer}
> + */
> +function* focusNode(selector, inspector) {
> +  inspector.markup.getContainer(inspector.markup._rootNode).elt.focus();

Looking at the other rootNode getters, I suspect we can possibly do inspector.walker.rootNode. In addition, we have a helper function getContainerForNodeFront that calls getContainer().

So, maybe getContainerForNodeFront(inspector.walker.rootNode, inspector).elt.focus() would work here.
Attachment #8750583 - Flags: review?(gl) → review+
Comment on attachment 8750583 [details]
MozReview Request: Bug 1242694 - improving inspector markup view accessibility (semantics and keyboard). r=gl

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51495/diff/3-4/
Attachment #8750583 - Attachment description: MozReview Request: Bug 1242694 - improving inspector markup view accessibility (semantics and keyboard). → MozReview Request: Bug 1242694 - improving inspector markup view accessibility (semantics and keyboard). r=gl
https://hg.mozilla.org/mozilla-central/rev/cd45f64c6d68
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Depends on: 1280360
Depends on: 1328003
Depends on: 1345482
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.