-
-
Notifications
You must be signed in to change notification settings - Fork 834
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
Use absolute positioning for the Composer on Safari #2660
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting that this introduces a system for introducing workarounds to safari mobile. Hopefully we won't have to use it too much 😨
I've gotten my hands on an old iphone, I'll test later today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed that this fixes the issue. Code looks good to me 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a Safari Mobile device to test.
I don't like the idea of having Safari-specific code in there because that means changing it will require having someone with Safari to check if it's still needed, but if there's no other solution so be it. Looks ok.
f5ac9cb
to
c4f726b
Compare
I agree, I really don't like having browser specific code either, but there doesn't seem to be a solution for both safari & non-safari, seems like it's either this, or choosing one bug over the other. If we do go ahead with this solution, It'd be best not to use this util anywhere else unless really forced to such as this case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to approve even though I have no way of testing 😇
I can confirm that this fixes the issue after having a friend test on nightly. |
Fixes #2652
Changes proposed in this pull request:
Use absolute positioning on Safari mobile, we can't use absolute positioning in all browsers because that causes other issues, however Safari mobile doesn't handle fixed positioning and inputs well either.
Reviewers should focus on:
I don't really like this tbh, there is no reliable way to tell what browser is used, the user agent string might change in the future, and feature inference (testing for specific features unique to the browser) is also not reliable as features might also be removed in the future.
I would have preferred trying to fix this on the JS side by maybe checking the composer position isn't off the viewport or perhaps trying somethings with a timeout before focusing on the text editor, but not being able to reproduce and test the issue doesn't help.
If anyone else has access to an iOS device and able to reproduce this wants to try and find a better fix please feel free.
Confirmed