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

Use capture phase in useEscapeKeydown #2761

Merged
merged 2 commits into from
Mar 6, 2024
Merged

Conversation

benoitgrelard
Copy link
Collaborator

Fixes #2653

@benoitgrelard benoitgrelard merged commit d7f5ed5 into main Mar 6, 2024
5 checks passed
@benoitgrelard benoitgrelard deleted the escape-keydown-capture branch March 6, 2024 14:55
@jaknas
Copy link

jaknas commented Mar 13, 2024

Hey @benoitgrelard , FYI this change introduced an issue for us, where an Ariakit's Combobox is used inside Radix-UI Dialog. Previously, I was using event.stopPropagation() in Combobox's hideOnEscape prop, which stopped the Radix-UI dialog from closing while the Combobox Popover is open.

Right now after this change to make it a capturing event listener, the same fix with event.stopPropagation stopped working.

Letting you know that this change is potentially breaking and if there's a workaround you'd suggest.

@diegohaz
Copy link

@jaknas I haven't tested this, but you could try stopping Radix from escaping when an Ariakit popup is open within it:

<Dialog.Content
  ref={ref}
  onEscapeKeyDown={(event) => {
    if (ref.current?.contains("[data-open]")) {
      event.preventDefault();
    }
  }}
>

jgoz added a commit to implydata/radix-primitives that referenced this pull request Mar 20, 2024
commit b32a933
Author: benoitgrelard <[email protected]>
Date:   Thu Mar 14 12:23:48 2024 +0000

    Publish release candidate

