feat: add keyboard suport for the canvas view #237

rthor · 2021-08-31T12:14:31Z

Why?

I can't use a mouse/trackpad with scroll functionality and therefore need keyboard support to be able to view and navigate large statecharts.

What this PR does:

Adds keyboard support for:

Panning
Zooming
Fit to view
Reset view

Note to maintainers:

I already talked with @davidkpiano about needing this functionality, and I hope it's okay that I just went ahead and added it. Wanted to use the new visualizer ASAP :) it's such a great workflow improvement for dealing with xstate! If you want this done differently, I am happy to help, or otherwise close this PR.

And congratz on the release 🎉

Features: - Panning - Zooming - Fit to view - Reset view

changeset-bot · 2021-08-31T12:14:33Z

🦋 Changeset detected

Latest commit: 65784f1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
xstate-viz-app	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

vercel · 2021-08-31T12:14:36Z

@rthor is attempting to deploy a commit to the Stately Team on Vercel.

A member of the Team first needs to authorize it.

vercel · 2021-08-31T13:15:17Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/statelyai/xstate-viz/8SiyzoquNwXNP8wfztuTbJVufoJg
✅ Preview: https://xstate-viz-git-fork-rthor-rthor-feat-keyboard-8221db-statelyai.vercel.app

mattpocock · 2021-08-31T13:27:37Z

src/CanvasContainer.tsx

