Closed
Bug 1438835
Opened 7 years ago
Closed 7 years ago
[VirtualizedTree]: Make keyboard vs mouse activation of VirtualizedTree container more resilient.
Categories
(DevTools :: Shared Components, defect)
DevTools
Shared Components
Tracking
(firefox60 fixed)
RESOLVED
FIXED
Firefox 60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: MarcoZ, Assigned: yzen)
References
Details
(Keywords: access)
Attachments
(1 file, 1 obsolete file)
9.45 KB,
patch
|
nchevobbe
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•7 years ago
|
Component: Developer Tools → Developer Tools: Accessibility Tools
Assignee | ||
Updated•7 years ago
|
Component: Developer Tools: Accessibility Tools → Developer Tools: Shared Components
Summary: Accessibility Inspector: Initial keyboard focus after selecting Inspect Accessibility from an element's context menu is inconsistent → [VirtualizedTree]: Make keyboard vs mouse activation of VirtualizedTree container more resilient.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → yzenevich
Status: NEW → ASSIGNED
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.
Assignee | ||
Comment 2•7 years ago
|
||
Attachment #8953596 -
Flags: review?(nchevobbe)
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
Flags: needinfo?(yzenevich)
Assignee | ||
Comment 4•7 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?
Flags: needinfo?(yzenevich) → needinfo?(nchevobbe)
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.
?
Flags: needinfo?(nchevobbe)
Assignee | ||
Comment 6•7 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.
Assignee | ||
Comment 7•7 years ago
|
||
Attachment #8953596 -
Attachment is obsolete: true
Attachment #8953596 -
Flags: review?(nchevobbe)
Attachment #8955181 -
Flags: review?(nchevobbe)
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
Attachment #8955181 -
Flags: review?(nchevobbe) → review+
Assignee | ||
Comment 9•7 years ago
|
||
Assignee | ||
Comment 10•7 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.
Comment 11•7 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
Comment 12•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•