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

UI: Fix keydown shortcut within shadow tree #24179

Merged
merged 2 commits into from
Sep 20, 2023

Conversation

stropitek
Copy link
Contributor

Closes #23790

What I did

I changed the part in the preview which checks if a keyboard event should be sent or not. Previously this was done using event.target, and I changed that to use event.composedPath()[0] instead, so that stories which render an input / textarea or contenteditable element within a shadow tree don't spuriously trigger storybook shortcuts.

The linked issue has a reproducible example of the bug.

Checklist for Contributors

Testing

I made sure there are no regressions in the e2e tests and updated the unit tests. However I did not implement new tests for stories including a shadow tree. I wasn't sure how to go about this.

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

I did not know how to manually test this in the storybook project directly. I use react and it would require to create a new react-only story and add the react-shadow dependency.

Instead, I checked my change manually within another project:

  • Checkout https://github.com/zakodium-oss/react-science
  • Run npm ci
  • Run npm run storybook
  • Run npx serve storybook-static
  • Go to one of the "Input" stories
  • Type "s" in the input => it toggles the sidebar
  • Edit ./storybook-static/sb-preview/runtime.js, replace let target = event.target; with let target = event.composedPath()[0]
  • Reload the input story and check that when in the input, typing s does not toggle the sidebar as expected. When focused element is within the preview but not the input, type s does toggle the sidebar as expected.

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts

  • Make sure this PR contains one of the labels below:

    Available labels
    • bug: Internal changes that fixes incorrect behavior.
    • maintenance: User-facing maintenance tasks.
    • dependencies: Upgrading (sometimes downgrading) dependencies.
    • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
    • cleanup: Minor cleanup style change. Will not show up in release changelog.
    • documentation: Documentation only changes. Will not show up in release changelog.
    • feature request: Introducing a new feature.
    • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
    • other: Changes that don't fit in the above categories.

🦋 Canary release

This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the @storybookjs/core team here.

core team members can create a canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>

@ndelangen ndelangen self-assigned this Sep 15, 2023
@ndelangen ndelangen changed the title UI: fix keydown focusInInput check when input is in a shadow tree UI: Fix keydown shortcut within shadow tree Sep 15, 2023
@@ -45,7 +45,7 @@ import type { StorySpecifier } from '../store/StoryIndexStore';
const globalWindow = globalThis;

function focusInInput(event: Event) {
const target = event.target as Element;
const target = event.composedPath()[0] as Element;
Copy link
Member

Choose a reason for hiding this comment

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

I figured I'd leave this here for future me:
https://developer.mozilla.org/en-US/docs/Web/API/Event/composedPath

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI, I found this article to be really helpful on the topic of events and shadow dom
https://pm.dartus.fr/blog/a-complete-guide-on-shadow-dom-and-event-propagation/

@stropitek
Copy link
Contributor Author

For the record, if the shadow root is closed, since we don't have access to the original dom node which dispatched the event, there is no way to know if we should or should not send the event through the channel.

With this implementation, the event will always be sent.

@ndelangen
Copy link
Member

@stropitek The custom web-component should maybe be stop the propagation of the event in that case, shouldn't it?

@stropitek
Copy link
Contributor Author

Yes, a custom web-component can stop the propagation of the event to the host (storybook) by calling stopPropagation on the keydown event (I tested it to be sure).

@ndelangen ndelangen merged commit 0d827bf into storybookjs:next Sep 20, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Within shadow tree, shortcuts are not appropriately ignored
3 participants