Closed julienw closed 5 years ago
At first I thought the arrow keys could be used for moving around the viewport, but in the Call Tree the arrow keys are used to select nodes, so perhaps those should be used for selecting nodes in the charts too. What do you think?
In that case either we can use other keys for moving around the viewport, or perhaps adding modifier keys to the arrows. But I imagine we would need more modifier keys to control movement speed and perhaps toggle zoom mode, so it could be pretty crowded on those keys if the arrows have multiple meanings. I can experiment a little.
This is a very interesting idea indeed. Thinking even more broadly, including accessibility, this could make a nice way to use the charts for keyboard users and/or users using an assistive technology. I'm not sure how this works, but we could put some real text somewhere, including the content of the selected node, so that an assistive technology (screen reader or something else) can read it.
Another key (eg space or enter) could display the tooltip.
With some modifier key (eg: alt) we could move the viewport around. With another modifier key (eg: shift) we could zoom in and out.
We could also use WASD for moving within the chart. I've seen other profilers do this but I don't remember which ones.
I doubt they use only WASD, but why not. Also note that the literal WASD is valid on QWERTY (and similar) keyboards only. Fortunately the code
property or KeyboardEvent
s can help here :)
Thanks for the input! I was thinking I could make a preview with both WASD and modifier + arrows so that we can test it, however I got stuck on the selection part. For the flame graph I think it conceptually is pretty straightforward how selection navigation with the arrows could work, but for the stack chart it is tricker. For instance, see this picture (my selection color is orange):
Since we can only select call nodes, the selection spans several frames in time. I'm not sure how navigating horizontally should work. Left and right navigation don't seem to make much sense with that kind of selection. Also, the tooltips refer to individual frames, so toggling the tooltip from the selection is difficult. Perhaps we need to be able to support selection individual frames in order to support keyboard based selection?
I wanted to implement both selection and movement in the same PR to get a better feeling of how it would work UX wise, but since it tricky perhaps I should just go with the navigation for now and defer selection for another issue. What do you think?
Deferring selection sounds reasonable to me.
good for me; maybe for selection we should have another keystroke like <enter>
.
Here's a preview you can checkout. It adds simple shortcuts for charts which use viewports. https://deploy-preview-1324--perf-html.netlify.com/public/6086f8478e068e9f9fa921ac5587b51c79d8b0d8/stack-chart/?globalTrackOrder=0-1-2-3-4&hiddenGlobalTracks=1-3&localTrackOrderByPid=7272-1-0~7391-0~&thread=5&v=3
I implemented three different navigation schemes at the same time, so that you can try them all out:
The zoom buttons I chose were pretty arbitrary, since using modifier keys with letters often conflict with various built in Firefox shortcuts. I also didn't pick the alt modifier for the arrow keys as that also navigates the history in Firefox.
Please use event.code
, as Julien suggested further upthread, instead of event.key
. I use the Dvorak keyboard layout so the WASD keys are in odd places. And if you use .code
, I can use the keys in the right places, even if they wouldn't result in the letters WASD if I was typing into a text field.
Ah, yes. Sorry, that helpful tip slipped my mind.
I've updated the code. Does the deploy preview update itself after pushing a commit (with a failing build, which I ignored)?
By the way, the code for focusing the viewports needs to be better, so don't look closer into that part please :)
That works a lot better, thanks! Two more things:
.preventDefault()
in the right place.requestAnimationFrame
?Busy times, but finally got around to work on this again.
@mstange I've updated the deploy preview addressing your two points. How does it feel now, you think? If it seems alright I'll make a new PR, candidate for merging, where I'll fix some issues I've found while testing this.
But how do we feel about the keys used here? While it's nice to have different options to choose between, it is perhaps a bit much to offer three distinct ones as it claims a number of keyboard keys. Perhaps we'd want to use keys for other things and it'll be harder to change if users have gotten used to (different) controls.
I think I'm most fond of the HJKL-controls, FWIW.
Feels pretty good to me! The speed is a bit fast, and I think you should compute the pixel delta based on the elapsed time (times the intended velocity), but other than that, I really like it. I don't think it's too much of a problem to reserve this many keys; I don't know what else we'd want to use keys for.
I've also sometimes noticed a problem where focus was lost is some way after I was in a different tab, meaning, when I switched back to the Firefox tab containing your perf.html instance, panning on key input no longer worked. Switching from the stack chart tab to the flame graph tab and back to the stack chart fixed it. Clicking the stack chart did not fix it.
I think I like the wasd mode more :) especially with q/e, this comes nicely under the fingers. And I'm using this on an azerty keyboard, so thanks for using .code
;)
I think we should also use the arrows without modifiers. Maybe in the future we'll change this to move between nodes, but now we should use these keys for this. The reason is that we're used to using the arrows to move in a webpage, and this is exactly what's missing in these chart panels.
Thanks for the feedback!
@julienw Greg already made a deploy preview where the flame graph nodes can be selected with the arrow keys: https://deploy-preview-1311--perf-html.netlify.com/public/91bdb2248d08174f07d9186aabbe1025e5ff9bf1/flame-graph/?globalTrackOrder=0-1-2-3-4&hiddenGlobalTracks=1-3&hiddenLocalTracksByPid=41032-1-4-2-3~41035-0~41033-0-1&localTrackOrderByPid=41032-6-0-1-2-3-4-5~41035-0~41033-2-0-1~&thread=11&transforms=df-274&v=3 If we use the arrow keys, his code won't work anymore. Do you have any suggestion of what keys we should use for node selection instead? Not saying you need to, just wondering, since then I can stay away from those keys :)
My take was that we should use the arrows for something now. So until we have greg's code working, we could hook those to your code.
But we should have the wasd/qe keys work as well.
So in the final situation, when Greg's code will land, we'll have the arrow keys navigating between "objects" (markers / nodes), and the wasd/qe keys for visual/spatial navigation.
ctrl + up/down keys could trigger zooming in (see https://github.com/devtools-html/perf.html/issues/1371 about that as well). Or ctrl + arrow keys would make it easier to navigate between part of the applications ? (eg: ctrl + up => focus the timeline, ctrl + down => focus the panel, ctrl + right => open the sidebar and focus the sidebar).
Shift + arrow keys would be for selecting several objects... When we support this :)
Got it. Thanks for clarifying :)
I have a PR up for review, please checkout the deploy preview to see if the speed and keyboard mappings are alright everyone. https://deploy-preview-1433--perf-html.netlify.com/public/6086f8478e068e9f9fa921ac5587b51c79d8b0d8/stack-chart/?globalTrackOrder=0-1-2-3-4&hiddenGlobalTracks=1-3&localTrackOrderByPid=7272-1-0~7391-0~&thread=5&v=3
I would make it a bit faster, but otherwise this looks good :)
Currently we can only move charts with the mouse (mousewheel to zoom, drag and drop to move around). We should support the keyboard too.