Marco Zehe (:MarcoZ) Reporter
	Description • 7 years ago

This is with the Feb 15 try build.

Using NVDA on Windows:
1. Focus a link.
2. Press Applications Key (the key with the popup) to open context menu.
3. Select Inspect Accessibility from the menu and press Enter.

Expected: Panel should open, and focus should be on the Name property of the link.
Actual: Panel opens, but focus lands on the tree instead of the focused item.

4. Press DownArrow.

Expected: Focus should move to first item.
Actual: Nothing happens.

5. Press Shift-Tab.

Result: Focus lands on the tree of accessible objects, and the link is highlighted.

6. Press Tab.

Result: Now, focus is on the Nae item of the link's properties. This is the state I would expect to be in after step 3 above.

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 1 • 7 years ago

There are still cases when the focus lands on the VirtualizedTree (like the one in summary) where relying on event targets is not reliable to decide whether to focus on the first node when no nodes are focused.

	Nicolas Chevobbe [:nchevobbe]
	Comment 3 • 6 years ago

Comment on attachment 8953596 [details] [diff] [review]
1438835 patch

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

Thanks yzen, this looks good. I only have a couple of comment and one question: 
could we end up in a bad state if the user mousedown on the tree, hold the click down, move the mouse outside the Tree and release the mouse button ? I don't know if the mouseup listener would be called in this case.

::: devtools/client/shared/components/VirtualizedTree.js
@@ +280,5 @@
> +        seen !== nextState.seen) {
> +      return true;
> +    }
> +
> +    if (mouseDown !== nextState.mouseDown) {

could we put this in the previous if with `mouseDown === nextState.mouseDown` ?

@@ +284,5 @@
> +    if (mouseDown !== nextState.mouseDown) {
> +      return false;
> +    }
> +
> +    return true;

We should not update by default, but only on specified cases

@@ +345,5 @@
>    /**
>     * Updates the state's height based on clientHeight.
>     */
>    _updateHeight() {
> +    this.setState({ height: this.refs.tree.clientHeight });

nice

::: devtools/client/shared/components/test/mochitest/test_tree_05.html
@@ +18,5 @@
>  <script type="application/javascript">
>  
> +"use strict";
> +
> +window.onload = async function () {

thanks for doing that

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 4 • 6 years ago

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #3)
> Comment on attachment 8953596 [details] [diff] [review]
> 1438835 patch
> 
> Review of attachment 8953596 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks yzen, this looks good. I only have a couple of comment and one
> question: 
> could we end up in a bad state if the user mousedown on the tree, hold the
> click down, move the mouse outside the Tree and release the mouse button ? I
> don't know if the mouseup listener would be called in this case.
> 
> 
> @@ +284,5 @@
> > +    if (mouseDown !== nextState.mouseDown) {
> > +      return false;
> > +    }
> > +
> > +    return true;
> 
> We should not update by default, but only on specified cases

Do you mean you'd like me to compare props here as well? I was just assuming that only states would be handled more granularly?

	Nicolas Chevobbe [:nchevobbe]
	Comment 5 • 6 years ago

I mean doing: 

```js
return (
  scroll !== nextState.scroll ||
  height !== nextState.height ||
  seen !== nextState.seen ||
  mouseDown === nextState.mouseDown
);
```
	
So it's obvious when we do updates.

Also, did you checked: 

> could we end up in a bad state if the user mousedown on the tree, hold the click down, move the mouse outside the Tree and release the mouse button ? I don't know if the mouseup listener would be called in this case.

?

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 6 • 6 years ago

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #3)
> Comment on attachment 8953596 [details] [diff] [review]
> 1438835 patch
> 
> Review of attachment 8953596 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks yzen, this looks good. I only have a couple of comment and one
> question: 
> could we end up in a bad state if the user mousedown on the tree, hold the
> click down, move the mouse outside the Tree and release the mouse button ? I
> don't know if the mouseup listener would be called in this case.
> 

Yes, I've verified that mouseup is fired in this case.

	Nicolas Chevobbe [:nchevobbe]
	Comment 8 • 6 years ago

Comment on attachment 8955181 [details] [diff] [review]
1438835 patch v2

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

Thanks yzen, it looks good to me. r+ with a green try

::: devtools/client/shared/components/VirtualizedTree.js
@@ +274,5 @@
>  
> +  shouldComponentUpdate(nextProps, nextState) {
> +    let { scroll, height, seen, mouseDown } = this.state;
> +
> +    return scroll !== nextState.scroll || height !== nextState.height ||

not: could we have one test by line ? it would be a bit easier to read

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 9 • 6 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c10541a55920678746ee7868c99530523d386d46

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 10 • 6 years ago

(In reply to Yura Zenevich [:yzen] from comment #9)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=c10541a55920678746ee7868c99530523d386d46

The only failures are due to bug 1438912 changes.

	Pulsebot
	Comment 11 • 6 years ago

Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3eef48c0a0d9
[VirtualizedTree] keep inner state for when the tree is moused down, to help with deciding when to keyboard focus on tree container. r=nchevobbe

	Cosmin Sabou [:CosminS]
	Comment 12 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/3eef48c0a0d9