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

Adds support for Safari on iOS, fixes #17 #18

Merged
merged 4 commits into from
Jun 25, 2020

Conversation

kennethormandy
Copy link
Contributor

I was able to reproduce the issue described in #17 using Safari on iOS 13.

The support check seemed correct from my testing, and all that was missing was a timeout so restoreScrollPosition will happen after nativeFocus.

This PR adds a 100ms timeout, which fixes the issue. Some comments on this Stack Overflow post suggest that’s also necessary for older versions of IE.

focus-options-polyfill-test

I also updated the test case to add an input, which worked better as a test for touch devices. When the polyfill is working, the keyboard will open automatically.

@calvellido calvellido self-requested a review June 22, 2020 18:24
@calvellido
Copy link
Owner

Thanks a ton for taking care of that @kennethormandy! Not in the PC at the moment, but I will try to have a look and merge it tomorrow 🙂

Copy link
Owner

@calvellido calvellido left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kennethormandy this approach is great, but I'm detecting a bit of flashing (basically the scrolling down being seen) on the behaviour, as those 100ms seems enough in CPU powerful enough devices for the scroll to go down and then get back. Paying attention, it can also be detected on you screen capture.

This flashing might not be ideal for the rest of cases of the polyfill, so we might need to just enable it for the iOS Safari cases, where this would be the price to pay for this to work.

Or, in my checks, a timeout value of 0 is also making this to work, while keeping the same behaviour for the rest of cases. Could you try it?

Thanks!

@kennethormandy
Copy link
Contributor Author

Good call, I’ve changed it to 0, and that also works on iOS without the screen flash.

Copy link
Owner

@calvellido calvellido left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏👏👏

@calvellido calvellido merged commit 3285d2b into calvellido:master Jun 25, 2020
@calvellido calvellido added the enhancement New feature or request label Jun 25, 2020
@calvellido
Copy link
Owner

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants