Web Inspector: VoiceOver: TreeOutline does not correctly indicate selected item

[Nikita Vasilyev]

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>

[Nikita Vasilyev]

Created attachment 391208 [details]
Patch

[Nikita Vasilyev]

My QuickTime isn't working to record a before/after video. Hopefully, I'll figure it out later today.

[Nikita Vasilyev]

Created attachment 391228 [details]
[Image] Before

[Nikita Vasilyev]

Created attachment 391229 [details]
[Image] With patch applied

[Nikita Vasilyev]

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

Created attachment 391246 [details]
[Video] With patch applied

[BJ Burg]

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 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]

Created attachment 391687 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 391687 [details]
Patch

Clearing flags on attachment: 391687

Committed r257380: <https://trac.webkit.org/changeset/257380>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.