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

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

Closed
tianyingchun opened this issue Jul 23, 2022 · 16 comments
Closed

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

tianyingchun opened this issue Jul 23, 2022 · 16 comments

Comments

@tianyingchun
Copy link

tianyingchun commented Jul 23, 2022

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 #1538

@tianyingchun
Copy link
Author

It should be issue :maybe

@zawilliams
Copy link

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.

@tianyingchun
Copy link
Author

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
Copy link
Author

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

@HIT2022
Copy link

HIT2022 commented Aug 1, 2022

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.

@mshick
Copy link

mshick commented Aug 1, 2022

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}>

@HIT2022
Copy link

HIT2022 commented Aug 2, 2022

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.

@mshick
Copy link

mshick commented Aug 2, 2022

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}>

@tianyingchun
Copy link
Author

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 :)

@denniswanjiru
Copy link

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.

@tianyingchun
Copy link
Author

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

@RobinMalfait
Copy link
Collaborator

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.

Screen.Recording.2022-08-31.at.13.31.45.mov

@RobinMalfait
Copy link
Collaborator

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.

@HIT2022
Copy link

HIT2022 commented Aug 31, 2022

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.

@RobinMalfait
Copy link
Collaborator

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:

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!

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

No branches or pull requests

6 participants