Bug 207968

Summary: Web Inspector: VoiceOver: TreeOutline does not correctly indicate selected item
Product: WebKit Reporter: Nikita Vasilyev <nvasilyev>
Component: Web InspectorAssignee: Nikita Vasilyev <nvasilyev>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, commit-queue, inspector-bugzilla-changes, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 209366    
Attachments:
Description Flags
Patch
bburg: review+, bburg: commit-queue-
[Image] Before
none
[Image] With patch applied
none
[Video] With patch applied
none
Patch none

Nikita Vasilyev
Reported 2020-02-19 15:05:18 PST
VoiceOver doesn't correctly read: - The selected DOM element in the Elements tab. - The selected resource in the Sources tab. Currently, it tries to read the entire tree from the start when selection changes. <rdar://59411874>
Attachments
Patch (12.99 KB, patch)
2020-02-19 15:14 PST, Nikita Vasilyev 		bburg: review+
bburg: commit-queue-
DetailsFormatted DiffDiff
[Image] Before (317.08 KB, image/png)
2020-02-19 17:39 PST, Nikita Vasilyev 		no flags
Details
[Image] With patch applied (288.09 KB, image/png)
2020-02-19 17:39 PST, Nikita Vasilyev 		no flags
Details
[Video] With patch applied (6.19 MB, video/quicktime)
2020-02-19 19:25 PST, Nikita Vasilyev 		no flags
Details
Patch (13.04 KB, patch)
2020-02-25 14:28 PST, Nikita Vasilyev 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Nikita Vasilyev
Comment 1 2020-02-19 15:14:02 PST
Created attachment 391208 [details] Patch
Nikita Vasilyev
Comment 2 2020-02-19 15:29:39 PST
My QuickTime isn't working to record a before/after video. Hopefully, I'll figure it out later today.
Nikita Vasilyev
Comment 3 2020-02-19 17:39:31 PST
Created attachment 391228 [details] [Image] Before
Nikita Vasilyev
Comment 4 2020-02-19 17:39:57 PST
Created attachment 391229 [details] [Image] With patch applied
Nikita Vasilyev
Comment 5 2020-02-19 17:40:58 PST
(In reply to Nikita Vasilyev from comment #2) > My QuickTime isn't working to record a before/after video. Hopefully, I'll > figure it out later today. Maybe not today :/ I posted screenshots instead.
Nikita Vasilyev
Comment 6 2020-02-19 19:25:35 PST
Created attachment 391246 [details] [Video] With patch applied
Blaze Burg
Comment 7 2020-02-25 13:43:26 PST
Comment on attachment 391208 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391208&action=review r=me > Source/WebInspectorUI/ChangeLog:8 > + Previously, the entire TreeOutline's DOM element had focus. With this patch, You didn't explain what the code changes do to affect focus, whether real focus or that reported to VO. Is it .ariaSelected, .ariaExpanded, or .tabIndex? > Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.css:64 > +.tree-outline.dom:not(.non-selectable):focus-within li.selected .selection-area { Interesting! Is this relevant to the VO fix, though? > Source/WebInspectorUI/UserInterface/Views/TreeElement.js:-519 > - if (!omitFocus) Any particular reason to reorder these if statements? > Source/WebInspectorUI/UserInterface/Views/TreeElement.js:568 > + this._listItemNode.removeAttribute("tabIndex"); It bothers me that tabIndex is set on focus but cleared on deselect. Can it be set on select more directly? Or can deselect() call an unfocus() method?
Nikita Vasilyev
Comment 8 2020-02-25 14:22:18 PST
Comment on attachment 391208 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391208&action=review >> Source/WebInspectorUI/ChangeLog:8 >> + Previously, the entire TreeOutline's DOM element had focus. With this patch, > > You didn't explain what the code changes do to affect focus, whether real focus or that reported to VO. Is it .ariaSelected, .ariaExpanded, or .tabIndex? By default, the VO cursor is synchronized with the keyboard focus. I made selected TreeElement keyboard focusable by setting tabIndex. Adding ariaSelected and ariaExpanded was just an icing on the cake — VO adds "selected" and "expanded" after reading the item. >> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.css:64 >> +.tree-outline.dom:not(.non-selectable):focus-within li.selected .selection-area { > > Interesting! Is this relevant to the VO fix, though? I've changed what DOM element has the focus from the tree root (.tree-outline) to the selected TreeElement. Basically, `.tree-outline` never has focus any more, but its children do. This CSS change is required to continue displaying which elements are selected. >> Source/WebInspectorUI/UserInterface/Views/TreeElement.js:-519 >> - if (!omitFocus) > > Any particular reason to reorder these if statements? Yes! Already selected TreeElement may not have focus. Calling `select(false)` should focus on the selected TreeElement. Without my change it wouldn't. >> Source/WebInspectorUI/UserInterface/Views/TreeElement.js:568 >> + this._listItemNode.removeAttribute("tabIndex"); > > It bothers me that tabIndex is set on focus but cleared on deselect. Can it be set on select more directly? Or can deselect() call an unfocus() method? I'll add `unfocus` method.
Nikita Vasilyev
Comment 9 2020-02-25 14:28:37 PST
Created attachment 391687 [details] Patch
WebKit Commit Bot
Comment 10 2020-02-25 15:12:52 PST
Comment on attachment 391687 [details] Patch Clearing flags on attachment: 391687 Committed r257380: <https://trac.webkit.org/changeset/257380>
WebKit Commit Bot
Comment 11 2020-02-25 15:12:53 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.