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

Issues when using drawer with autofocus input on iOS #258

Merged
merged 4 commits into from
Apr 6, 2024

Conversation

ze-kel
Copy link
Contributor

@ze-kel ze-kel commented Feb 6, 2024

I was trying to use drawer like this:

  1. user clicks an "edit" button
  2. drawer opens with a single input that has autofocus. keyboard opens simultaneously
  3. user clicks away or clicks done. input calls onBlur, drawer closes

While trying to do this I encountered problems

  1. Animations are janky while opening or closing a keyboard on iOS
  2. Auto focus is broken on ios when using controlled drawer(open + onOpenChange)
  3. There is a noticeable delay when closing the drawer in this scenario

For #1 I've added a prop to disable usePreventScroll.

We can theoretically try to stagger both in animation and keyboard closing or try to implement some sort of auto detection for "keyboard and drawer open at the same time" case, but I'm not sure it's worth it.

Also I am aware that setting modal=false will also disable usePreventScroll, however I do need focus trapping inside component that radix does when modal is true.


#2 and happens due to extra renders when the following happens:

  1. User opens the modal
  2. onOpenChange(true) fires
  3. Drawer renders with openProp=true and isOpen=false
  4. useEffect works and sets isOpen to true
  5. Drawer renders with openProp=true and isOpen=true and is now opened

A far as I understand this extra render makes iOS think that input with autofocus appeared not as a result of a user action and therefore it ignores autofocus.

This can be fixed simply by making internal value changes simultaneously with onOpenChange.
Update: after looking at tests and thinking some more I realised that my approach was wrong. We can't just assume that onOpenChange will change the value to whatever it's called with. Maybe someone has a form in a drawer that prevents user from closing it until all fields are validated or something like that.
A better way to solve this is by using useLayoutEffect. It's the same hook except when fired it will fire before rendering, set internal prop and the render correctly.


Both of these also fixed #3 and made it way smoother.

Videos for reference before and after.

before.MP4

^notice that keyboard closes first and then after some delay the drawer also disappears

after.MP4

Copy link

vercel bot commented Feb 6, 2024

@ze-kel is attempting to deploy a commit to the emil Team on Vercel.

A member of the Team first needs to authorize it.

@emilkowalski
Copy link
Owner

This is amazing, can we document this prop in the readme and ensure the tests pass?

@ze-kel
Copy link
Contributor Author

ze-kel commented Feb 6, 2024

Added prop to readme.
Found a better way of solving second issue, updated head post

This can be fixed simply by making internal value changes simultaneously with onOpenChange.
Update: after looking at tests and thinking some more I realised that my approach was wrong. We can't just assume that onOpenChange will change the value to whatever it's called with. Maybe someone has a form in a drawer that prevents user from closing it until all fields are validated or something like that. A better way to solve this is by using useLayoutEffect. It's the same hook except when fired it will fire before rendering, set internal prop and the render correctly.

This also fixed all tests, except for one:

  1. [iPhone] › tests/base.spec.ts:53:7 › Base tests › should not close when dragged up

However this one fails for me even on main branch. Not sure why, maybe that will pass on CI env.

@ze-kel
Copy link
Contributor Author

ze-kel commented Feb 6, 2024

Looked into broken test. It was caused by using event.screenY(screenX) instead of event.clientY(clientX). I replaced it in all functions. The difference is basically that screenY accounts for zoom level. I guess playwright was running tests with browser zoom for some reason.

This also causes incorrect distance estimation when browser zoom is not 100%. Here's before and after:

Screen.Recording.2024-02-06.at.23.29.03.mp4

Now all tests pass locally on my machine, even the commented one('should close when dragged down') which I uncommented,

@Fr0stmourne
Copy link

Fr0stmourne commented Feb 22, 2024

Our team has come across first issue as well, for now we're using patch-package fix to disable usePreventScroll. Would be great if this one was merged

@emilkowalski emilkowalski merged commit 7e340d3 into emilkowalski:main Apr 6, 2024
1 of 2 checks passed
@ReangeloJ
Copy link

@emilkowalski It hasn't been merged in main yet.

image

@ant1m4tt3r
Copy link

@Fr0stmourne did you patched the minified published version? i'm having some trouble doing so while this commit isn't published.

@ant1m4tt3r
Copy link

it was published a few hours ago, thank you!

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

Successfully merging this pull request may close these issues.

5 participants