Closed Bug 1243131 Opened 8 years ago Closed 8 years ago

Keyboard navigation for switching between snapshots

Categories

(DevTools :: Memory, defect, P3)

Product:
DevTools
Component:
Memory
Type:
defect

Tracking

(firefox47 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 47
Tracking Flags:
Tracking Status
firefox47 --- fixed

People

(Reporter: fitzgen, Assigned: jdescottes)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

bug1243131.v5.patch (rebased)
12.07 KB, patch
jdescottes
: review+
Details | Diff | Splinter Review 
<Ctrl or Alt or Shift> + <Up or Down> should navigate between snapshots.
Priority: -- → P3
Assignee: nobody → jdescottes
Depends on: 1246650
Helen: Any input on the key modifier we should use here?
Flags: needinfo?(hholmes)
Let's go with ctrl. Shift seems an odd choice and alt seems to be used in editors for incrementing/decrementing numbers.
Flags: needinfo?(hholmes)
Attached patch bug1243131.v1.patch (obsolete) — — Splinter Review 
Thanks for the feedback Helen. Assuming ctrlKey here means accelKey (ie metaKey on OSX and ctrlKey on other OSes).

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

Nick: I tried to go for the simplest approach here : memory app attaches a keydown listener on the window, and the tree nodes no longer consume keydown events with modifiers. 

Attaching the event via React to #memory-tool is feasible but it requires to always keep the focus inside #memory-tool. And I think we want the CTRL+UP/DOWN shortcuts to be always available when the panel is opened, wherever the focus maybe inside the panel. 

Idea for another patch/bug : it would be nice to also have the first row focused by default when we open a snapshot. That would make for an easier keyboard navigation. There are some focus conflicts to resolve for this though (eg. when typing in filter field, tabletree is recreated, but focus should remain in filter field).

Let me know what you think.
Attachment #8717437 - Flags: feedback?(nfitzgerald)
(In reply to Julian Descottes [:jdescottes] from comment #3)
> Created attachment 8717437 [details] [diff] [review]
> bug1243131.v1.patch
> 
> Thanks for the feedback Helen. Assuming ctrlKey here means accelKey (ie
> metaKey on OSX and ctrlKey on other OSes).

Now I'm confused. Macs have a ctrlKey? Should it not just be ctrl everywhere?
(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #4)
> (In reply to Julian Descottes [:jdescottes] from comment #3)
> > Created attachment 8717437 [details] [diff] [review]
> > bug1243131.v1.patch
> > 
> > Thanks for the feedback Helen. Assuming ctrlKey here means accelKey (ie
> > metaKey on OSX and ctrlKey on other OSes).
> 
> Now I'm confused. Macs have a ctrlKey? Should it not just be ctrl everywhere?

Summary of our discussion on IRC : most devtools shortcuts that rely on CTRL are mapped to COMMAND on OSX, so we can use the same mapping here. 

(NB: metaKey is the event property mapped to the "COMMAND" modifier).
Comment on attachment 8717437 [details] [diff] [review]
bug1243131.v1.patch

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

Looks great! Feel free to r? on the next one

::: devtools/client/memory/app.js
@@ +54,5 @@
>      return {};
>    },
>  
> +  componentDidMount() {
> +    window.addEventListener("keydown", this.onKeyDown);

I'm not convinced that we need to register this listener on the window instead of on the app's container -- did you observe funky behavior? The app's container is the only element within the <body>  so it seems to me it should be very hard to focus the <body> or window without focusing the app's container. Although if you have found that it is possible in some conditions, then by all means let's keep what we have here.

@@ +96,5 @@
> +      this.selectSnapshot(snapshots[selectedIndex - 1]);
> +    }
> +    // On ACCEL+DOWN, select next snapshot.
> +    if (isAccelKey && e.key === "ArrowDown") {
> +      this.selectSnapshot(snapshots[selectedIndex + 1]);

We should clamp the new selected index within the range [0, snapshots.length) both here and above.

::: devtools/client/memory/test/browser/browser_memory_keyboard-snapshot-list-dominator.js
@@ +28,5 @@
> +}
> +
> +this.test = makeMemoryTest(TEST_URL, function* ({ panel }) {
> +  // Taking snapshots and computing dominator trees is slow :-/
> +  requestLongerTimeout(4);

No need to do dominator trees in this test -- a census would exercise the tree component's key bindings as well, and wouldn't require the longer timeout.

::: devtools/client/memory/test/browser/browser_memory_keyboard-snapshot-list.js
@@ +1,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +// Test that using ACCEL+UP/DOWN arrows, the user can navigate between snapshots
> +// in census view.

I don't think testing both census and dominator trees is buying us any additional test coverage -- they are both using the same tree component. I think it is safe to keep just this census test.

Additionally, we should test that we can't go up past the first snapshot or down past the last snapshot. The first and last snapshot should stay selected, respectively.

::: devtools/client/shared/components/tree.js
@@ +406,5 @@
>      }
>  
> +    // Allow parent nodes to use navigation arrows with modifiers.
> +    let hasModifier = e.altKey || e.ctrlKey || e.shiftKey || e.metaKey;
> +    if (hasModifier) {

Small style nit: no need to use a temp `hasModifier` variable here, it isn't really buying readability IMO, just inline the expression into the condition.
Attachment #8717437 - Flags: feedback?(nfitzgerald) → feedback+
Thanks for the feedback!

Listener on window
===================

Registering the listener on window is necessary here, unless we want to make sure that the focus is always trap inside the memory container. The activeElement of the memory panel is almost always the body at the moment. Exceptions to this rule: when a row is focused and when an input/button of the toolbar is focused.

FWIW, for my initial take on this bug I changed the census/dominator view to always have a default selected row. This way the focus was always trapped in the memory-container. But that's a heavier code change, and it comes with some complications, that's why I preferred to go with the simpler solution of adding the listener on window here.

Testing the noop
================

We discussed how to test the noop (press ctrl+UP on first row). I changed my mind: I will go with your proposal and assume the selection is synchronous. Relying on another update (eg. inverting the tree) is actually just another assumption, and doesn't make the test more stable.

try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=f88a04b94ebf
Attachment #8717437 - Attachment is obsolete: true
Attachment #8717664 - Flags: review?(nfitzgerald)
Comment on attachment 8717664 [details] [diff] [review]
bug1243131.v2.patch (addressed feedback comments)

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

Great! Thanks!
Attachment #8717664 - Flags: review?(nfitzgerald) → review+
Try looks ok. 
Can you push attachment 8717664 [details] [diff] [review] ?
Keywords: checkin-needed
failed to apply:

Hunk #1 FAILED at 9
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/memory/test/browser/browser.ini.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh bug1243131.v2.patch
Flags: needinfo?(jdescottes)
Keywords: checkin-needed
Just rebased. Carry over r+.
Attachment #8717664 - Attachment is obsolete: true
Flags: needinfo?(jdescottes)
Attachment #8718296 - Flags: review+
Can you push attachment 8718296 [details] [diff] [review] ? It should apply fine now.
Keywords: checkin-needed
I compared the execution time on different platforms for this test : 

- linux : 5 seconds
- linux debug : 45 seconds
- linux 64 : 6 seconds
- linux 64 debug : 16 seconds
- osx : 2 seconds
- osx debug : 6 seconds
- win8 : 1.7 seconds
- win8 debug : 8 seconds

It's really puzzling that the test is so slow on Linux 32 debug. The logs are also extremely verbose for Linux32 debug build : 90k lines vs 18k lines for the same job on Linux 64 debug. And it all seems related to the initial creation of snapshots. Only creating the test data already takes 25 seconds, which leaves only 20 seconds for the test to run.

Looking at our test environment, our test page creates 100 objects every 10 ms [1]. This may be pretty slow on low performance platforms. Here is a try push using a much slower allocation script, since this doesn't really matter for my test case : 

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

[1] https://dxr.mozilla.org/mozilla-central/source/devtools/client/memory/test/browser/doc_steady_allocation.html#10
Carry over r+.

I tried speeding up the test in several ways, to no avail. And sadly this test cannot be split up as it tests a very simple flow.

Have to increase the timeout here. As discussed on IRC we'll try to move as many memory integration tests to other test solutions (xpcshell, chrome) that should be much faster. This one cannot be migrated for now, as it relies on event listeners added on window.

try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=c58f8f4f5e43
Attachment #8718296 - Attachment is obsolete: true
Attachment #8718893 - Flags: review+
Carry over r+, rebased.
Attachment #8718893 - Attachment is obsolete: true
Attachment #8719335 - Flags: review+
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/8618a85c5801
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: