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

[Bug]: Dialog/Modal does not close by mouse click when wrapped in component with outer onClick event #414

Closed
bennettdams opened this issue Apr 20, 2021 · 4 comments

Comments

@bennettdams
Copy link

bennettdams commented Apr 20, 2021

What package within Headless UI are you using?

@headlessui/react

What version of that package are you using?

v1.0.0

What browser are you using?

Chrome

Reproduction repository

https://codesandbox.io/s/proud-voice-dvdru?file=/src/App.tsx

Describe your issue

When the "Dialog" component is used in a component that is used in a component where an onClick handler is defined, the Dialog's is not closing when clicking on its button/on the overlay. Pressing the escape key works though.

Setup:

  • ChildWithModal component, that brings its own "Dialog"
  • Card component, clickable if needed

Here's an example with two childs. Only difference: in one example clicking the Card component should also open the Dialog.

App.tsx

    const { isOpen: isOpen1, open: open1, close: close1 } = useModal();
    const { isOpen: isOpen2, open: open2, close: close2 } = useModal();
    ...
    <div>
      <Card>
        <ChildWithModal
          isOpen={isOpen1}
          open={open1}
          close={close1}
        />
      </Card>

      <Card onClick={open2}>
        <ChildWithModal
          isOpen={isOpen2}
          open={open2}
          close={close2}
        />
      </Card>
    </div>

Card.tsx

// Generic card, clickable if needed
function Card({
  onClick,
  children
}: {
  onClick?: () => void;
  children: ReactNode;
}) {
  function handleClick(event: MouseEvent<HTMLDivElement>) {
    event.preventDefault();
    event.stopPropagation();
    console.log("Card | onClick", event);

    if (onClick) onClick();
  }

  return (
    <div
      className={`bg-blue-200 h-20 ${onClick && "cursor-pointer"}`}
      onClick={handleClick}
    >
      <div>{children}</div>
    </div>
  );
}

I'm not sure why preventing default/stopping propagation does not help here.

Demonstration

dialog


Full reference, but I would suggest looking at the CodeSandbox:

Click to expand

import "./styles.css";
import { useState, Fragment, ReactNode, MouseEvent, useEffect } from "react";
import { Dialog, Transition } from "@headlessui/react";

/**
 * First example from https://headlessui.dev/react/dialog,
 * just changed variable names and some styling.
 */
function Modal({
  isOpen,
  close,
  title
}: {
  isOpen: boolean;
  close: () => void;
  title: string;
}) {
  return (
    <Transition show={isOpen} as={Fragment}>
      <Dialog
        as="div"
        id="modal"
        className="fixed inset-0 z-10 overflow-y-auto"
        // initialFocus={cancelButtonRef}
        static
        open={isOpen}
        onClose={close}
      >
        <div className="min-h-screen px-4 text-center">
          <Transition.Child
            as={Fragment}
            enter="ease-out duration-300"
            enterFrom="opacity-0"
            enterTo="opacity-100"
            leave="ease-in duration-200"
            leaveFrom="opacity-100"
            leaveTo="opacity-0"
          >
            <Dialog.Overlay className="fixed inset-0 bg-gray-400 bg-opacity-50" />
          </Transition.Child>

          {/* This element is to trick the browser into centering the modal contents. */}
          <span
            className="inline-block h-screen align-middle"
            aria-hidden="true"
          >
            &#8203;
          </span>
          <Transition.Child
            as={Fragment}
            enter="ease-out duration-300"
            enterFrom="opacity-0 scale-95"
            enterTo="opacity-100 scale-100"
            leave="ease-in duration-200"
            leaveFrom="opacity-100 scale-100"
            leaveTo="opacity-0 scale-95"
          >
            <div className="inline-block w-full max-w-md p-6 my-8 overflow-hidden text-left align-middle transition-all transform bg-white shadow-xl rounded-2xl">
              <Dialog.Title
                as="h3"
                className="text-lg font-medium leading-6 text-gray-900"
              >
                Payment successful
              </Dialog.Title>
              <div className="mt-2">
                <p className="text-sm text-gray-500">
                  Your payment has been successfully submitted. We’ve sent your
                  an email with all of the details of your order.
                </p>
              </div>

              <div className="mt-4">
                <button
                  type="button"
                  className="inline-flex justify-center px-4 py-2 text-sm font-medium text-blue-900 bg-blue-100 border border-transparent rounded-md hover:bg-blue-200 focus:outline-none focus-visible:ring-2 focus-visible:ring-offset-2 focus-visible:ring-blue-500"
                  onClick={close}
                >
                  Got it, thanks!
                </button>
              </div>
            </div>
          </Transition.Child>
        </div>
      </Dialog>
    </Transition>
  );
}

// Child component that brings its own modal.
function ChildWithModal({
  isOpen,
  open,
  close,
  title
}: {
  isOpen: boolean;
  open: () => void;
  close: () => void;
  title: string;
}) {
  function handleClick(event: MouseEvent<HTMLDivElement>) {
    event.preventDefault();
    event.stopPropagation();
    console.log("Child | onClick", event);

    open();
  }

  return (
    <>
      <div className="h-full cursor-pointer" onClick={handleClick}>
        <h1>Child with modal</h1>
        <p>{title}</p>
      </div>
      <Modal title={title} isOpen={isOpen} close={close} />
    </>
  );
}

