The Variables View should be keyboard accessible

RESOLVED FIXED in Firefox 21

Status

P2
normal
RESOLVED FIXED
6 years ago
5 months ago

People

(Reporter: vporof, Assigned: vporof)

Tracking

({access})

unspecified
Firefox 21
access
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

6 years ago
It's currently not.

Updated

6 years ago
Keywords: access
(Assignee)

Updated

6 years ago
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Priority: -- → P2
(Assignee)

Comment 1

6 years ago
Created attachment 706284 [details] [diff] [review]
v1

This was easier than I thought. Needs a test.
(Assignee)

Comment 2

6 years ago
Created attachment 706414 [details] [diff] [review]
v2

Added keyboard shortcuts for selecting the variables view.
Attachment #706284 - Attachment is obsolete: true
(Assignee)

Comment 3

6 years ago
Created attachment 706582 [details] [diff] [review]
v3

Added tests.
Attachment #706414 - Attachment is obsolete: true
Attachment #706582 - Flags: review?(past)
(Assignee)

Comment 4

6 years ago
Created attachment 706792 [details] [diff] [review]
v3.1

Fixed oranges.
Try: https://tbpl.mozilla.org/?tree=Try&rev=5726aa5debd3
Attachment #706582 - Attachment is obsolete: true
Attachment #706582 - Flags: review?(past)
Attachment #706792 - Flags: review?(past)
(Assignee)

Updated

6 years ago
Depends on: 832470
(Assignee)

Updated

6 years ago
Blocks: 831794
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).
Also, Delete loses focus from the variables view. When I press Delete focus moves to the debugger source view.
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.
(Assignee)

Comment 8

6 years ago
Created attachment 707212 [details] [diff] [review]
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.
Attachment #706792 - Attachment is obsolete: true
Attachment #706792 - Flags: review?(past)
Attachment #707212 - Flags: review?(past)
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?
Attachment #707212 - Flags: review?(past) → review+
(Assignee)

Comment 10

6 years ago
> 
> ::: 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!
(Assignee)

Comment 11

6 years ago
Created attachment 707806 [details] [diff] [review]
v4.1

Addressed comments. Focusing the variables view scrolls the selected node into view if not visible.
Attachment #707806 - Flags: review+
(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.
(Assignee)

Comment 13

6 years ago
(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.
(Assignee)

Comment 14

6 years ago
Created attachment 708155 [details] [diff] [review]
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.
Attachment #707806 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/d3156a1adca7
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 21

Updated

6 years ago
Blocks: 843187

Updated

5 years ago
Depends on: 918017

Updated

5 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.