Support PageDown, PageUp, Home and End keys in calltree for navigation by julienw · Pull Request #1143 · firefox-devtools/profiler

julienw · 2018-07-18T14:25:21Z

I use the call tree a lot these days, with the sidebar work, and it makes me crazy, maybe since I joined the project, so here it is.

I took the opportunity to move to enzyme for this component and add a test for this change.

julienw · 2018-07-18T14:26:54Z

-              }
+    if (isNavigationKey) {
+      switch (event.key) {
+        case 'ArrowLeft': {


I move to using event.key instead of keyCode which is a lot better.
But the code change here is because I moved the curly brackets to embed the break command, which reduced the indentation. I believe that if you configure the diff to ignore the whitespace changes this is better.

julienw · 2018-07-18T14:28:52Z

+      // VirtualList's virtualization.
+      jest
+        .spyOn(HTMLElement.prototype, 'getBoundingClientRect')
+        .mockImplementation(() => getBoundingBox(1000, 2000));


I genuinely tried to make it work without this, by trying to make the VirtualList rerender, but I failed. Also because this isn't the goal for this test I decided to go this way, like it was in other tests actually.

Note that I fake getBoundingClientRect only here because I realized that the default implementation in JSDom (I think it returns 0 for all values) makes all the other tests pass properly.

Is there an assertion you can put in here to ensure that the virtualization doesn't kick in? I'd feel a bit safer if the test working wasn't implicit on this property. I wouldn't work too hard on it if it's hard to do though.

e.g. in horrible pseudo-code:

expect(VirtualTree.virtualization).toBe(false) // or expect(visibleRows.length).toBe(callTree.rows.length)

codecov · 2018-07-18T14:29:43Z

Codecov Report

Merging #1143 into master will increase coverage by 0.39%.
The diff coverage is 72.54%.

@@            Coverage Diff             @@
##           master    #1143      +/-   ##
==========================================
+ Coverage   73.28%   73.68%   +0.39%     
==========================================
  Files         140      140              
  Lines        8633     8659      +26     
  Branches     2026     2038      +12     
==========================================
+ Hits         6327     6380      +53     
+ Misses       2036     2018      -18     
+ Partials      270      261       -9

Impacted Files	Coverage Δ
src/components/shared/TreeView.js	`74.1% <72.54%> (+17.03%)`	⬆️
src/components/timeline/TracingMarkers.js	`55.55% <0%> (-0.75%)`	⬇️
src/components/shared/VirtualList.js	`90.9% <0%> (+1.01%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b916caa...f2711a6. Read the comment docs.

julienw · 2018-07-18T14:29:47Z

@@ -495,29 +495,27 @@ exports[`calltree/ProfileCallTreeView computes a width for a call tree of a real
    >


All the changes here come from the move to enzyme. This is quite straightforward: looks like enzyme retains the "key" props in the output, and otherwise this is some space changes.

julienw · 2018-07-18T14:32:41Z

+    simulateKey('PageUp');
+    expect(selectedText()).toBe('name84'); // 15 rows above
+    simulateKey('Home');
+    expect(selectedText()).toBe('name1');


Note I didn't test 'ArrowLeft' and 'ArrowRight' because it would need a separate profile and a separate test, and this wasn't the goal of this code change. But if you want I can add one.

gregtatum

Looks good! I left some optional feedback, and the only thing before landing is getting it to work on MacBooks. Feel free to ping me again for manual testing.

gregtatum · 2018-07-19T15:03:15Z

 import type { IconWithClassName } from '../../types/reducers';
 import type { CssPixels } from '../../types/units';

+const PAGE_KEYS_DELTA = 15;


Could you include a comment on how you came up with this magic number?

I think using a magic number here is fine, but I think the more consistent behavior with other UI is to have it move based on the size of the viewport. That's more complicated to implement, so this is fine and provides value.

That was also my rationale here :)

added a comment

gregtatum · 2018-07-19T15:06:18Z

-              }
+    if (isNavigationKey) {
+      switch (event.key) {
+        case 'ArrowLeft': {


The event.key switch is "beaucoup mieux"!

gregtatum · 2018-07-19T15:17:42Z

+      // VirtualList's virtualization.
+      jest
+        .spyOn(HTMLElement.prototype, 'getBoundingClientRect')
+        .mockImplementation(() => getBoundingBox(1000, 2000));


Is there an assertion you can put in here to ensure that the virtualization doesn't kick in? I'd feel a bit safer if the test working wasn't implicit on this property. I wouldn't work too hard on it if it's hard to do though.

e.g. in horrible pseudo-code:

expect(VirtualTree.virtualization).toBe(false) // or expect(visibleRows.length).toBe(callTree.rows.length)

gregtatum · 2018-07-19T15:23:54Z

+      };
+    }
+
+    const { simulateKey, selectedText } = setup();


Optional to your opinion: I find it a bit surprising that the setup function is in the it block, usually I've written them in the describe block, then run them in the it.

yeah, I know this is a bit surprising. I wanted to abstract away the possible operations but this is useful to this test only (for now).

For a test that would test "left/right" arrows, we'll need a different profile. So I'll create a "describe" block for navigation keys, and generalize a bit this setup function to take a profile string. I think this will be better, thanks for the comment.

gregtatum · 2018-07-19T15:33:02Z

-              }
-            }
+        }
+        case 'PageUp': {


This doesn't work on my MacBook. Cmd + Up is the correct shortcut.

Page up: event.metaKey && event.key === "ArrowUp"
Page down: event.metaKey && event.key === "ArrowDown"

I'm so sad.

@gregtatum I looked closer. Actually on MacOS it seems there are other shortcuts, at least according to http://osxdaily.com/2015/07/07/page-up-page-down-mac-keyboard/ and http://osxdaily.com/2015/06/17/home-end-button-mac-keyboard-functions/ and my test on an old MacBook we have in the office:

Fn + Up/Down give PageUp and PageDown

Fn + Left/Right give Home and End

Cmd + Up you're mentioning actually gives the behavior for Home but doesn't generate the value Home.

I could add the support for Home (and the Cmd + Down for End) but I'd like your feedback first, especially because you said it was PageUp for you.

Yeah I don't think Cmd + Up is the right shortcut. It doesn't do page scrolling in Activity Monitor or in Thunderbird, for example. Here's the key handling code of the XUL tree view, which we could use for inspiration: https://searchfox.org/mozilla-central/rev/8384a6519437f5eefbe522196f9ddf5c8b1d3fb4/toolkit/content/widgets/tree.xml#822-905

Yeah, I don't know why I put this on page up/down, when I meant home/end. Sorry about that.

In TextEdit and Firefox TextAreas I get the following behavior:

Cmd + Up/Down sends the cursor to the top/bottom of the window and scrolls the view into place.

Fn + left/right sends the viewport to the top/bottom, but does not affect the cursor.

Right now this patch does the opposite:

Cmd + Up/Down sends the viewport to the top/bottom, but does not affect the selected call node.

Fn + left/right sends the current selection to the top/bottom of the window and scrolls the view into place.

As a Mac user, I would appreciate having the consistent behavior with the shortcut I use, even if the other one is available. It's not a big deal breaker though if we want simpler code.

I can implement Cmd + Up/Down for sure, but I'd rather keep Fn + left/right as it is already in the patch, because then I don't need to implement different behavior for different OS. (maybe we'll have to eventually).

Also I believe that text editors are different. On my OS, Home and End doesn't move up/down in a text area, rather they go respectively at the start and end of a line. It would be nice to know how these shortcuts work for you on apps and contexts that are more list-based, eg Apple Mail or Firefox' bookmarks list.

From https://support.apple.com/en-us/HT201236:

Fn–Up Arrow: Page Up: Scroll up one page. Fn–Down Arrow: Page Down: Scroll down one page. Fn–Left Arrow: Home: Scroll to the beginning of a document. Fn–Right Arrow: End: Scroll to the end of a document. Command–Up Arrow: Move the insertion point to the beginning of the document. Command–Down Arrow: Move the insertion point to the end of the document. Command–Left Arrow: Move the insertion point to the beginning of the current line. Command–Right Arrow: Move the insertion point to the end of the current line.

(which is what you describe, with more information about other shortcuts)

I think eventually I'd like to make Home work like this:

if the current selected node isn't a root node, go to the root node of the current selected node.

if the current selected node is a root node, go to the top of the tree.
Because this is a bit more work I left this out of this PR initially, but please tell me what you think :)

I'm not sure how End would work in this context though. Maybe the behavior outlined in #517 ?

…stcase + add an assertion that the virtualization behavior isn't triggered

…ry value

… the testcase + add an assertion that the virtualization behavior isn't triggered

julienw · 2018-07-24T12:09:21Z

  _onKeyDown(event: KeyboardEvent) {
-    const hasModifier = event.ctrlKey || event.altKey || event.metaKey;
-    const isArrowKey = event.key.startsWith('Arrow');
+    const hasModifier = event.ctrlKey || event.altKey;


Because I removed event.metaKey from this boolean calculation, I'd like some MacOS user check that I'm not hijacking and preventing some well known browser or OS shortcut using Command from working. ping @gregtatum @mstange

julienw · 2018-07-24T12:09:53Z

+            if (this.props.tree.hasChildren(selected)) {
+              this._select(this.props.tree.getChildren(selected)[0]);
            }
          }


I reorganized the switch to organize it by pair (eg: ArrowUp close to ArrowDown)

gregtatum · 2018-07-24T22:13:12Z

Thanks, this is working as my muscle memory would expect on Mac. Everything seems to be working correctly manually as far as I can tell.

julienw · 2018-07-25T11:52:43Z

Thanks for the manual check !

julienw commented Jul 18, 2018

View reviewed changes

julienw requested a review from gregtatum July 18, 2018 14:30

julienw commented Jul 18, 2018

View reviewed changes

gregtatum approved these changes Jul 19, 2018

View reviewed changes

julienw added 5 commits July 20, 2018 15:14

Handle more navigation keys in the call tree and marker table

69a50df

Move to Enzyme for ProfileCallTreeView

0558dde

Add a test for the up and down navigation keys

7c9a6ee

Review comments for tests: extract the 'setup' function out of the te…

9daff6c

…stcase + add an assertion that the virtualization behavior isn't triggered

Review comments for app code: add a comment for our hardcoded arbitra…

73f3c03

…ry value

julienw force-pushed the support-pagedown-pageup-in-calltree branch from 8a3f9bb to 73f3c03 Compare July 20, 2018 13:34

julienw added 2 commits July 24, 2018 14:06

fixup! Review comments for tests: extract the 'setup' function out of…

9265efa

… the testcase + add an assertion that the virtualization behavior isn't triggered

Review comments for app code: implement Cmd + Up/Down for MacOS users

f2711a6

julienw commented Jul 24, 2018

View reviewed changes

julienw merged commit 9934bc9 into firefox-devtools:master Jul 25, 2018

		@@ -495,29 +495,27 @@ exports[`calltree/ProfileCallTreeView computes a width for a call tree of a real
		>

Conversation

julienw commented Jul 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Jul 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gregtatum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

julienw Jul 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gregtatum commented Jul 24, 2018

Uh oh!

julienw commented Jul 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Jul 18, 2018 •

edited

Loading

julienw Jul 24, 2018 •

edited

Loading