1438912 - [VirtualizedTree] Do not allow navigation to next node on ArrowRight key, only allow expand on expandable node.

Status: VERIFIED FIXED

(DevTools :: Shared Components, defect)

Description

[Carmen Fat [:cfat] - Ecosystem QA]

Attachment: right arrow.gif

[Affected versions]:
- Nightly 60.0a1 (2018-02-15) try build 

[Affected Platforms]:
- All Windows
- All Mac
- All Linux

[Prerequisites]:
- Have the latest try build 60.0a1 from (2018-02-15) installed.
- Accessibility tab is enabled and focused.

[Steps to reproduce]:
1. Click on the first line from the "Properties" panel.
2. Press the "Right arrow" key twice and observe what happens next.

[Expected Results]:
- Nothing happens, the "Right arrow" key is not used to navigate through lines.

[Actual results]:
- Next line is selected.

[Notes]:
- After navigating through all lines from the panel, the user is able to press only one time the key in order to continue the navigation.  
- This is not reproducible on the "Role" and "Name" panel.
- Attached a screen recording with the issue.

Comment 1

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1438912 patch

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Isn't it something we could keep ?
I think it was done because the Tree is used in the Memory panel, where you can have a lot of nested elements.
So you can explore the Tree quite easily by only hitting one key (this is also what perf-html does).
Without it, you need to play with <kbd>↓</kbd> and <kbd>→</kbd>, which can be a bit cumbersome.

Comment 3

[Julien Wajsberg [:julienw]]

Note: I'm working on a patch for perf where the right arrow key will both expand and move in one key (while at the moment we need 2 key events).

Comment 4

[Marco Zehe (:MarcoZ)]

(In reply to Julien Wajsberg [:julienw] from comment #3)
> Note: I'm working on a patch for perf where the right arrow key will both
> expand and move in one key (while at the moment we need 2 key events).

Please don't do that. Screen readers announce the changed state from collapsed to expanded, and if this is followed by an immediate focus event, that announcement will be nuked by the stuff that gets spoken with the focus event. Also, this is unlike any other tree control I've ever seen and will introduce more confusion than actual benefits.

I can compromise on keeping the RightArrow behavior for navigating to the next node. I just tested again with native Windows controls like in Windows Explorer, and it behaves a bit inconsistently. E. g. it moves down into the children upon an expanded node, but if the node is non-expandable, RightArrow doesn't do anything. But what it NEVER does is change the focus and expand items at the same time.

Comment 5

[Julien Wajsberg [:julienw]]

> Please don't do that. Screen readers announce the changed state from collapsed to expanded, and if this is followed by an immediate focus event, that announcement will be nuked by the stuff that gets spoken with the focus event. Also, this is unlike any other tree control I've ever seen and will introduce more confusion than actual benefits.

In that patch we do a lot more than just this actually, also expanding until the first "branch". I believe this can be very useful for sighted users, but I understand how it can be a problem for users of assistance technology. Please follow along in [1] for perf.html and let's focus on devtools' tree here.

[1] https://github.com/devtools-html/perf.html/issues/517

Comment 6

[Yura Zenevich [:yzen] (please use "needinfo")]

It feels like something to good to have to be configurable. Would perhaps having those key event handler behaviors exposed through props with the default one being "do not move on right just expand if expandable" be a good compromise?

Comment 7

[Marco Zehe (:MarcoZ)]

SGTM.

Comment 8

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1438912 patch v2

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8955258 [details] [diff] [review]
1438912 patch v2

Review of attachment 8955258 [details] [diff] [review]:
-----------------------------------------------------------------

Could we be more explicit about what's going on with left and right arrow keys ?
I think it would be a simpler move (and one that does not change current usage) to have a `preventNavigationOnArrowLeftRight` prop, that we would check before doing the actual `focusParentNode`/`focusNextNode` calls.

What do you think ?

Comment 10

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #9)
> Comment on attachment 8955258 [details] [diff] [review]
> 1438912 patch v2
> 
> Review of attachment 8955258 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Could we be more explicit about what's going on with left and right arrow
> keys ?
> I think it would be a simpler move (and one that does not change current
> usage) to have a `preventNavigationOnArrowLeftRight` prop, that we would
> check before doing the actual `focusParentNode`/`focusNextNode` calls.
> 
> What do you think ?

I think this still does not resolve the fact that the default behavior would remain the same (and thus confusing for keyboard/screen reader users). I think the idea of that prop (instead of override is fine, as long as we can settle on what default value is, I would vote for it to be true by default.

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

Yes, I agree to make it default.
The only thing is to set it to false in performance and memory panel

Comment 12

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1438912 patch v3

I realized that we do want navigate to parent on arrow left so this is specific to navigate to next node (arrow right) only.

Comment 13

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8957309 [details] [diff] [review]
1438912 patch v3

Review of attachment 8957309 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. Thanks Yura

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/af023ff0275b
by default, do not navigate to next node on ArrowRight key. r=nchevobbe

Comment 15

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c68b341cf14
Backed out changeset af023ff0275b for merge conflicts on a CLOSED TREE

Comment 16

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/d54b35a3194d
Backed out changeset af023ff0275b for merge conflicts

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/adcccd4e15f8
by default, do not navigate to next node on ArrowRight key. r=nchevobbe

Comment 18

[Andreea Pavel [:apavel]]

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

Comment 19

[Timea Cernea [:tbabos][inactive]]

On the latest Firefox Nightly (Build ID: 20180426220144) the issue cannot be reproduced on Mac OS X 10.12, Windows 10 x64 and Ubuntu 16.04 x64, thus I will close the issue as VERIFIED FIXED.