diagram-js icon indicating copy to clipboard operation
diagram-js copied to clipboard

Make canvas selectable / keyboard binding implicit

Open nikku opened this issue 2 years ago • 25 comments

This PR ships the foundations to support cross-browser/application copy and paste:

  • Makes canvas selectable (previously incorporated via https://github.com/bpmn-io/diagram-js/issues/661, but later reverted as we could not follow up on the original feature).
  • Adds implicitly bind keyboard against the selectable canvas instead of allowing to bind against any DOM element on the page.

For demonstration purpose checkout application integration example in form-js.

This ensures a predictable editing experience, simplifies the keyboard and ensures worry free interoperability with multiple editors and inputs on the page.


Previous calls to keyboard.bind(someElement) or configuration via keyboard.bindTo log a descriptive descriptive error:

image


Closes https://github.com/bpmn-io/diagram-js/issues/661

nikku avatar Aug 15 '22 22:08 nikku

This can be tried out via:

npx @bpmn-io/sr nikku/bpmn-js-native-copy-paste#keyboard-bind -c "npm run start"

What I did is to remove any input quirks but rely on canvas to be actually browser selected. As a side-effect though that means that we consistently need to select the canvas when modeling operations happen:

capture IH2DTf_optimized

I do not see a simple way to accomplish this (yet).


At this point we have two options:

  • :a: ditch this approach and keep existing manual keybinding
  • :b: invest into consistently + properly re-focus the canvas

:b: is a requirement for cross browser copy + paste to work reliably so I'd argue we need to pursue this route anyway.

nikku avatar Aug 16 '22 11:08 nikku

The following shows how we may mitigate this: Whenever the canvas is mouse hovered we re-focus the canvas.

For debugging purposes I added a solid blue focus outline (solid blue => receives keyboard events + can copy + paste).

capture 3jv7hP_optimized


Try out via

npx @bpmn-io/sr nikku/bpmn-js-native-copy-paste#keyboard-bind -c "npm run start"

nikku avatar Aug 16 '22 14:08 nikku

On MacOS, the focus state is persisted once I hover the canvas even if switch tab afterwards and come back. Is this is supposed to work like this? I am not opinionated but it would be logical to me that focus fades away when I move mouse outside without clicking.

barmac avatar Aug 16 '22 14:08 barmac

Cf. recording

https://user-images.githubusercontent.com/28307541/184912457-4cb8059b-2bf0-4027-912d-52aba7155da7.mov

barmac avatar Aug 16 '22 15:08 barmac

https://github.com/bpmn-io/diagram-js/pull/662#issuecomment-1216757684

I guess so. Keyboard inputs also don't magically unfocus once you leave with the mouse. The existing implementation works in the exact same way, or at least should work in the same way.

nikku avatar Aug 16 '22 15:08 nikku

Hmm I don't like the infinite scrolling of the canvas which triggers once you paste outside of the canvas, but this solution is still an improvement compared to what we have right now. Thus, 👍 from me.

barmac avatar Aug 16 '22 15:08 barmac

Fortunately infinite scrolling is an existing feature (cf. demo.bpmn.io):

capture e4fk9B_optimized

nikku avatar Aug 16 '22 15:08 nikku

Indeed, it's an out of scope feature to be fixed 😆

barmac avatar Aug 16 '22 15:08 barmac

When we implicitly bind to the canvas, does it imply that extensions that rely on the command stack need to catch and trigger undo/redo manually? I'm thinking about the Properties panel here, where the most common case was to bind the keyboard to a common container.

I tested it on Edge and it works as expected, no new issues found from my side

marstamm avatar Aug 16 '22 15:08 marstamm

When we implicitly bind to the canvas, does it imply that extensions that rely on the command stack need to catch and trigger undo/redo manually?

Exactly. The related hacks are all gone from the code base. If you want to trigger undo/redo via keyboard shortcuts you have to implement that yourself (i.e. attach a respective listener on the properties panel).

nikku avatar Aug 16 '22 16:08 nikku

@philippfromme Given that there is a little bit of blinking during focus/unfocus going on we could decide to ditch the focus outline. What do you think?

nikku avatar Aug 17 '22 08:08 nikku

While I was able to break the focus-on-hover on Safari, it's due to some element.hover bug – which is IMO an edge case we shouldn't care about.

https://user-images.githubusercontent.com/28307541/185079281-690ceb1e-c49f-43b3-9e37-06319e212a02.mov


Regarding the cross-browser copy and paste, on MacOS I am able to copy and paste across single browser tabs or even windows, but I cannot do that between different browsers (Safari -> Chrome, Chrome -> Safari). The clipboardData appeared to be empty when I tried to debug it.

barmac avatar Aug 17 '22 09:08 barmac

Found the following issues integration testing this against different libraries in our eco-system:

  • dmn-js-drd: no issues (works out of the box)
  • dmn-js-decision-table / literal expression editor: has it's own keyboard implementation :arrow_right: needs porting to similar bind behavior, i.e. either make the table selectable + bind to it.
  • form-js: re-uses diagram-js keyboard but has no canvas :arrow_right: need to improve the bind / unbind behavior proposed in this PR to get the actual diagram container. Two ideas:
    • expose diagram with container in form-js, too
    • pass the container on diagram.init for components to catch it and do stuff with it (i.e . register/unregister key bindings)

nikku avatar Aug 17 '22 13:08 nikku

form-js: re-uses diagram-js keyboard but has no canvas ➡️ need to improve the bind / unbind behavior proposed in this PR to get the actual diagram container. Two ideas:

  • expose diagram with container in form-js, too
  • pass the container on diagram.init for components to catch it and do stuff with it (i.e . register/unregister key bindings)

Given we follow one of these approaches, would we also have to follow up with explicit keyboard bindings for the Forms Properties Panel?

I'm asking because we plan to make it independent from the editor (e.g. render it in an own container, as we do with the other properties panels, cf. https://github.com/bpmn-io/form-js/pull/291).

pinussilvestrus avatar Aug 17 '22 14:08 pinussilvestrus

[...] would we also have to follow up with explicit keyboard bindings for the Forms Properties Panel?

Absolutely. Taking it one step at a time here though. Blueprint for properties panel binding can be found here: https://github.com/bpmn-io/bpmn-js-properties-panel/pull/739.

nikku avatar Aug 17 '22 14:08 nikku

This is now available as a pre-release via [email protected] and [email protected].

nikku avatar Aug 17 '22 15:08 nikku

Found one issue integration testing this:

capture VmbO2q_optimized

From what it looks like we need to prevent the built in scroll on focus behavior (https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/focus).

nikku avatar Aug 17 '22 16:08 nikku

Addressed https://github.com/bpmn-io/diagram-js/pull/662#issuecomment-1218241894 via https://github.com/bpmn-io/diagram-js/pull/662/commits/b6bfeb90182a437318f0634e3d71f5c03047e01a

nikku avatar Aug 17 '22 16:08 nikku

The form-js integration, specifically the playground shows best what browser native focus handling + implicit keyboard binding can accomplish:

capture CPUDfT_optimized

nikku avatar Aug 17 '22 21:08 nikku

The form-js integration, specifically the playground shows best what browser native focus handling + implicit keyboard binding can accomplish:

Nice, so it fixes https://github.com/bpmn-io/form-js/issues/277 👍

pinussilvestrus avatar Aug 18 '22 06:08 pinussilvestrus

I rebased this branch with the current main, I saved the previous state in this other branch since we had some conflicts while rebasing it

vsgoulart avatar Jan 29 '24 10:01 vsgoulart

@nikku do you have a (non-binding) ETA for this PR? :sweat_smile:

Just so I can update the team

vsgoulart avatar Feb 12 '24 14:02 vsgoulart

@vsgoulart I'm going to assess it this week.

nikku avatar Feb 12 '24 17:02 nikku