-
Notifications
You must be signed in to change notification settings - Fork 419
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make canvas selectable / keyboard binding implicit #662
base: develop
Are you sure you want to change the base?
Conversation
This can be tried out via:
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: I do not see a simple way to accomplish this (yet). At this point we have two options:
|
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). Try out via
|
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. |
Cf. recording Screen.Recording.2022-08-16.at.17.00.18.mov |
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. |
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. |
Fortunately infinite scrolling is an existing feature (cf. demo.bpmn.io): |
Indeed, it's an out of scope feature to be fixed 😆 |
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 |
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). |
@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? |
While I was able to break the focus-on-hover on Safari, it's due to some Screen.Recording.2022-08-17.at.10.59.15.movRegarding 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. |
Found the following issues integration testing this against different libraries in our eco-system:
|
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. bpmn-io/form-js#291). |
Absolutely. Taking it one step at a time here though. Blueprint for properties panel binding can be found here: bpmn-io/bpmn-js-properties-panel#739. |
This is now available as a pre-release via |
This prepares for bpmn-io/diagram-js#662, but is built in a way that is backwards compatible.
This prepares for bpmn-io/diagram-js#662, but is built in a way that is backwards compatible.
@nikku do you have a (non-binding) ETA for this PR? 😅 Just so I can update the team |
50a2150
to
c176043
Compare
@vsgoulart I'm going to assess it this week. |
Further explaination after discussion with @jarekdanielak: What #662 (comment) aimed to accomplish is to ensure that the canvas remains focusable, as the user expects, after external on canvas edits happened. Such edits are:
➡️ Once back "on the canvas" I have the reasonable expectation that canvas keyboard shortcuts work (again). We can solve this by attempting to restore canvas focus when overlays etc. are closed. |
c176043
to
ea7051f
Compare
The solution with focus on hover works well. I will prepare alpha release and work on integrating it down the stream. Known issues:
|
9037f13
to
06c1822
Compare
Keyboard is now implicitly bound to the canvas element (svg). Legacy configuration via `keyboard.bindTo` config or by passing an element to `Keyboard#bind()` results in a descriptive error to be raised. BREAKING CHANGES: * Keyboard is now implicitly bound to the (focusable) canvas parent. Prior usages result in human readable errors to be raised.
0e7e8cc
to
b439ec6
Compare
@@ -229,8 +228,14 @@ Canvas.prototype._init = function(config) { | |||
height: '100%' | |||
}); | |||
|
|||
assignStyle(svg, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[q]: Why do we do this here, rather than via CSS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Camunda Modeler style was overruling any attempt to set it in diagram-js other than inline like that.
lib/features/popup-menu/PopupMenu.js
Outdated
@@ -262,6 +262,8 @@ PopupMenu.prototype.close = function() { | |||
|
|||
this.reset(); | |||
|
|||
this._canvas.focus(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Proposal: Consider if an element other than the document was focused, and if not, restore canvas focus.
Rationale: A popup menu action may, i.e. open a modal, and we don't want to take the focus if the modal was focused.
TODO: Also write a test case for such a scenario.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 👍
Do you have an idea how to mock menu closing into some modal-like container?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Create a DOM element outside of the Canvas. Focus it as part of the popup menu action. Observe the result.
Alternatively you could create a DOM element during the click, i.e. an <input autofocus="true" />
and append it to the DOM.
/** | ||
* @type {boolean} | ||
*/ | ||
this._focused = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's also expose this as an API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Useful in the following scenario:
eventBus.on([ 'canvas.focusin', 'canvas.focusout' ], () => {
if (canvas.isFocussed()) {
// bind keyboard listeners
} else {
// unbind keyboard listeners
}
});
Alternatively we can better support this via a more general event canvas.focus.changed
:
eventBus.on([ 'canvas.focus.changed' ], (event) => {
if (event.focussed) {
// bind keyboard listeners
} else {
// unbind keyboard listeners
}
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The later one will simplify things, and better align with canvas.viewbox.changed
that we already emit.
This PR ships the foundations to support cross-browser/application copy and paste:
keyboard
binding implicit / remove ability to bind to any element #661, but later reverted as we could not follow up on the original feature).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 viakeyboard.bindTo
log a descriptive descriptive error:Closes #661