headlessui icon indicating copy to clipboard operation
headlessui copied to clipboard

Closing Dialog breaks site on Safari mobile (<ios 14)

Open tianyingchun opened this issue 3 years ago • 9 comments

Closing a dialog in Safari on iOS 13.2 renders my app unusable. The problem seems to be the node

...
which stays in the DOM even after closing the dialog. In other browsers, this node is removed from the DOM.

What package within Headless UI are you using?

"@headlessui/react": "1.6.6",

What browser are you using?

Safari ios 13

Reproduction URL

  1. clone repo https://github.com/tianyingchun/headlessui
  2. yarn install
  3. open http://localhost:3000/ on the modern mobile safari (ios15) it works fine .
  4. open http://localhost:3000/ on the safari of ios 13 click Open Sidebar it will popup modal, while close dialog please see below dom image

while sidebar modal is open, and try to close it , and then try to re-click Open Sidebar, the web page is no response. the reasons as below picture shown.

image

Describe your issue while Dialog closed(transition end) it should clean <div id="#headlessui-portal-root" />, && remove <html style="overflow: hidden;" /> at safari of (ios <14 )

related existed issue https://github.com/tailwindlabs/headlessui/issues/1538

tianyingchun avatar Jul 23 '22 15:07 tianyingchun

It should be <Transition /> issue :maybe

tianyingchun avatar Jul 24 '22 08:07 tianyingchun

Yes this is happening for me as well. Seems to be not only Safari but just small breakpoints in general. In Chrome the Dialog component does the same thing at small breakpoints. You can press the Escape key multiple times and it will trigger opening and closing the Dialog as many times as you want to.

zawilliams avatar Jul 26 '22 05:07 zawilliams

