Mobile Web Safari: Adds auto scroll back - Issue 5894 #6413

sidferreira · 2021-11-23T02:12:30Z

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

Platform	Recording
Mobile Web Android	https://user-images.githubusercontent.com/143615/142961774-34921c18-5e3b-425c-86e5-acabe6149b39.mov
iPhone 13 Pro Max	https://user-images.githubusercontent.com/143615/143508193-65e44413-091d-4d78-bb0e-1560d02af98c.mp4
iPhone 13 Pro	https://user-images.githubusercontent.com/143615/143508219-05aadb38-871c-4372-a568-95b11ff3cef3.mp4
iPhone 13	https://user-images.githubusercontent.com/143615/143508250-18e90f02-d99c-4f01-9d78-4962a748c7c6.mp4
iPhone 13 Mini	https://user-images.githubusercontent.com/143615/143508264-06d64d01-13e7-4e81-9b95-1f9a3bf05430.mp4
iPhone 12 iOS 15 Pro Max	https://user-images.githubusercontent.com/143615/143508374-049947b1-764f-4bdb-a975-58f3b314248a.mp4
iPhone 12 iOS 15 Pro	https://user-images.githubusercontent.com/143615/143508389-5d4e143e-ae8c-409f-93b5-9793d6b72676.mp4
iPhone 12 iOS 15	https://user-images.githubusercontent.com/143615/143508409-82d2867e-9b3c-4f4d-b7cd-1d03fce893d9.mp4
iPhone 12 iOS 15 Mini	https://user-images.githubusercontent.com/143615/143508439-ba5de795-bcb1-4a71-8d5c-bf8d5229fae2.mp4
iPhone 12 iOS 14 Pro Max	https://user-images.githubusercontent.com/143615/143508282-fbdac104-b54f-4f84-96ee-0b917554850d.mp4
iPhone 12 iOS 14 Pro	https://user-images.githubusercontent.com/143615/143508311-44536d40-e622-465c-a79b-96dc456b1fc0.mp4
iPhone 12 iOS 14	https://user-images.githubusercontent.com/143615/143508346-0fb34ff8-4e91-4853-8115-15e4630cd47e.mp4
iPhone 12 iOS 14 Mini	https://user-images.githubusercontent.com/143615/143508366-b5586bf6-5248-4ca0-9e61-0ed5b5501a93.mp4
iPhone 11 iOS 14 Pro Max	https://user-images.githubusercontent.com/143615/143508512-fb38ec91-f8c1-4bdf-be7b-39035116d6f3.mp4
iPhone 11 iOS 14 Pro	https://user-images.githubusercontent.com/143615/143508530-b2b898fb-121f-45c4-9701-616db05a9a74.mp4
iPhone 11 iOS 14	https://user-images.githubusercontent.com/143615/143508551-567f6903-c540-4f65-8e0d-817c8591b307.mp4

sidferreira · 2021-11-23T02:19:14Z

@tgolen I'm not sure if import './libs/autoScrollback'; at the app.js is the best choice, so I'm open to suggestions.

roryabraham

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 what autoScrollback does and why it doesn't have a non-web implementation.
Maybe we should rename autoScrollback to mobileSafariAutoScrollback
Wherever you're using someString.indexOf('someOtherString') > 0), we can instead use Str.contains(somString, 'someOtherString') from expensify-common, to improve readability and consistency.
You've also got some lint errors.

src/libs/autoScrollback/index.web.js

sidferreira · 2021-11-25T03:50:33Z

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.

@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.

parasharrajat

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.

sidferreira · 2021-11-25T17:28:35Z

@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.

parasharrajat · 2021-11-25T17:32:20Z

But you are using javascript scrollTo https://developer.mozilla.org/en-US/docs/Web/API/Window/scrollTo#browser_compatibility

About the implementation, I did test each of the devices there to be sure it was working as expected.

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.

sidferreira · 2021-11-25T17:34:11Z

@parasharrajat I'll revisit it, but I did try the smooth option on it and had no success. For sure I'll be glad to remove the dep!

sidferreira · 2021-11-25T23:53:33Z

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

parasharrajat

I am yet to test it on a device..

src/libs/autoScrollback/index.js

src/libs/autoScrollback/index.web.js

parasharrajat

The code looks good.

