828987 - The Variables View should be keyboard accessible

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Victor Porof [:vporof][:vp]]

It's currently not.

Comment 1

[Victor Porof [:vporof][:vp]]

Attachment: v1

This was easier than I thought. Needs a test.

Comment 2

[Victor Porof [:vporof][:vp]]

Attachment: v2

Added keyboard shortcuts for selecting the variables view.

Comment 3

[Victor Porof [:vporof][:vp]]

Attachment: v3

Added tests.

Comment 4

[Victor Porof [:vporof][:vp]]

Attachment: v3.1

Fixed oranges.
Try: https://tbpl.mozilla.org/?tree=Try&rev=5726aa5debd3

Comment 5

[Mihai Sucan [:msucan]]

This is awesome work. Just some comments:

1. when I press Enter on a non-expandable property I expect I can edit the value. Currently, Enter goes to the next property (like key Down). Maybe Shift-Enter could start edit for property name.

2. when I press Left on a property that has a collapsible parent, I expect to go to the parent object, and collapse. Currently Left does the same as Up (go to the previous property).

Comment 6

[Mihai Sucan [:msucan]]

Also, Delete loses focus from the variables view. When I press Delete focus moves to the debugger source view.

Comment 7

[Mihai Sucan [:msucan]]

When I use Page Up/Down the scrolling works correctly, but then if I use Up/Down to do finer scrolling, vview uses the last scroll position it had when I used Up/Down - it doesn't continue scrolling from the new location I went to with Page Up/Down. This breaks user's habit.

Comment 8

[Victor Porof [:vporof][:vp]]

Attachment: v4

Addressed Mihai's comments, except the issue of refocusing a variable after pressing delete. When deleting (or renaming, or changing the value of) a variable, complex behaviors may or may not occur, depending on the client implementation of the variables view. This, combined with the fact that lazy emptying clears the whole view in favor of a new container node, makes refocusing the-right-thing a complex problem. Let's leave this for a followup if anyone complains.

Comment 9

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 707212 [details] [diff] [review]
v4

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

When focusing the variables view I expect the selected node to scroll into view if not visible, otherwise there is no visual feedback. My test is to focus on a variable, scroll down so that the innermost scope is no longer visible in the pane, click on the source editor and then trigger the shortcut to focus the variables view again. Another way to see the problem is to navigate with the arrow keys downwards until the first item in the tree is hidden, and then press the Home key.

r=me with this fixed.

::: browser/devtools/shared/VariablesView.jsm
@@ +13,5 @@
>  const LAZY_EXPAND_DELAY = 50; // ms
>  const LAZY_APPEND_DELAY = 100; // ms
>  const LAZY_APPEND_BATCH = 100; // nodes
> +const PAGE_SIZE_SCROLL_HEIGHT_RATIO = 200;
> +const PAGE_SIZE_MAX_JUMPS = 30;

This feels a bit small in my testing (only 3 lines). I think it would be best to get page up/down to focus the first/last visible item in the tree after scrolling it to the end/start of the variables pane.