// Generic card, clickable if needed
function Card({
  onClick,
  children
}: {
  onClick?: () => void;
  children: ReactNode;
}) {
  function handleClick(event: MouseEvent<HTMLDivElement>) {
    event.preventDefault();
    event.stopPropagation();
    console.log("Card | onClick", event);

    if (onClick) onClick();
  }

  return (
    <div
      className={`bg-blue-200 h-20 ${onClick && "cursor-pointer"}`}
      onClick={handleClick}
    >
      <div>{children}</div>
    </div>
  );
}

function useModal() {
  const [isOpen, setIsOpen] = useState(false);

  function open() {
    setIsOpen(true);
  }

  function close() {
    setIsOpen(false);
  }

  return { isOpen, open, close };
}

export default function App() {
  const { isOpen: isOpen1, open: open1, close: close1 } = useModal();
  const { isOpen: isOpen2, open: open2, close: close2 } = useModal();

  return (
    <div className="App p-10 space-y-10 px-20">
      <Card>
        <ChildWithModal
          title="Example #1: everything works as expected"
          isOpen={isOpen1}
          open={open1}
          close={close1}
        />
      </Card>

      <Card onClick={open2}>
        <ChildWithModal
          title="Example #2: close button and outside click not working, press escape key"
          isOpen={isOpen2}
          open={open2}
          close={close2}
        />
      </Card>
    </div>
  );
}

@bennettdams bennettdams changed the title [Bug]: Dialog/Modal does not close when wrapped in component with outer onClick event [Bug]: Dialog/Modal does not close by mouse click when wrapped in component with outer onClick event Apr 20, 2021
@bishwenduk029
Copy link

bishwenduk029 commented Apr 21, 2021

@bennettdams , I think the button event is bubbling to the Card and since Card has a click handler so it is getting invoked. The following structure is responsible for the same I guess.

<Card {hasOnClick}>
 <ChildWithModal {noClickHandler, it's child has one but it is sibling of Modal so propagation is not stopped }>
   <Modal {noClickHandler}>
    <button {targetOfClick}>

Now the button click reaches Card and the onClick gets triggered. May be putting some sort of safeguards on useModal could help.

function useModal() {
  const [isOpen, setIsOpen] = useState(false);

  function open() {
    if (!isOpen) setIsOpen(true);
  }

  function close() {
    if (isOpen) setIsOpen(false);
  }

  return { isOpen, open, close };
}

@bennettdams
Copy link
Author

bennettdams commented Apr 21, 2021

May be putting some sort of safeguards

@bishwenduk029 Thanks! Yes, using safeguards does help and fixes the issue.

I'm not that deep into event propagation - shouldn't the bubbling be stoped by the handlers (.stopPropagation())?
Is this a bug in HeadlessUI or did I use it wrong?

@RobinMalfait
Copy link
Member

Hey! Thank you for your bug report!
Much appreciated! 🙏

This should be fixed in #422, and will be available in the next release.
You can already try it using npm install @headlessui/react@dev or yarn add @headlessui/react@dev.

I also updated your reproduction CodeSandbox that includes the fix: https://codesandbox.io/s/heuristic-cdn-k9oiq

RobinMalfait added a commit that referenced this issue Apr 21, 2021
* re-export the `screen` utility for quick debugging purposes

* stop event propagation when clicking inside a Dialog

Fixes: #414
@bennettdams
Copy link
Author

@RobinMalfait What a quick response... Thanks a lot!

RobinMalfait added a commit that referenced this issue Apr 26, 2021
* Fixed typos (#350)

* chore: Fix typo in render.ts (#347)

* Better vue link (#353)

* Better vue link

* add better React link

Co-authored-by: Robin Malfait <[email protected]>

* Enable NoScroll feature for the initial useFocusTrap hook (#356)

* enable NoScroll feature for the initial useFocusTrap hook

Once you are using Tab and Shift+Tab it does the scrolling.

Fixes: #345

* update changelog

* Revert "Enable NoScroll feature for the initial useFocusTrap hook (#356)"

This reverts commit 19590b0.

Solution is not 100% correct, so will revert for now!

* Improve search (#385)

* make search case insensitive for the listbox

* make search case insensitive for the menu

* update changelog

* add `disabled` prop to RadioGroup and RadioGroup Option (#401)

* add `disabled` prop to RadioGroup and RadioGroup Option

Also did some general cleanup which in turn fixed an issue where the
RadioGroup is unreachable when a value is used that doesn't exist in the
list of options.

Fixes: #378

* update changelog

* Fix type of `RadioGroupOption` (#400)

Match RadioGroupOption value types to match modelValue allowed types for RadioGroup

* update changelog

* fix typo's

* chore(CI): update main workflow (#395)

* chore(CI): update main workflow

* Update main.yml

* fix dialog event propagation (#422)

* re-export the `screen` utility for quick debugging purposes

* stop event propagation when clicking inside a Dialog

Fixes: #414

* improve dialog escape (#430)

* Make sure that `Escape` only closes the top most Dialog

* update changelog

* add defaultOpen prop to Disclosure component (#447)

* add defaultOpen prop to Disclosure component

* update changelog

Co-authored-by: Shuvro Roy <[email protected]>
Co-authored-by: Alex Nault <[email protected]>
Co-authored-by: Eugene Kopich <[email protected]>
Co-authored-by: Nathan Shoemark <[email protected]>
Co-authored-by: Michaël De Boey <[email protected]>
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

No branches or pull requests

3 participants