-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Mobile Web Safari: Adds auto scroll back - Issue 5894 #6413
Mobile Web Safari: Adds auto scroll back - Issue 5894 #6413
Conversation
abff303
to
e04c6a7
Compare
@tgolen I'm not sure if |
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 have one major critique of this implementation: it seems very brittle and that it will not be very future-proof. I would love to see a responsive implementation that does not check against exact pixel heights of various iPhone models. Looking at this code, it seems like that should be possible.
Other than that, I have a few more minor notes/suggestions here:
- The
autoScrollback/index.js
file should at least contain a comment explaining whatautoScrollback
does and why it doesn't have a non-web implementation. - Maybe we should rename
autoScrollback
tomobileSafariAutoScrollback
- Wherever you're using
someString.indexOf('someOtherString') > 0)
, we can instead useStr.contains(somString, 'someOtherString')
fromexpensify-common
, to improve readability and consistency. - You've also got some lint errors.
@roryabraham So, did a review, made some extra tests, and managed to find a better solution IMHO. Sadly it still not a silver bullet (it's hard to find the sweet spot UI wise), it should cover properly iPhone 11, iPhone 12, iPhone 13, including Mini, Pro, Pro Max with a smaller complexity. |
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.
First of all, I don't think we need to use the smooth-scroll polyfill. Could you please tell me why did you use it?
I doubt the implementation...Looks very volatile.
@parasharrajat So, I mentioned previously ( #5894 (comment) ) and it seems to me that the team were OK with that. Now, the main reason we need it is cause Safari doesn't support Smooth Scroll ( https://caniuse.com/css-scroll-behavior ), but I must admit that library was just one I found at that moment. I recently found another ( https://github.com/CristianDavideConte/universalSmoothScroll ) that may be more recent. About the implementation, I did test each of the devices there to be sure it was working as expected. |
But you are using javascript scrollTo https://developer.mozilla.org/en-US/docs/Web/API/Window/scrollTo#browser_compatibility
I will add to what Rory said, I would love to see a responsive implementation that does not check against exact pixel heights of various iPhone models. |
@parasharrajat I'll revisit it, but I did try the |
@parasharrajat @roryabraham So, in the end, I did found a workaround and with that I could drop the extra lib. It isn't as smooth as I would like, but I think it isn't a big deal (of course I can add the polyfill back at any time) |
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 am yet to test it on a device..
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.
The code looks good.
Edit:
Not tested on the device yet.
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.
Few comments:
-
Overall, the code is pretty simple and responsive, so great job there 👍
-
Let's add the smoothscroll polyfill back. I tested this out on a physical device and the "jumpy" scroll feels like a glitchy UX. The goal here is to simulate an experience that's as close to the default iOS behavior as possible. The smooth scrolling was definitely better.
-
Let's rename this library to
iOSSafariAutoScrollback
, just to make it extra clear what it's all about. -
It's pretty bizarre that we're importing a library that just does things totally by side-effect. Instead, I think that
autoScrollback
should default-export a function like so:export default function () { if (getBrowser() === CONST.BROWSER.SAFARI && Str.contains(userAgent, 'iphone os 1')) { document.addEventListener('touchstart', touchStarted); document.addEventListener('touchend', scrollbackAfterTouch); document.addEventListener('scroll', scrollbackAfterScroll); document.addEventListener('focusin', startsWaitingForScroll); document.addEventListener('focusout', stopsWaitingForScroll); } }
Of course,
index.js
would just export a noop:export default () => {};
Then
App.js
would import and execute that function like so:import initializeiOSSafariAutoScrollback from './libs/iOSSafariAutoScrollback'; ... ... initializeiOSSafariAutoScrollback();
-
Do we still plan to follow-up and get similar code merged directly into
react-native-web
?
Totally forgot about this tbh. Should I reach them about it? You guys wanna do it? |
LGTM, I think we can merge this code in our repo, then do a follow-up to:
Do you think you can handle that? I think it's fair to create a separate Upwork milestone with a separate (smaller) payout, if that's something you'd be interested in. |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
@roryabraham cc |
initializeiOSSafariAutoScrollback(); | ||
|
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.
If this is something that's only intended to run on web, maybe you should consider importing and calling it only inside src/setup/platformSetup/index.website.js
I'll would even go as far as
export default function () {
// AppRegistry.runApplication(
// ...
if (getBrowser() === CONST.BROWSER.SAFARI || Str.contains(userAgent, 'iphone os 1')) {
const applyScrollFix = require('../libs/iOSSafariAutoScrollback');
applyScrollFix();
}
This way extra code is imported only when needed
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.
@kidroca it is merged already... but that's a GREAT suggestion!
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.
Great suggestion @kidroca! If either of you wouldn't mind submitting a quick PR, I'll review it.
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.
Thanks guys!
@roryabraham I'll open a PR about this today, when I get the chance
|
||
|
||
export default function () { | ||
if (!getBrowser() === CONST.BROWSER.SAFARI || !Str.contains(userAgent, 'iphone os 1')) { |
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.
Isn't this supposed to be
getBrowser() !== CONST.BROWSER.SAFARI
!getBrowser()
would be evaluated to boolean
otherwise, so it's most probably the second part of the check that is doing all the work here
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.
WOW. It should be getBrowser() == CONST.BROWSER.SAFARI
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.
So we want to exist early if it regular safari, and only apply the polyfill for mobile ?
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.
This is an early return
condition
I though it tried to exist early if the browser is not Safari or mobile safari
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.
IT should only work on Safari web.
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.
OMG 🤦♂️ how I did that?!
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.
@kidroca if possible, load that file only for Website + Safari
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.
Hey I'm slightly confused I thought the issue is "Mobile Web Safari: Adds auto scroll back"
Which mean we're only targeting ios Safari (iPhone & iPads) and should apply the fix there, correct?
My plan is to use this expression
if (_.every([/iP(ad|hone)/i, /WebKit/i, /CriOS/i], re => re.test(navigator.userAgent))) {
const applyAutoScrollbackFix = require('../../libs/iOSSafariAutoScrollback');
applyAutoScrollbackFix();
}
The check is based on: https://stackoverflow.com/a/29696509/4834103
I can extract
const isMobileSafari = () => _.every([/iP(ad|hone)/i, /WebKit/i, /CriOS/i], re => re.test(navigator.userAgent));
But I don't know where to put it
Since it's only one time usage so far, I think I'll just keep it inline
Otherwise getBrowser
seems to be a nice place, but I'm not sure how exactly to fit this expression there, or just add a isMobileSafari
check
Hey, guys I've checked out 1 For some reason the text field is covered by the keyboard (only happened once) rec1.mov2 Keyboard is partially covering input rec2.movThis is without applying any modifications to the code and after |
1st seems like an issue , I didn't face it as I think I tested it on old safari where the address field is at the top. |
@parasharrajat I tested both in iOS14 and 15, will take a look! |
@kidroca just in case, do you mind sharing the exact commit you were testing? |
Yes, it was: 0360fd7 |
@kidroca @parasharrajat #6572 I couldn't reproduce it, still addressed discussed issues. |
I will try to reproduce. |
🚀 Deployed to staging by @roryabraham in version: 1.1.17-8 🚀
|
🚀 Deployed to production by @roryabraham in version: 1.1.18-3 🚀
|
Details
On Mobile Web Safari, if you scroll up too much when the keyboard is open, this code will scroll back automatically
Fixed Issues
$ #5894
Tests
Open mobile web, go to composer with software keyboard active, scroll up as much as you can
QA Steps
Tested On
Recordings