Skip to content
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

fix: port implicit keyboard binding over to decision editors #921

Merged
merged 5 commits into from
Nov 15, 2024

Conversation

jarekdanielak
Copy link
Contributor

Closes #920

Proposed Changes

Adjust Keyboard modules in dmn-js-decision-table and dmn-js-literal-expression to follow new implicit keyboard binding behavior.

Change the test for keyboard binding to subscript onto events instead of directly checking internal property.

Checklist

To ensure you provided everything we need to look at your PR:

  • Brief textual description of the changes present
  • Visual demo attached
  • Steps to try out present, i.e. using the @bpmn-io/sr tool
  • Related issue linked via Closes {LINK_TO_ISSUE} or Related to {LINK_TO_ISSUE}

@jarekdanielak jarekdanielak self-assigned this Nov 14, 2024
@bpmn-io-tasks bpmn-io-tasks bot added the needs review Review pending label Nov 14, 2024
Copy link
Member

@nikku nikku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see that the viewer-container or any other element is actively keyboard focusable. What is our strategy here?

What I see is that when the bpmn-io overlay is focused the undo/redo still triggers; that is somewhat unexpected, too:

capture SK8FKK_optimized

Could you elaborate how this currently is supposed to work?

@jarekdanielak
Copy link
Contributor Author

The goal of this pull request was to simply make sure the configuration in dmn-js is aligned with the new configuration in diagram-js, so that there is no need to explicitly specify bindTo parameter for any keyboard abstraction that we provide.

It doesn't change or fix any of the flaws we have now. The undo behavior you are showing can be reproduced with current version too. I think this one actually follows native browser behavior - you don't have to have an input focused in order to undo in it. See blow:

Screen.Recording.2024-11-14.at.18.05.31.mov

I agree some of undo/redo behaviors are unexpected. I've recently investigated it as a part of this Undo/redo in BKM issue.

In effort to wrap up integration of diagram-js@15 in our stack, I decided to only remove deprecated configuration this time and leave improvements for later.

Copy link
Member

@nikku nikku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I propose we link bpmn-io/diagram-js#662 as a breaking change resource.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants