Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Description • 8 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

	Patrick Brosset <:pbro>
	Comment 1 • 8 years ago

Triaging (filter on CLIMBING SHOES).

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 2 • 8 years ago

Here's the win64 build, Marco:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7b8d81818d5

	Marco Zehe (:MarcoZ)
	Comment 3 • 8 years ago

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;'

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 4 • 8 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.

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 5 • 8 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

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 6 • 8 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

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 7 • 8 years ago

Here's a more polished version, Marco:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0107ddbf28e1

	Marco Zehe (:MarcoZ)
	Comment 8 • 8 years ago

(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.

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 9 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/51495/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51495/

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 10 • 8 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!

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 11 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0e3cf61e819

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 12 • 8 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!

	Gabriel Luong [:gl] (ΦωΦ)
	Comment 13 • 8 years ago

(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.

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 14 • 8 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/

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 15 • 8 years ago

Updated to make rebase with master and fix tests.

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 16 • 8 years ago

Hi Marco, would you give the latest version a try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=35eb824aa51a

	Marco Zehe (:MarcoZ)
	Comment 17 • 8 years ago

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.

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 18 • 8 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/

	Gabriel Luong [:gl] (ΦωΦ)
	Comment 19 • 8 years ago

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.

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 20 • 8 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/

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 21 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7282a25a7ebb

	Pulsebot
	Comment 22 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd45f64c6d68

	Carsten Book [:Tomcat]
	Comment 23 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/cd45f64c6d68