Closed
Bug 1242694
Opened 8 years ago
Closed 7 years ago
[a11y] Add proper tree view semantics and keyboard navigation to inspector markup view.
Categories
(DevTools :: Inspector, defect, P3)
DevTools
Inspector
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 | ||
Updated•8 years ago
|
Assignee: nobody → yzenevich
Status: NEW → ASSIGNED
Comment 1•7 years ago
|
||
Triaging (filter on CLIMBING SHOES).
Priority: -- → P3
Whiteboard: [btpp-backlog]
Assignee | ||
Comment 2•7 years ago
|
||
Here's the win64 build, Marco: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7b8d81818d5
Flags: needinfo?(mzehe)
Comment 3•7 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;'
Flags: needinfo?(mzehe)
Assignee | ||
Comment 4•7 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•7 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•7 years ago
|
Flags: needinfo?(mzehe)
Assignee | ||
Comment 6•7 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•7 years ago
|
||
Here's a more polished version, Marco: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0107ddbf28e1
Comment 8•7 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.
Flags: needinfo?(mzehe)
Assignee | ||
Comment 9•7 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/51495/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/51495/
Attachment #8750583 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 10•7 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•7 years ago
|
Attachment #8750583 -
Flags: review?(bgrinstead) → review?(gl)
Assignee | ||
Updated•7 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 11•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0e3cf61e819
Assignee | ||
Comment 12•7 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)
Comment 13•7 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.
Flags: needinfo?(gl)
Assignee | ||
Comment 14•7 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•7 years ago
|
||
Updated to make rebase with master and fix tests.
Assignee | ||
Comment 16•7 years ago
|
||
Hi Marco, would you give the latest version a try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=35eb824aa51a
Flags: needinfo?(mzehe)
Comment 17•7 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.
Flags: needinfo?(mzehe)
Assignee | ||
Comment 18•7 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 19•7 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.
Attachment #8750583 -
Flags: review?(gl) → review+
Assignee | ||
Comment 20•7 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
Assignee | ||
Comment 21•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7282a25a7ebb
Comment 23•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/cd45f64c6d68
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Updated•5 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•