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

RESOLVED FIXED in Firefox 49

Status

defect
P3
normal
RESOLVED FIXED
3 years ago
11 months ago

People

(Reporter: yzen, Assigned: yzen)

Tracking

(Depends on 2 bugs, Blocks 1 bug, {access})

unspecified
Firefox 49
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: [btpp-backlog])

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
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)

Updated

3 years ago
Assignee: nobody → yzenevich
Status: NEW → ASSIGNED
Triaging (filter on CLIMBING SHOES).
Priority: -- → P3
Whiteboard: [btpp-backlog]
(Assignee)

Comment 2

3 years ago
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)
(Assignee)

Comment 4

3 years ago
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.
(Assignee)

Comment 5

3 years ago
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
(Assignee)

Updated

3 years ago
Flags: needinfo?(mzehe)
(Assignee)

Comment 6

3 years ago
(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
(Assignee)

Comment 7

3 years ago
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)
(Assignee)

Comment 10

3 years ago
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!
(Assignee)

Updated

3 years ago
Attachment #8750583 - Flags: review?(bgrinstead) → review?(gl)
(Assignee)

Updated

3 years ago
Summary: [a11y] Add proper tree view semantics to inspector. → [a11y] Add proper tree view semantics and keyboard navigation to inspector markup view.
(Assignee)

Comment 12

3 years ago
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)
(Assignee)

Comment 14

3 years ago
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/
(Assignee)

Comment 15

3 years ago
Updated to make rebase with master and fix tests.
(Assignee)

Comment 16

3 years ago
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)
(Assignee)

Comment 18

3 years ago
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+
(Assignee)

Comment 20

3 years ago
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

Comment 23

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cd45f64c6d68
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Depends on: 1280360

Updated

2 years ago
Depends on: 1328003

Updated

2 years ago
Depends on: 1345482

Updated

11 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.