it seems that This is a serious problem, and it will break all user operations on website, i preparing migrate all Dialog/Transiton to [mui](https://mui.com/) it seems that They are more stable

tianyingchun avatar Jul 26 '22 05:07 tianyingchun

tailwindcss is amazing but tailwindui update/bug fix / corresponding very very slow. :(

tianyingchun avatar Jul 26 '22 05:07 tianyingchun

This issue also affects an older Electron/Chromium browser I use. I've been force closing the Dialog component via onClose as a workaround to remove Dialog.Overlay, but doing so of course eliminates the exit Transition, making this workaround non-ideal.

HIT2022 avatar Aug 01 '22 14:08 HIT2022

Also experiencing this on latest Chrome. As mentioned, the Dialog component does not seem to clean up after itself when combined with Transition (as the TailwindUI code does).

I was able to work around this by setting the Dialog open prop as well as the Transition that wraps it.

E.g., from this:

    <Transition.Root show={isOpen} as={Fragment}>
      <Dialog as="div" className="relative z-10" onClose={onClose}>

To this:

    <Transition.Root show={isOpen} as={Fragment}>
      <Dialog as="div" className="relative z-10" open={isOpen} onClose={onClose}>

mshick avatar Aug 01 '22 19:08 mshick

To this:

    <Transition.Root show={isOpen} as={Fragment}>
      <Dialog as="div" className="relative z-10" open={isOpen} onClose={onClose}>

Oddly, for me this does clean up the overlay but it also kills off the exit transition.

HIT2022 avatar Aug 02 '22 04:08 HIT2022

It's a little more involved, but maybe this would preserve the exit transition?

const [dialogIsOpen, setDialogIsOpen] = useState(isOpen);

return (
    <Transition.Root show={isOpen} as={Fragment} afterLeave={() => setDialogIsOpen(false)}>
      <Dialog as="div" className="relative z-10" open={dialogIsOpen} onClose={onClose}>

mshick avatar Aug 02 '22 14:08 mshick

i think the transition implementaiton is bad, it seems that it not easy to fix. i have just give it up, cause of "no multiple Dialog" context support, "no transition dialog campaitible for old safari browser", and "slowly feedback for these bug", i prepare to switch to mui it seems it has powerfull flexiblity and stable feature :)

tianyingchun avatar Aug 02 '22 14:08 tianyingchun

It's a little more involved, but maybe this would preserve the exit transition?

const [dialogIsOpen, setDialogIsOpen] = useState(isOpen);

return (
    <Transition.Root show={isOpen} as={Fragment} afterLeave={() => setDialogIsOpen(false)}>
      <Dialog as="div" className="relative z-10" open={dialogIsOpen} onClose={onClose}>

After doing this I am encountering this error

Unhandled Runtime Error
Error: Did you forget to passthrough the `ref` to the actual DOM node?

Do you have any idea on how to fix it.

denniswanjiru avatar Aug 13 '22 02:08 denniswanjiru

@DennisWanjiru no idear for this at now, i am doing migration all front end code from headlessui to mui

tianyingchun avatar Aug 13 '22 02:08 tianyingchun

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

First of all sorry for the slow response. I've been looking into this issue and this is currently happening because you are on iOS 13.2. Internally we use the transitionend, transitionrun and transitioncancel events and some of those have been introduced in iOS 13.4. (They technically existed since iOS 12 but those were never being called).

Even if you don't want to update to iOS 14, 15 or even 16, I think updating to iOS >= 13.4 should solve this issue already for iOS devices.

--

@HIT2022 which version of Electron / Chrome are you using? Can you create a minimal reproduction repo so that we can take a look at this problem?

--

@mshick the example you showed technically works, but it bypasses the Transition completely so your "leave" transitions will be instant because the Dialog will already be closed before the Transition component gets the time to transition out.

You also mentioned "latest Chrome" could you create a minimal reproduction repo of that behaviour? Here is a little video running the original reproduction from @tianyingchun on the latest Chrome version which seems to work fine.

https://user-images.githubusercontent.com/1834413/187669187-1f479ebc-893f-48f3-a9d4-3c340467298a.mov

RobinMalfait avatar Aug 31 '22 11:08 RobinMalfait

@RobinMalfait here is the user agent string - https://user-agents.net/string/mozilla-5-0-windows-nt-6-3-win64-x64-applewebkit-537-36-khtml-like-gecko-colibri-1-10-0-chrome-66-0-3359-181-electron-3-0-10-safari-537-36

I can re-create via https://headlessui.com/react/dialog -

headless

HIT2022 avatar Aug 31 '22 17:08 HIT2022

Ah perfect thanks @HIT2022. Looking at that one it looks like you are on Electron version 3 (which is 15 versions behind) and Chrome version 66 (which is 40 versions behind) and came out in April of 2018.

This is also the source of the same issue as iOS 13.2 issues because transitionrun which we rely on was introduced in Chrome 74 (https://developer.mozilla.org/en-US/docs/Web/API/Element/transitionrun_event) same for transitioncancel which ws introduced in Chrome 87 (https://developer.mozilla.org/en-US/docs/Web/API/Element/transitioncancel_event)

I don't know if we will find a good solution or even want to support such old versions. But at a minimum I would recommend you to update at least a few versions because you are behind quite a a lot and many of those upgrades contain important security fixes as well.

RobinMalfait avatar Aug 31 '22 18:08 RobinMalfait

Unfortunately updating versions is not possible as my app's userbase is comprised of users of a commercial release of Electron/Chrome that was deprecated following the release of a replacement (yet heavily panned) software. But I understand & would of course not expect 4 year old browser implementations to still be supported. Thanks for the detail.

HIT2022 avatar Aug 31 '22 18:08 HIT2022

If we find a solution that solves these issues (e.g.: not using native transition* events) that doesn't hold us back or isn't a big maintenance burden then I'm happy to implement those. But I can't promise an ETA or if we will find a solution to these problems.

Since the original issue is also about an old version of iOS I'm going to close this issue because of the aforementioned reasons. I know there are still some issues around the Transition component but they are tracked in other issues like:

  • #1557
  • #1638

Those will be solved in one of the next PRs.


If anyone has other issues feel free to open new issues with minimal reproduction repo(s) attached so that we can take a look at those. Hope you all understand!

RobinMalfait avatar Aug 31 '22 19:08 RobinMalfait