Bug 207968

Created attachment 391208 [details]
Patch

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

Created attachment 391228 [details]
[Image] Before

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

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

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

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?

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.

Created attachment 391687 [details]
Patch

Comment on attachment 391687 [details]
Patch

Clearing flags on attachment: 391687

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

All reviewed patches have been landed.  Closing bug.