@@ +728,5 @@
> +          PAGE_SIZE_SCROLL_HEIGHT_RATIO),
> +          PAGE_SIZE_MAX_JUMPS);
> +
> +        while (jumps--) {
> +          this.focusPrevItem();

It would be nice if we could focus the right item directly, but I don't see any flickering, so I'm fine with this, too.

@@ +967,5 @@
> +   * @return boolean
> +   *         True if the specified item is a direct child, false otherwise.
> +   */
> +  isChildOf: function S_isChildOf(aParent) {
> +    return this.ownerView == aParent;

Wouldn't you feel safer with strict equality checking in a case where multiple types are expected?

Comment 10

[Victor Porof [:vporof][:vp]]

> 
> ::: browser/devtools/shared/VariablesView.jsm
> @@ +13,5 @@
> >  const LAZY_EXPAND_DELAY = 50; // ms
> >  const LAZY_APPEND_DELAY = 100; // ms
> >  const LAZY_APPEND_BATCH = 100; // nodes
> > +const PAGE_SIZE_SCROLL_HEIGHT_RATIO = 200;
> > +const PAGE_SIZE_MAX_JUMPS = 30;
> 
> This feels a bit small in my testing (only 3 lines). I think it would be
> best to get page up/down to focus the first/last visible item in the tree
> after scrolling it to the end/start of the variables pane.
> 

The amount of jumped lines depends on the total height of the variables view (aka total number of displayed variables and properties). If you'd expand this and the window scope, then there would be ~25 or 30 nodes jumped on each Page Down.

> @@ +728,5 @@
> > +          PAGE_SIZE_SCROLL_HEIGHT_RATIO),
> > +          PAGE_SIZE_MAX_JUMPS);
> > +
> > +        while (jumps--) {
> > +          this.focusPrevItem();
> 
> It would be nice if we could focus the right item directly, but I don't see
> any flickering, so I'm fine with this, too.
> 

Yeah, unfortunately nsIDOMXULCommandDispatcher doesn't provide such an API :(
(and I would like to stick to that implementation instead of redefining my own focus-queue-structure).

> @@ +967,5 @@
> > +   * @return boolean
> > +   *         True if the specified item is a direct child, false otherwise.
> > +   */
> > +  isChildOf: function S_isChildOf(aParent) {
> > +    return this.ownerView == aParent;
> 
> Wouldn't you feel safer with strict equality checking in a case where
> multiple types are expected?

Since we're talking about equality between objects, this is merely a reference-to-the-same-thing check, no type coercion is ever expected. In other words: meh!

Comment 11

[Victor Porof [:vporof][:vp]]

Attachment: v4.1

Addressed comments. Focusing the variables view scrolls the selected node into view if not visible.

Comment 12

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Victor Porof [:vp] from comment #10)
> > 
> > ::: browser/devtools/shared/VariablesView.jsm
> > @@ +13,5 @@
> > >  const LAZY_EXPAND_DELAY = 50; // ms
> > >  const LAZY_APPEND_DELAY = 100; // ms
> > >  const LAZY_APPEND_BATCH = 100; // nodes
> > > +const PAGE_SIZE_SCROLL_HEIGHT_RATIO = 200;
> > > +const PAGE_SIZE_MAX_JUMPS = 30;
> > 
> > This feels a bit small in my testing (only 3 lines). I think it would be
> > best to get page up/down to focus the first/last visible item in the tree
> > after scrolling it to the end/start of the variables pane.
> > 
> 
> The amount of jumped lines depends on the total height of the variables view
> (aka total number of displayed variables and properties). If you'd expand
> this and the window scope, then there would be ~25 or 30 nodes jumped on
> each Page Down.

Yes I see this, but I think that always scrolling one full page would be what the user expects. I don't feel too strong about this though, so feel free to ignore me if you think otherwise.

Two things that still misbehave though are:
a) expanding the global scope and hitting the End key (scrolls to the last element - uneval, but doesn't highlight it).
b) more often than not focusing on the editor and hitting Cmd-Shift-V doesn't scroll to the local scope, even though it is highlighted when I scroll manually.

Comment 13

[Victor Porof [:vporof][:vp]]

(In reply to Panos Astithas [:past] from comment #12)
> (In reply to Victor Porof [:vp] from comment #10)
> > > 
> > > ::: browser/devtools/shared/VariablesView.jsm
> > > @@ +13,5 @@
> > > >  const LAZY_EXPAND_DELAY = 50; // ms
> > > >  const LAZY_APPEND_DELAY = 100; // ms
> > > >  const LAZY_APPEND_BATCH = 100; // nodes
> > > > +const PAGE_SIZE_SCROLL_HEIGHT_RATIO = 200;
> > > > +const PAGE_SIZE_MAX_JUMPS = 30;
> > > 
> > > This feels a bit small in my testing (only 3 lines). I think it would be
> > > best to get page up/down to focus the first/last visible item in the tree
> > > after scrolling it to the end/start of the variables pane.
> > > 
> > 
> > The amount of jumped lines depends on the total height of the variables view
> > (aka total number of displayed variables and properties). If you'd expand
> > this and the window scope, then there would be ~25 or 30 nodes jumped on
> > each Page Down.
> 
> Yes I see this, but I think that always scrolling one full page would be
> what the user expects. I don't feel too strong about this though, so feel
> free to ignore me if you think otherwise.

I don't feel strongly about this either so ok, sure, I'll boost the ratio.

> 
> Two things that still misbehave though are:
> a) expanding the global scope and hitting the End key (scrolls to the last
> element - uneval, but doesn't highlight it).
> b) more often than not focusing on the editor and hitting Cmd-Shift-V
> doesn't scroll to the local scope, even though it is highlighted when I
> scroll manually.

Hmm, this is pretty spectacular (in a bad way). The same implementation is used for all focus operations (there's also a ensureElementIsVisible call to the container nsIScrollBoxObject), so I would suspect something fishy going on on a lower level.

Comment 14

[Victor Porof [:vporof][:vp]]

Attachment: v4.2

Increased the container size to nodes scrolled ratio so that more stuff is jumped on Page Up/Down.

I did my best to avoid the weird XUL focus bugs by manually scrolling the container when focusing the first and last nodes (plus some other hacks). Also optimized the getItemForNode and getScopeForNode methods to make rapid continuous focus faster.

Comment 15

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/integration/fx-team/rev/d3156a1adca7

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

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