Edit:
Not tested on the device yet.

sidferreira · 2021-11-29T19:26:40Z

@roryabraham

roryabraham

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?

src/libs/autoScrollback/index.web.js

…yfill back

sidferreira · 2021-11-30T01:18:06Z

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?

roryabraham · 2021-11-30T02:26:36Z

LGTM, I think we can merge this code in our repo, then do a follow-up to:

Open an issue in react-native-web
Submit a PR to fix it.
If/when that PR is merged and released with a new version of react-native-web, then we should update react-native-web and remove this code.

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.

OSBotify · 2021-11-30T02:27:37Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

sidferreira · 2021-11-30T03:06:00Z

@roryabraham cc

kidroca · 2021-11-30T17:18:51Z

src/App.js

+initializeiOSSafariAutoScrollback();
+


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

@kidroca it is merged already... but that's a GREAT suggestion!

Great suggestion @kidroca! If either of you wouldn't mind submitting a quick PR, I'll review it.

Thanks guys!
@roryabraham I'll open a PR about this today, when I get the chance

kidroca · 2021-12-01T17:43:37Z

src/libs/iOSSafariAutoScrollback/index.web.js

+
+
+export default function () {
+    if (!getBrowser() === CONST.BROWSER.SAFARI || !Str.contains(userAgent, 'iphone os 1')) {


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

WOW. It should be getBrowser() == CONST.BROWSER.SAFARI

So we want to exist early if it regular safari, and only apply the polyfill for mobile ?

This is an early return condition
I though it tried to exist early if the browser is not Safari or mobile safari

IT should only work on Safari web.

OMG 🤦‍♂️ how I did that?!

@kidroca if possible, load that file only for Website + Safari

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

kidroca · 2021-12-01T19:05:34Z

Hey, guys I've checked out main in order to apply the suggested update, but it seems there are cases that aren't covered

1 For some reason the text field is covered by the keyboard (only happened once)

rec1.mov

2 Keyboard is partially covering input
3 Depending on where you start to drag you can hide the input and not get it back (you can notice 2 scroll bars at some point)

rec2.mov

This is without applying any modifications to the code and after npm install

parasharrajat · 2021-12-01T19:11:04Z

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.
@sidferreira Could you please look into it?

sidferreira · 2021-12-01T19:13:56Z

@parasharrajat I tested both in iOS14 and 15, will take a look!

sidferreira · 2021-12-02T19:20:43Z

@kidroca just in case, do you mind sharing the exact commit you were testing?

kidroca · 2021-12-03T00:16:02Z

Yes, it was: 0360fd7

sidferreira · 2021-12-03T03:25:44Z

@kidroca @parasharrajat #6572 I couldn't reproduce it, still addressed discussed issues.

parasharrajat · 2021-12-03T06:25:33Z

I will try to reproduce.

OSBotify · 2021-12-07T00:22:08Z

🚀 Deployed to staging by @roryabraham in version: 1.1.17-8 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2021-12-08T20:13:57Z

🚀 Deployed to production by @roryabraham in version: 1.1.18-3 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

sidferreira requested a review from a team as a code owner November 23, 2021 02:12

MelvinBot requested review from roryabraham and removed request for a team November 23, 2021 02:12

Adds autoScrollBack behavior to Mobile Web Safari

e04c6a7

sidferreira force-pushed the sidferreira/issue_5894 branch from abff303 to e04c6a7 Compare November 23, 2021 02:16

sidferreira mentioned this pull request Nov 23, 2021

[$250] mWeb (Safari) - Dragging the screen up creates empty space between keyboard and composer - Reported by: @parasharrajat #5894

Closed

sidferreira changed the title ~~Mobile Web Safari: Adds auto scroll back~~ Mobile Web Safari: Adds auto scroll back - Issue 5894 Nov 23, 2021

roryabraham assigned sidferreira Nov 23, 2021

roryabraham requested changes Nov 23, 2021

View reviewed changes

Mobile Web Safari - Keyboard Visible Overscroll - review

038536a

sidferreira requested a review from roryabraham November 25, 2021 07:16

parasharrajat suggested changes Nov 25, 2021

View reviewed changes

sidferreira added 2 commits November 25, 2021 15:37

Mobile Web Safari - Keyboard Visible Overscroll - review 2

df1f19a

Mobile Web Safari - Keyboard Visible Overscroll - type

92af417

sidferreira requested a review from parasharrajat November 25, 2021 23:53

parasharrajat suggested changes Nov 26, 2021

View reviewed changes

Mobile Web Safari - Keyboard Visible Overscroll - review 3

4c38171

sidferreira requested a review from parasharrajat November 28, 2021 04:11

parasharrajat reviewed Nov 28, 2021

View reviewed changes

roryabraham requested changes Nov 30, 2021

View reviewed changes

src/libs/autoScrollback/index.web.js Outdated Show resolved Hide resolved

src/libs/autoScrollback/index.web.js Outdated Show resolved Hide resolved

src/libs/autoScrollback/index.web.js Outdated Show resolved Hide resolved

sidferreira added 2 commits November 29, 2021 16:20

Mobile Web Safari - Keyboard Visible Overscroll - review 3

3534799

Mobile Web Safari - Keyboard Visible Overscroll - review 3 - Adds pol…

ec6cd6a

…yfill back

sidferreira requested a review from roryabraham November 30, 2021 01:15

roryabraham approved these changes Nov 30, 2021

View reviewed changes

roryabraham merged commit f70f5da into Expensify:main Nov 30, 2021

sidferreira mentioned this pull request Nov 30, 2021

Scroll back Mobile Safari when overscrolled necolas/react-native-web#2170

Open

kidroca reviewed Nov 30, 2021

View reviewed changes

kidroca reviewed Dec 1, 2021

View reviewed changes

OSBotify mentioned this pull request Dec 6, 2021

Deploy Checklist: New Expensify 2021-12-06 #6605

Closed

95 tasks

parasharrajat mentioned this pull request Dec 7, 2021

mWeb - Text input box hidden by keypad when typing message #6621

Closed

roryabraham mentioned this pull request Mar 15, 2022

Main chat composer not visible on mobile web #8135

Closed

roryabraham mentioned this pull request Apr 8, 2022

Revert iOS Safari auto scrollback #8571

Merged

1 task



		export default function () {
		if (!getBrowser() === CONST.BROWSER.SAFARI \|\| !Str.contains(userAgent, 'iphone os 1')) {

Mobile Web Safari: Adds auto scroll back - Issue 5894 #6413

Mobile Web Safari: Adds auto scroll back - Issue 5894 #6413

Conversation

sidferreira commented Nov 23, 2021 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Recordings

sidferreira commented Nov 23, 2021

roryabraham left a comment

Choose a reason for hiding this comment

sidferreira commented Nov 25, 2021 • edited Loading

parasharrajat left a comment

Choose a reason for hiding this comment

sidferreira commented Nov 25, 2021

parasharrajat commented Nov 25, 2021 • edited Loading

sidferreira commented Nov 25, 2021

sidferreira commented Nov 25, 2021

parasharrajat left a comment

Choose a reason for hiding this comment

parasharrajat left a comment • edited Loading

Choose a reason for hiding this comment

sidferreira commented Nov 29, 2021

roryabraham left a comment • edited Loading

Choose a reason for hiding this comment

sidferreira commented Nov 30, 2021

roryabraham commented Nov 30, 2021

OSBotify commented Nov 30, 2021

sidferreira commented Nov 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parasharrajat Dec 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kidroca Dec 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sidferreira Dec 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kidroca commented Dec 1, 2021

parasharrajat commented Dec 1, 2021 • edited Loading

sidferreira commented Dec 1, 2021

sidferreira commented Dec 2, 2021

kidroca commented Dec 3, 2021

sidferreira commented Dec 3, 2021

parasharrajat commented Dec 3, 2021

OSBotify commented Dec 7, 2021

OSBotify commented Dec 8, 2021

sidferreira commented Nov 23, 2021 •

edited

Loading

sidferreira commented Nov 25, 2021 •

edited

Loading

parasharrajat commented Nov 25, 2021 •

edited

Loading

parasharrajat left a comment •

edited

Loading

roryabraham left a comment •

edited

Loading

parasharrajat Dec 1, 2021 •

edited

Loading

kidroca Dec 1, 2021 •

edited

Loading

sidferreira Dec 1, 2021 •

edited

Loading

parasharrajat commented Dec 1, 2021 •

edited

Loading