Fix scrolling to pickers #16433

cubuspl42 · 2023-03-22T23:57:50Z

Details

Based on this PR to our react-native-picker-select fork

Fixed scrolling to input components on Web
Added conditional ref-forwarding logic in components/Picker

Fixed Issues

$ #14783
PROPOSAL: #14783 (comment)

Tests

Go to Settings > Workspaces > (any workspace) > Reimburse expenses > Connect bank account > Connect manually
Perform "Connect bank account (Step 1 of 5) anyhow (for example, fill: 011103093 / 067014822)
In the next step, Input correct values for all inputs before "State" picker, but leave "State" empty ("-")
Click/tap some text input nearby
Scroll the view, so the "State" field is out of sight (it should be possible on all platforms)
Press "Save"
Press "Fix the errors"

Verify that other inputs lost focus
- Ensure the keyboard is hidden
- Ensure that text inputs don't have blinking cursor
Verify that the view scrolled to the picker
- Ensure that, after scrolling, the picker ends up close to the top of the screen, consistently with other inputs

Offline tests

QA Steps

See Tests

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

web.mp4

Mobile Web - Chrome

(working as expected)

Mobile Web - Safari

(working as expected)

Desktop

(working as expected)

iOS

ios.mp4

Android

android.mp4

MelvinBot · 2023-03-22T23:57:53Z

Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers!

github-actions · 2023-03-22T23:58:06Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

cubuspl42 · 2023-03-23T00:36:57Z

I have read the CLA Document and I hereby sign the CLA

cubuspl42 · 2023-03-23T00:40:17Z

@parasharrajat I'm still going over the checklist, I'll continue tomorrow.

I tried to check relevant documents, conventions, and apply common sense.

For now, I'm wondering if we should include another form in the testing steps, as the one mentioned in the original issue ("Add debit card") doesn't really allow for scrolling on a typical screen on Android.

This form would work...:

...but I still haven't found the path to manually trigger this flow.

cubuspl42 · 2023-03-23T08:33:20Z

QA Steps

See Tests

@parasharrajat Does it make sense? I think that both sections would look the same here (?).

cubuspl42 · 2023-03-23T08:34:38Z

On iOS the scroll view scrolled to the picker, creating extra "stretched" void on the bottom of the screen

@parasharrajat This is an interesting property of iOS scroll view. I personally don't have much against this effect, but we need to ensure that it is acceptable behavior.

Conclusion: Not a regression, let's leave this an an unrelated thing

parasharrajat · 2023-03-23T09:39:29Z

This iOS scroll looks weird to me as I am not an iPhone user. Is it present on main?

cubuspl42 · 2023-03-23T09:47:32Z

This iOS scroll looks weird to me as I am not an iPhone user. Is it present on main?