+  if (positive.includes(key)) {
+    return delta;
+  } else if (negative.includes(key)) {
+    return -delta;


Suggested change

return -delta;

return 0 - delta;

Just a nit, feel free to ignore

mattpocock · 2021-08-31T13:28:50Z

@rthor Very nice! Spotted some odd behaviour in the top-right when you're on the tabs:

https://www.loom.com/share/25d827205468451b9fb84ec5636a418f

mattpocock · 2021-08-31T13:39:55Z

An idea for a fix for the above would be not allowing WASD navigation if the element you're focusing has a role of 'tab'. Kinda weird, maybe there's a better way of doing this @Andarist?

export const isTabLikeElement = (el: HTMLElement) => {
  return el.getAttribute('role') === 'tab';
};

Andarist

Thank you for an excellent PR! I have left some nits and requested minor changes. I've tested this though and the core of the functionality looks great. It would be nice to get some tests for this - but it's definitely more important for this to be merged in sooner than later so if you don't have time to figure out how to write Cypress tests for this - it's OK. We should add them eventually :p

BTW. I'm also working today on the Hand icon so you could switch to the Pan mode when that gets pressed. I hope this will also be a nice improvement in this area.

Andarist · 2021-08-31T13:47:54Z

src/CanvasContainer.tsx

+      const dx = getDeltaX(e.key, e.shiftKey);
+      const dy = getDeltaY(e.key, e.shiftKey);
+
+      if (e.key === '+') {


q: I've noticed in a different diagramming software that = works for this and Shift+= gets actually ignored. I'm OK with the chosen solution - just wondering if you know what's more common, from your experience?

In my experience + seems to be more common, but I have no preference, and am happy to do whichever you prefer.

Let's stick to + then 👍 I definitely have no preference of my own here

Andarist · 2021-08-31T13:54:35Z

src/CanvasContainer.tsx

+      } else if (e.key === 'f') {
+        e.preventDefault();
+        canvasService.send('FIT_TO_VIEW');
+      } else if (dx !== 0 || dy !== 0) {


nit: I would probably prefer for this condition to be more explicit. All preceding branches test the key first, before mapping that to any inner logic - and while I don't care if we list the exact keys here (although I find it useful), I feel like not having that explicit key-based test here blurs the logic a little bit.

With keys/tests listed here, we can quickly look at this to check what keys are actually handled by this listener - by hiding it in an abstraction that is called for all keys (even those that we don't care about at all) we have to learn that those other keys are actually ignored and by ignored I mean that 0 gets returned. This, of course, is a simple piece of code that can be understood quickly but is still a little bit more complex for what it is.

Imagine that you draw out this flow with a statechart - to implement this exact logic you would have to assign those values to context, then go through tests and eventually test against those temporarily assigned values. Whereas if you go with tests-first approach you only have a list of conditions mapped to appropriate actions - which, IMHO, ends up being a little bit easier :)

Andarist · 2021-08-31T13:58:47Z

src/CanvasContainer.tsx

+  useEffect(() => {
+    function keydownListener(e: KeyboardEvent) {
+      const target = e.target as HTMLElement;
+      if (isTextInputLikeElement(target)) {


q: from your experience - are those kinds of shortcuts always "global"? and by global I mean - are they also activatable from controls like buttons etc (assuming that the currently focused control doesn't come with an action of its own for this particular shortcut)?

I'm asking because we need to also ignore target.tagName === 'INPUT' && target.type === 'range', at least for the relevant keys. If those shortcuts are global then this should be only selectively ignored for arrows but not for WSAD keys.

So attaching the listeners directly to the canvas container? I was going to do this initially, only opted for global since that is being done for the spacebar lock logic.

It's often easy to focus body or some other element - so hooking into a global thing like window seems to be preferable because we won't miss keystrokes.

Sorry if my question was confusing - the question was about if conceptually those kinds of things are truly global, which includes those being triggered also on activatable elements. It was sort of an introduction to the comment about the range input because I've wanted to establish what kind of filtering logic we need to support.

Andarist · 2021-08-31T14:16:02Z

An idea for a fix for the above would be not allowing WASD navigation if the element you're focusing has a role of 'tab'. Kinda weird, maybe there's a better way of doing this @Andarist?

I don't really have a better idea than this - it's just a bit of a problem because there are a lot of default interactions that we could forget about and logic like this should, ideally, not conflict with those. The only two approaches I can think of for handling this is to:

try to prevent activating the logic based on conditions like this one
try to stop propagation everywhere

The problem with the second approach is that it's hard to apply it globally and it requires adding this almost everywhere - while also requiring listeners for native button behavior ("clicking" using Enter/Space), etc. So it, IMHO, doesn't scale. This makes me stick with the first approach - although it has its shortcomings as well.

#usetheplatform

rthor · 2021-08-31T14:28:01Z

Thanks for the quick reviews! I'll go through those and make appropriate fixes.

Regarding the control logic for panning, would these types of events make sense?

{
  on: {
    'PAN.LEFT': {
      actions: canvasModel.assign({
        pan: (ctx, e) => ({
          dx: ctx.pan.dx - (e.isLongPan ? LONG_PAN : SHORT_PAN),
          dy: ctx.pan.dy,
        }),
      }),
      target: '.throttling',
      internal: false,
    },
    'PAN.RIGHT': {
      // ...
    },
    'PAN.UP': {
      // ...
    },
    'PAN.DOWN': {
      // ...
    },
}

Then for choosing the correct event per key, would we just like a switch case:

switch (e.key) {
    case 'a':
    case 'ArrowLeft':
      return canvasService.send('PAN.LEFT')
    case 'd':
    // ...
}

Or is there a better way to do this?

Andarist · 2021-08-31T14:53:18Z

If we want to move more of this logic to the machine itself then the proposed changes sound OK to me.

rthor · 2021-08-31T15:15:11Z

Moved the logic to canvasMachine, increased the small pan delta, and fixed the tab behavior with your proposed solution @mattpocock. Testing this should be quite straightforward. Can either do it in this PR or follow up. Just don't have time right now.

BTW. I'm also working today on the Hand icon so you could switch to the Pan mode when that gets pressed. I hope this will also be a nice improvement in this area.

Looking forward to this!!

Andarist · 2021-09-01T23:07:47Z

src/CanvasContainer.tsx

+    function keydownListener(e: KeyboardEvent) {
+      const target = e.target as HTMLElement;
+
+      if (isTextInputLikeElement(target) || isTabLikeElement(target)) {


This is, of course, nit-picking (to some extent) but it seems to me that if we want to have a bullet-proof logic that works in the very same way for different interactions then we should selectively apply those filters based on the pressed key.

Why this is IMHO important? If we allow, let's say, WSAD keys on buttons then why we don't allow them on tab elements? Tab elements don't have WSAD-based interactions of their own, after all.

In addition to that - we should not handle arrow keys here when the target is an input of type range.

Andarist · 2021-09-01T23:11:20Z

src/utils.ts

@@ -210,3 +210,6 @@ export const isTextInputLikeElement = (el: HTMLElement) => {
    el.isContentEditable
  );
 };
+
+export const isTabLikeElement = (el: HTMLElement) =>
+  el.getAttribute('role') === 'tab';


An interesting fact is that @testing-library/dom cheks all space-delimited roles:
https://github.com/testing-library/dom-testing-library/blob/fbbb29a6d9655d41bc8f91d49dc64326f588c0d6/src/queries/role.js#L107-L112

Should we use the same logic for that? 🤔

nit: this probably should be called isTabElement or hasTabRole

We'll probably have the very same issue with arrows triggering canvas interactions when navigating through menus. Damn, handling all the cases here is hard! 😬

# Conflicts: # src/CanvasContainer.tsx # src/canvasMachine.tsx # src/utils.ts

Andarist · 2021-10-20T19:38:30Z

@rthor I'm deeply sorry that this has somehow slipped us and that we didn't follow up on this sooner. We've been busy with a lot of other stuff recently - and our focus has partially shifted away from the viz to other things.

This was a very important PR - it contains a very important feature that can improve the accessibility of the app. It was also amazing to receive such an impactful PR from an external contributor. Thank you for the work done here ❤️ and don't hesitate in the future to ping us if we stall with stuff like that.

I've picked this up today, rebased, I've done some additional research, and tried to cover as many cases as possible to avoid breaking existing functionality. The PR is awaiting review from the other team members - but I think we are very close to landing this (and releasing soon).

rthor · 2021-10-20T22:14:48Z

@Andarist no worries at all! I’m at fault too, I got busy with other projects as well and didn’t finish or follow up. Sorry about that!

I’ve been using the local version of the repo in the meantime, and it’s great! Just wrote a new state machine this morning using the visualizer :)

I’ll try to contribute more in the future as xstate (and now stately’s viz) are vital to my projects! Have a great week! Looking forward to seeing this one go live :)

mattpocock · 2021-10-21T11:09:26Z

Wanted to echo what @Andarist said above. This is a super-cool PR.

On to feedback - main functionality appears to be working nicely, but fit-to-view seems to be broken when I test this locally?

Andarist · 2021-10-21T11:42:31Z

On to feedback - main functionality appears to be working nicely, but fit-to-view seems to be broken when I test this locally?

That's probably an unwanted side-effect of my math adjustments - gonna fix this 🔜

Andarist · 2021-10-26T11:54:19Z

src/CanvasContainer.tsx

+      // entry contains `contentRect` but we are interested in the `clientRect`
+      // height/width are going to be the same but not the offsets
+      const clientRect = entry.target.getBoundingClientRect();


Might not be ideal to read it like this here - but it works for now. This fixes the problem depicted here: https://www.loom.com/share/271104072f2d45d2854b4c4b90b40386

farskid

@rthor Testing this now and I realized all the keybindings are working except for zooming.
Pressing - zooms out but zooming in happens only via Shift and +.

I'm on the latest Chrome.

Andarist · 2021-10-30T08:33:04Z

@farskid see the comment here: #237 (comment) . I'm OK with adjusting this however we want, I just have not researched this enough. For instance, Excalidraw requires Shift for both zooming out and zooming in

farskid · 2021-10-30T08:53:39Z

@farskid see the comment here: #237 (comment) . I'm OK with adjusting this however we want, I just have not researched this enough. For instance, Excalidraw requires Shift for both zooming out and zooming in

My issue with it is that why we need shift for one when the other one doesn't

mattpocock

Looks great, fit-to-view is working. The +, - thing doesn't bother me particularly, happy to approve without fiddling with that.

He asked me to

Andarist · 2021-11-02T13:24:04Z

@farskid I've changed this to not require Shift for both zoom in/out. I've also added numpad handling 😱 since it has -/+ too

feat: add keyboard suport for the canvas view

249b963

Features: - Panning - Zooming - Fit to view - Reset view

fix: reverse panning directions

ddbd3e0

davidkpiano requested review from farskid, Andarist, davidkpiano, laurakalbag and mattpocock August 31, 2021 12:58

mattpocock reviewed Aug 31, 2021

View reviewed changes

Andarist previously requested changes Aug 31, 2021

View reviewed changes

rthor added 2 commits August 31, 2021 14:57

fix: move panning logic to canvas machine

b94acf7

fix: ignore arrow panning on tabs

bf4564d

Andarist reviewed Sep 1, 2021

View reviewed changes

Andarist added 2 commits October 20, 2021 13:11

Merge branch 'dev' into rthor/feat-keyboard-handlers

a65546e

# Conflicts: # src/CanvasContainer.tsx # src/canvasMachine.tsx # src/utils.ts

Tweaks and improvements

7599a26

Andarist force-pushed the rthor/feat-keyboard-handlers branch from ce08d9b to 7599a26 Compare October 20, 2021 13:48

vercel bot deployed to Preview October 20, 2021 13:48 View deployment

Andarist requested a review from mattpocock October 20, 2021 19:38

Andarist added 2 commits October 21, 2021 15:21

Rename context.pan to context.viewbox

b5d7174

Fix zooming and fit-to-view

0fafc59

Andarist reviewed Oct 26, 2021

View reviewed changes

vercel bot deployed to Preview October 26, 2021 11:54 View deployment

farskid suggested changes Oct 29, 2021

View reviewed changes

mattpocock requested a review from farskid November 2, 2021 13:14

mattpocock approved these changes Nov 2, 2021

View reviewed changes

Unify how zooming hotkeys are handled

65784f1

davidkpiano approved these changes Nov 4, 2021

View reviewed changes

Andarist merged commit f0538a3 into statelyai:dev Nov 4, 2021

github-actions bot mentioned this pull request Nov 4, 2021

Version Packages #290

Merged

rthor deleted the rthor/feat-keyboard-handlers branch February 16, 2022 22:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add keyboard suport for the canvas view #237

feat: add keyboard suport for the canvas view #237

rthor commented Aug 31, 2021

changeset-bot bot commented Aug 31, 2021 •

edited

vercel bot commented Aug 31, 2021

vercel bot commented Aug 31, 2021 •

edited

mattpocock Aug 31, 2021

mattpocock Aug 31, 2021

mattpocock commented Aug 31, 2021

mattpocock commented Aug 31, 2021

Andarist left a comment

Andarist Aug 31, 2021

rthor Aug 31, 2021

Andarist Aug 31, 2021

Andarist Aug 31, 2021

Andarist Aug 31, 2021

rthor Aug 31, 2021

Andarist Aug 31, 2021

Andarist commented Aug 31, 2021

rthor commented Aug 31, 2021

Andarist commented Aug 31, 2021

rthor commented Aug 31, 2021 •

edited

Andarist Sep 1, 2021

Andarist Sep 1, 2021

Andarist Sep 1, 2021

Andarist commented Oct 20, 2021

rthor commented Oct 20, 2021

mattpocock commented Oct 21, 2021 •

edited

Andarist commented Oct 21, 2021

Andarist Oct 26, 2021

farskid left a comment

Andarist commented Oct 30, 2021

farskid commented Oct 30, 2021

mattpocock left a comment

Andarist commented Nov 2, 2021

feat: add keyboard suport for the canvas view #237

feat: add keyboard suport for the canvas view #237

Conversation

rthor commented Aug 31, 2021

Why?

What this PR does:

Note to maintainers:

changeset-bot bot commented Aug 31, 2021 • edited

🦋 Changeset detected

vercel bot commented Aug 31, 2021

vercel bot commented Aug 31, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattpocock commented Aug 31, 2021

mattpocock commented Aug 31, 2021

Andarist left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andarist commented Aug 31, 2021

rthor commented Aug 31, 2021

Andarist commented Aug 31, 2021

rthor commented Aug 31, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andarist commented Oct 20, 2021

rthor commented Oct 20, 2021

mattpocock commented Oct 21, 2021 • edited

Andarist commented Oct 21, 2021

Choose a reason for hiding this comment

farskid left a comment

Choose a reason for hiding this comment

Andarist commented Oct 30, 2021

farskid commented Oct 30, 2021

mattpocock left a comment

Choose a reason for hiding this comment

Andarist commented Nov 2, 2021

changeset-bot bot commented Aug 31, 2021 •

edited

vercel bot commented Aug 31, 2021 •

edited

rthor commented Aug 31, 2021 •

edited

mattpocock commented Oct 21, 2021 •

edited