commit d312472
Author: Benoît Grélard <[email protected]>
Date:   Thu Mar 14 12:20:17 2024 +0000

    Testing updated transitive dep of react-remove-scroll (radix-ui#2776)

commit b64b9f1
Author: benoitgrelard <[email protected]>
Date:   Tue Mar 12 11:17:33 2024 +0000

    Publish release candidate

commit 8b38903
Author: Benoît Grélard <[email protected]>
Date:   Tue Mar 12 11:13:46 2024 +0000

    [Slider] Add ability to name each thumb for more flexibility (radix-ui#2766)

    Fixes radix-ui#2454

commit be80c2a
Author: benoitgrelard <[email protected]>
Date:   Thu Mar 7 10:28:17 2024 +0000

    Publish release candidate

commit 4736d55
Author: Benoît Grélard <[email protected]>
Date:   Thu Mar 7 10:24:44 2024 +0000

    Upgrade react-remove-scroll version (radix-ui#2765)

    Fixes radix-ui#1548
    Fixes radix-ui#2367

commit 82eebe9
Author: benoitgrelard <[email protected]>
Date:   Wed Mar 6 15:04:36 2024 +0000

    Publish release candidate

commit 38f7f14
Author: Benoît Grélard <[email protected]>
Date:   Wed Mar 6 15:00:54 2024 +0000

    [DropdownMenu] Prevent scroll on initial menu focus (radix-ui#2762)

    Fixes radix-ui#2331

commit 834330f
Author: benoitgrelard <[email protected]>
Date:   Wed Mar 6 14:59:19 2024 +0000

    Publish release candidate

commit d7f5ed5
Author: Benoît Grélard <[email protected]>
Date:   Wed Mar 6 14:55:08 2024 +0000

    Use capture phase in `useEscapeKeydown` (radix-ui#2761)

    * Use capture phase

    * Create 75dcd823.yml

commit 2790136
Author: benoitgrelard <[email protected]>
Date:   Tue Mar 5 14:13:07 2024 +0000

    Publish release candidate

commit ad69155
Author: Benoît Grélard <[email protected]>
Date:   Tue Mar 5 14:09:51 2024 +0000

    [Label] Don't eagerly prevent double-click (radix-ui#2753)

    * [Label] Don't eagerly prevent double-click

    Fixes radix-ui#2656

    * Update Label.stories.tsx

    * Update Label.tsx

    * PR feedback

commit b5b3162
Author: Benoît Grélard <[email protected]>
Date:   Tue Mar 5 14:09:13 2024 +0000

    Update CODEOWNERS

commit 7d884d2
Author: benoitgrelard <[email protected]>
Date:   Fri Mar 1 22:14:10 2024 +0000

    Publish release candidate

commit f58a28c
Author: Nicholas Chiang <[email protected]>
Date:   Fri Mar 1 15:10:32 2024 -0700

    fix(Popper): disable pointer events when hidden (radix-ui#2745)

    * fix(Popper): disable pointer events when hidden

    This patch sets `pointer-events: none` when the `<PopperContent>` is
    hidden so that the UI behaves as if it is not there at all. This ensures
    that users can interact with elements beneath the `<PopperContent>`
    uninterrupted.

    Ref: radix-ui#2743 (comment)

    Fixes: e5ba0d9 ("fix(Popper): use `visibility` to hide instead of `opacity` (radix-ui#2744)")

    * Move to wrapper

    ---------

    Co-authored-by: Benoît Grélard <[email protected]>

commit 8b1162c
Author: benoitgrelard <[email protected]>
Date:   Thu Feb 29 19:43:17 2024 +0000

    Publish release candidate

commit e5ba0d9
Author: Nicholas Chiang <[email protected]>
Date:   Thu Feb 29 12:39:51 2024 -0700

    fix(Popper): use `visibility` to hide instead of `opacity` (radix-ui#2744)

    * fix(Popper): use `visibility` to hide instead of `opacity`

    When using `hideWhenDetached`, the opacity is changed to 0 instead of the
    visibility. This lets users click items that they cannot see. This is
    generally a bad user experience.

    This patch updates the `Popper` to set the `visibility` to `hidden` when
    `hideWhenDetached` is used. This ensures the user cannot interact with
    the hidden element.

    This aligns with what is in the `@floating-ui/react-dom` documentation.

    Ref: https://floating-ui.com/docs/hide#usage

    Closes: radix-ui#2743

    * Update packages/react/popper/src/Popper.tsx

    * Create 5793010b.yml

    ---------

    Co-authored-by: Benoît Grélard <[email protected]>

commit ef4cc7d
Author: benoitgrelard <[email protected]>
Date:   Thu Feb 29 13:44:52 2024 +0000

    Publish release candidate

commit fdc34ad
Author: Benoît Grélard <[email protected]>
Date:   Thu Feb 29 13:38:47 2024 +0000

    [NavigationMenu] Remove unsuported `disableOutsidePointerEvents` prop (radix-ui#2741)

    * [NavigationMenu] Remove unsuported `disableOutsidePointerEvents` prop

    Fixes radix-ui#2731

    * Trigger status?

    * Revert "Trigger status?"

    This reverts commit 3c827c0.

commit 8e4dfde
Author: benoitgrelard <[email protected]>
Date:   Wed Feb 28 17:26:11 2024 +0000

    Publish release candidate

commit 1fbb93c
Author: Benoît Grélard <[email protected]>
Date:   Wed Feb 28 17:22:06 2024 +0000

    [RovingFocusGroup] Move only with single arrow keys (radix-ui#2739)

    Fixes radix-ui#2732

commit f243570
Author: benoitgrelard <[email protected]>
Date:   Wed Feb 28 16:40:05 2024 +0000

    Publish release candidate

commit 57c1450
Author: Daniel Kremniov <[email protected]>
Date:   Wed Feb 28 18:36:08 2024 +0200

    feat: added CSP nonce prop (radix-ui#2728)

commit ddb0a12
Author: Benoît Grélard <[email protected]>
Date:   Wed Feb 28 14:39:00 2024 +0000

    yarn workspaces foreach default (radix-ui#2737)

commit a8fa795
Author: Benoît Grélard <[email protected]>
Date:   Wed Feb 28 14:08:13 2024 +0000

    Upgrade node/yarn/storybook (radix-ui#2736)

commit c31c972
Author: Benoît Grélard <[email protected]>
Date:   Mon Sep 25 15:10:49 2023 +0100

    New release

commit c578e3f
Author: benoitgrelard <[email protected]>
Date:   Mon Sep 25 13:44:53 2023 +0000

    Publish release candidate

commit fadebe7
Author: Benoît Grélard <[email protected]>
Date:   Mon Sep 25 14:37:30 2023 +0100

    [ScrollArea] Fix types (radix-ui#2408)

commit 28bebf2
Author: andy-hook <[email protected]>
Date:   Wed Sep 6 11:03:49 2023 +0000

    Publish release candidate

commit 1ccd02f
Author: Dott <[email protected]>
Date:   Wed Sep 6 19:55:24 2023 +0900

    Fix issue link in contribution guide (radix-ui#2381)

    * document: fix duplicated contribution guide open issues page link

    * run yarn version check as declined

commit ea63769
Author: andy-hook <[email protected]>
Date:   Wed Aug 16 13:26:17 2023 +0000

    Publish release candidate

commit baf9862
Author: Alexey Ryabov <[email protected]>
Date:   Wed Aug 16 16:18:41 2023 +0300

    [Avatar] Fix flashing of broken image (radix-ui#2340)

    * [Avatar] Fix flashing of broken image

    * Use isomorphic `useLayoutEffect`

    * [Avatar] Bump patch version

commit 3e0642e
Author: andy-hook <[email protected]>
Date:   Tue Aug 8 10:17:02 2023 +0000

    Publish release candidate

commit 2bc8f49
Author: Andy Hook <[email protected]>
Date:   Tue Aug 8 11:10:35 2023 +0100

    Update readme image (radix-ui#2328)

    * Update readme image

    * Versions
@jaknas
Copy link

jaknas commented May 17, 2024

@jaknas I haven't tested this, but you could try stopping Radix from escaping when an Ariakit popup is open within it:

<Dialog.Content
  ref={ref}
  onEscapeKeyDown={(event) => {
    if (ref.current?.contains("[data-open]")) {
      event.preventDefault();
    }
  }}
>

@diegohaz Thanks for help! Unfortunately it didn't work. First, contains needs to be called with a Node. This can be solved with using querySelector first:

<Dialog.Content
  ref={ref}
  onEscapeKeyDown={(event) => {
    const element = ref.current.querySelector("[data-open]");
    if (ref.current?.contains(element)) {
      console.log("Calling preventDefault()");
      event.preventDefault();
    }
  }}
>

This seems to work when event.target (or focus) is on the combobox input, but it fails to work when event.target is on combobox option in the popover. In that case, the popover doesn't close at all – as if Ariakit internally checks the preventedDefault property? Here's a quick screencast presenting that:

combobox-dialog.mov

@SebKranz
Copy link

SebKranz commented Jul 9, 2024

We're also running into this issue with the HeadlessUI-Combobox.

I'm not sure if handling escape in the capturing-phase is a good idea. From what I understand, it breaks compatibility with every component that doesn't use Radix's <DismissableLayer>.

By being a capturing listener at the root, there's basically no way for other components to intercept the event unless we manually add the onEscapeKeyDown prop to every component using <DismissableLayer> everywhere. Since the DismissableLayerContext is internal, I don't see a way to integrate third-party components into Radix's layering system.

From the discussion, it seems like this change was made to support the inverse of this use-case, under the assumption that it would be a harmless change (#2653 (comment)). Maybe it would be worth reconsidering that decision.

For anyone looking for a workaround: I decided to override the version back to 1.0.3 for now.

// package.json
{ 
  "name":  ...
  "dependencies": {
    ...
  },
  "overrides": {
    "@radix-ui/react-use-escape-keydown": "1.0.3"
  },
}

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