Yes, present on main, with scrolling to any text input. Not present when scrolling to pickers (as on main it obviously doesn't work) and when scrolling to checkboxes (as it throws an exception).

No happens for text inputs and pickers, so not really a regression.

cubuspl42 · 2023-03-23T09:59:02Z

@parasharrajat Do you have thoughts on the focusRef naming? Pointing the ref to any internal widgets breaks the test suite, like I mentioned. Not obvious for me what are the work-arounds. If we'd do any pivoting, it's best to make that decision early, I think!

Also it's my first time sketching all the "Testing" sections, so any tips if I'm doing it right would be very appreciated!

cubuspl42 · 2023-03-23T10:19:25Z

@parasharrajat

I ran the tests on all platforms & verified they passed on

Is this about the automated tests or the manual steps I describe? I'm a bit confused. Are automatic tests platform-dependent?

cubuspl42 · 2023-03-23T11:30:13Z

@parasharrajat This is an example, when the user leaves out "Legal business name" and "State" fields in the "Company information" form on Web.

They press "Fix the errors", the form scrolls to "Legal business name", they fix the error, press "Fix the errors" again, then seemingly the form scrolls to "Legal business name" again.

scrolling-incosistency-web-2.mp4

It actually scrolls to "State", but focusing at the center (inconsistent with other inputs).

It's because with the current approach, we don't ensure that the focus/measureLayout contract is fulfilled, at least not on the Web. Our in-app scrolling logic is not launched, and instead we rely on the side-effect of focus that happens on Web, which has different behavior than the in-app logic.

I would still propose to do things consistently, also on the Web, unless we have any technical arguments against it:

scrolling-consistent-1.mp4

This includes providing the necessary accessibility for the input on the Web.

~~What would you say?~~

~~(While this is maybe worth discussing, this thing has much higher priority)~~

(Conslusion: with forwarding measureLayout calls cross-platform, this doesn't happen)

src/components/Picker/Picker.js

cubuspl42 · 2023-03-23T17:41:38Z

@parasharrajat Force pushed to add GPG signing (impossible otherwise)

parasharrajat · 2023-03-23T18:29:25Z

Thanks. I didn't udnderstnad #16433 (comment) . What are you proposing?

Can you explain in terms code?

parasharrajat · 2023-03-23T18:30:41Z

Please Mark this PR Ready for review.

cubuspl42 · 2023-03-23T18:33:58Z

Thanks. I didn't udnderstnad #16433 (comment) . What are you proposing?

Can you explain in terms code?

I haven't proposed anything yet. Before we go to fixing, can you see the problem/inconsistency I'm talking about?

This is the artifact of the chosen approach (I wasn't aware about it before, but it came out during manual testing). My original proposal didn't have this problem/inconsistency.

A solution I'd propose would, as I originally suggested, forward the measureLayout to the root View.

It wouldn't instantly mean that we'd have re-introduce the "blur all inputs before focusing the new one" block. We could go hybrid. This is an example commit (on another branch, just for the sake of discussion) introducing that change. Many details are possible to adjust, but the essence is to make components/Picker expose measureLayout like the Form expects. On Web, if we forward the whole ref to react-native-picker/picker, we're lacking exposed measureLayout method.

MelvinBot · 2023-03-23T18:40:39Z

@johnmlee101 @parasharrajat One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

cubuspl42 · 2023-03-23T18:42:28Z

Please Mark this PR Ready for review.

Done.

~~But some checkboxes aren't done yet, I'll update them very soon.~~ Basically everything is done

Also, while the PR might be ready for review (and was ready for early review before), ~~I wouldn't say it's ready to merge until we figure out if the scrolling on Web works as expected~~ (while that's maybe worth discussing, it's nothing compared to this)

cubuspl42 · 2023-03-24T11:39:47Z

@parasharrajat
@johnmlee101

Hey guys, while I'm sorry that this came out this late (I spent too much time on the technicalities, including GPG and alike, it's on my side), but I'm starting to question whether the "forward ref to RNPicker's TextInput" works at all. While scrolling to the picker actually does work (at least on mobile, proving that TextInput's measureLayout works as expected), the main "selling point" of that approach was bullet-proof blur/focus behavior.

After I resolved all technical discussions and started to do all the heavy testing, I can't see any blur/focus behavior with this approach. I'm very uncomfortable with this, as it would mean that the original author never really tested it or that I can't get this to work because of some silly mistake on my side.

text-input-focus-issue.mp4

It's difficult for me, as I'm working with not-fully-my proposal, but I have to take responsibility for it (as I agreed to that, and I know that).

Still, I ask you for a big favor of checking out the code, and verifying (both by reading the code and hands-on) if I implemented the "forward-to-internal-TextInput" right and if you reproduce my results.

If you do, it would be awesome if you agreed to do some emergency Slack chat session, or even video meeting to figure out the next steps.

(Conclusion: With Keyboard.dismiss() this doesn't happen)

parasharrajat · 2023-03-24T15:07:37Z

I think it is fine, we agreed to not add any UI for the focused state. #14783 (comment)

cubuspl42 · 2023-03-24T15:36:06Z

I think it is fine, we agreed to not add any UI for the focused state. #14783 (comment)

What is fine? Could you elaborate?

Have you seen the video in this post?

The focus call on the "State" picker is a total no-op, it doesn't remove the cursor from the "Zip code" input, it doesn't hide the keyboard, it doesn't do anything.

I'm not talking about styling.

parasharrajat · 2023-03-24T15:37:51Z

Oops. So yeah, you mentioned in your proposal to blur the inputs before focusing. Is that not working?

parasharrajat · 2023-03-29T17:24:40Z

Last change #16433 (comment)

...regarding the Mobile Safari glitch.

cubuspl42 · 2023-03-29T17:41:36Z

@parasharrajat 👍

parasharrajat · 2023-03-29T18:08:53Z

Screenshots

🔲 iOS / native

screen-2023-03-28_18.32.31.mp4

🔲 iOS / Safari

🔲 MacOS / Desktop

🔲 MacOS / Chrome

screen-2023-03-28_04.28.15.mp4

🔲 Android / Chrome

screen-2023-03-28_04.31.13.mp4

🔲 Android / native

screen-2023-03-28_04.27.33.mp4

parasharrajat

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

cc: @johnmlee101

🎀 👀 🎀 C+ reviewed

parasharrajat · 2023-03-29T18:12:53Z

FYI @johnmlee101, I noticed a few inconsistencies #16433 (comment). Those are either native behavior or out of the scope of this PR.

parasharrajat · 2023-03-31T22:26:31Z

Bump @johnmlee101

johnmlee101 · 2023-04-03T15:39:18Z

Lint failures

cubuspl42 · 2023-04-03T16:14:37Z

Lint failures

@johnmlee101 Ouch. Sorry about that. Fixed.

Does the PR look good otherwise?

johnmlee101

I think everything looks good from my perspective. Good comments, and this should be a net positive with RBR

cubuspl42 · 2023-04-03T16:37:56Z

@johnmlee101 Accidentally pushed an unsigned commit, now fixed. It's because of a GUI client I use. I'll set up a pre-push hook on my side to not make that happen again.

johnmlee101 · 2023-04-03T18:24:38Z

@parasharrajat shall I wait for your final approval or am I good to merge?

cubuspl42 · 2023-04-03T18:31:12Z

@johnmlee101 The PR was approved by @parasharrajat before I removed the unused import. So from my perspective it's effectively approved by both of you.

OSBotify · 2023-04-04T16:45:19Z

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

OSBotify · 2023-04-05T03:18:46Z

🚀 Deployed to staging by https://github.com/johnmlee101 in version: 1.2.95-0 🚀

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

OSBotify · 2023-04-06T00:58:45Z

🚀 Deployed to production by https://github.com/thienlnam in version: 1.2.95-0 🚀

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

fedirjh · 2023-07-05T21:02:08Z

src/components/Form.js

+                        const formContentRef = this.formContentRef.current;
+
+                        // Start with dismissing the keyboard, so when we focus a non-text input, the keyboard is hidden
+                        Keyboard.dismiss();


Hey there ,this line caused a regression in #20507, it dismiss Keyboard for all inputs including text. We should have excluded text inputs.

cubuspl42 force-pushed the fix-state-picker-1 branch from 0fa9b74 to 3f9bb67 Compare March 22, 2023 23:59

cubuspl42 commented Mar 23, 2023

View reviewed changes

src/components/Picker/Picker.js Outdated Show resolved Hide resolved

cubuspl42 mentioned this pull request Mar 23, 2023

Support ref property inside textInputProps Expensify/react-native-picker-select#6

Closed

cubuspl42 force-pushed the fix-state-picker-1 branch from 3f9bb67 to 5e8502f Compare March 23, 2023 17:41

cubuspl42 marked this pull request as ready for review March 23, 2023 18:40

cubuspl42 requested a review from a team as a code owner March 23, 2023 18:40

melvin-bot bot requested review from johnmlee101 and parasharrajat and removed request for a team March 23, 2023 18:40

Merge remote-tracking branch 'upstream/main' into fix-state-picker-1

beeab89

BasePicker: Add more details to a comment

a9ea91c

...regarding the Mobile Safari glitch.

parasharrajat previously approved these changes Mar 29, 2023

View reviewed changes

cubuspl42 dismissed parasharrajat’s stale review via 1d7ff2b April 3, 2023 16:15

johnmlee101 previously approved these changes Apr 3, 2023

View reviewed changes

formSubmitPropTypes.js: Remove unused import

530b437

cubuspl42 dismissed johnmlee101’s stale review via 530b437 April 3, 2023 16:36

cubuspl42 force-pushed the fix-state-picker-1 branch from 1d7ff2b to 530b437 Compare April 3, 2023 16:36

0xmiros mentioned this pull request Apr 3, 2023

[HOLD for payment 2023-04-27] [$1000] iOS - Workspace - Save button bounces when selecting currency. #16353

Closed

6 tasks

johnmlee101 approved these changes Apr 3, 2023

View reviewed changes

parasharrajat approved these changes Apr 4, 2023

View reviewed changes

johnmlee101 merged commit 7368dac into Expensify:main Apr 4, 2023

OSBotify mentioned this pull request Apr 5, 2023

Deploy Checklist: New Expensify 2023-04-05 #16956

Closed

15 tasks

MelvinBot mentioned this pull request Apr 6, 2023

[HOLD for payment 2023-04-13] [$4000] Can not focus on state field when clicking "fix the errors" while adding debit card Action Performed #14783

Closed

1 task

Ollyws mentioned this pull request Apr 6, 2023

[HOLD for payment 2023-05-09] [$2000] Connect Bank Account: Screen content is shifted to the top if user click on Fix the error button when the checkbox is unselected. #15483

Closed

1 task

fedirjh reviewed Jul 5, 2023

View reviewed changes

fedirjh mentioned this pull request Jul 5, 2023

[HOLD for payment 2023-07-06] [$1000] Android - Keyboard jumps when clicking on fix the errors after entering incorrect contact method #20507

Closed

6 tasks

Fix scrolling to pickers #16433

Fix scrolling to pickers #16433

Conversation

cubuspl42 commented Mar 22, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

MelvinBot commented Mar 22, 2023

github-actions bot commented Mar 22, 2023 • edited Loading

cubuspl42 commented Mar 23, 2023

cubuspl42 commented Mar 23, 2023

cubuspl42 commented Mar 23, 2023

QA Steps

cubuspl42 commented Mar 23, 2023 • edited Loading

parasharrajat commented Mar 23, 2023

cubuspl42 commented Mar 23, 2023

cubuspl42 commented Mar 23, 2023 • edited Loading

cubuspl42 commented Mar 23, 2023

cubuspl42 commented Mar 23, 2023 • edited Loading

cubuspl42 commented Mar 23, 2023

parasharrajat commented Mar 23, 2023 • edited Loading

parasharrajat commented Mar 23, 2023

cubuspl42 commented Mar 23, 2023 • edited Loading

MelvinBot commented Mar 23, 2023

cubuspl42 commented Mar 23, 2023 • edited Loading

cubuspl42 commented Mar 24, 2023 • edited Loading

parasharrajat commented Mar 24, 2023 • edited Loading

cubuspl42 commented Mar 24, 2023

parasharrajat commented Mar 24, 2023

parasharrajat commented Mar 29, 2023

cubuspl42 commented Mar 29, 2023

parasharrajat commented Mar 29, 2023

Screenshots

🔲 iOS / native

🔲 iOS / Safari

🔲 MacOS / Desktop

🔲 MacOS / Chrome

🔲 Android / Chrome

🔲 Android / native

parasharrajat left a comment • edited Loading

Choose a reason for hiding this comment

Reviewer Checklist

parasharrajat commented Mar 29, 2023

parasharrajat commented Mar 31, 2023

johnmlee101 commented Apr 3, 2023

cubuspl42 commented Apr 3, 2023 • edited Loading

johnmlee101 left a comment

Choose a reason for hiding this comment

cubuspl42 commented Apr 3, 2023

johnmlee101 commented Apr 3, 2023

cubuspl42 commented Apr 3, 2023

OSBotify commented Apr 4, 2023

OSBotify commented Apr 5, 2023

OSBotify commented Apr 6, 2023

fedirjh Jul 5, 2023

Choose a reason for hiding this comment

cubuspl42 commented Mar 22, 2023 •

edited

Loading

github-actions bot commented Mar 22, 2023 •

edited

Loading

cubuspl42 commented Mar 23, 2023 •

edited

Loading

cubuspl42 commented Mar 23, 2023 •

edited

Loading

cubuspl42 commented Mar 23, 2023 •

edited

Loading

parasharrajat commented Mar 23, 2023 •

edited

Loading

cubuspl42 commented Mar 23, 2023 •

edited

Loading

cubuspl42 commented Mar 23, 2023 •

edited

Loading

cubuspl42 commented Mar 24, 2023 •

edited

Loading

parasharrajat commented Mar 24, 2023 •

edited

Loading

parasharrajat left a comment •

edited

Loading

cubuspl42 commented Apr 3, 2023 •

edited

Loading