Fix OTP autofill in iOS Safari #21482

ginsuma · 2023-06-24T10:26:46Z

Details

Fixed Issues

$ #20642
PROPOSAL: #20642 (comment)

Tests

Go to new.expensify.com in Safari on iOS.
Enter your phone number.
Click on the code that appears in the keyboard autofill.
Notice that in the first digit placeholder, all 6 digits are visible but in other slots the correct digits are visible.

Verify that no errors appear in the JS console

Offline tests

N/A

QA Steps

Same as tests

Verify that no errors appear in the JS console

PR Author Checklist

I linked the correct issue in the ### Fixed Issues section above
I wrote clear testing steps that cover the changes made in this PR
- I added steps for local testing in the Tests section
- I added steps for the expected offline behavior in the Offline steps section
- I added steps for Staging and/or Production testing in the QA steps section
- I added steps to cover 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 tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
I included screenshots or videos for tests on all platforms
I ran the tests on all platforms & verified they passed on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
I followed proper code patterns (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 the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- 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
  - If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
- 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 grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is 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 followed the guidelines as stated in the Review Guidelines
I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
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
I verified that if a function's arguments changed that all usages have also been updated correctly
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 code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
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.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

mac_safari.mp4

Mobile Web - Safari

result.mp4

Mobile Web - Chrome

Screen.Recording.2023-06-28.at.4.55.55.PM.mov

Desktop

Screen.Recording.2023-06-28.at.5.09.32.PM.mov

iOS

my.build.mp4

Android

Screen.Recording.2023-06-28.at.5.04.04.PM.mov

github-actions · 2023-06-24T10:27:01Z

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

melvin-bot · 2023-06-24T10:27:17Z

@grgia Please 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]

ginsuma · 2023-06-24T10:34:32Z

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

ginsuma · 2023-06-24T10:34:42Z

recheck

grgia · 2023-06-26T11:24:09Z

@neil-marcellini look's like this one's your issue. Let me know if you need me to review and I'll reassign!

neil-marcellini

I'll wait to review again until after @thesahindia approves, as per our new process, but here are some initial thoughts.

You say that we can't override the background color. Why not? The user agent stylesheet is simply the default style from the browser, as I understand it, so we should be able to set the backgroundColor here rather than using the transitionDelay hack.

Also, how can we test this in the dev environment?

ginsuma · 2023-06-26T17:28:07Z

@neil-marcellini you can test it by use your iPhone and receive sms from any number with format: Your code: 123456

ginsuma · 2023-06-26T17:34:12Z

I'll wait to review again until after @thesahindia approves, as per our new process, but here are some initial thoughts.

You say that we can't override the background color. Why not? The user agent stylesheet is simply the default style from the browser, as I understand it, so we should be able to set the backgroundColor here rather than using the transitionDelay hack.

Also, how can we test this in the dev environment?

I tried to override it but

https://developer.mozilla.org/en-US/docs/Web/CSS/:autofill

ginsuma · 2023-06-27T14:54:27Z

Quick bump for @thesahindia review.

thesahindia · 2023-06-28T08:09:21Z

You say that we can't override the background color. Why not? The user agent stylesheet is simply the default style from the browser, as I understand it, so we should be able to set the backgroundColor here rather than using the transitionDelay hack

It seems like we can't change the backgroundColor but we have one another option. We can use opacity 0 instead of using color: 'transparent' but there will be a regression that the caret (text input cursor) in the magic code input will not appear.

Also, how can we test this in the dev environment?

on mWeb if you are using the same wifi on your physical ios device and mac then you can open safari and navigate to http://<Mac-IP-Address>:<Port-Number>

I couldn't build the native app so will have to ask about in the channel I guess.

@ginsuma, did you try building the app on physical device? Also you will need to attach the screenshots of all platforms you can just show the magic input page in other platforms so we know that the changes aren't breaking anything.

ginsuma · 2023-06-28T08:19:50Z

It seems like we can't change the backgroundColor but we have one another option. We can use opacity 0 instead of using color: 'transparent' but there will be a regression that the caret (text input cursor) in the magic code input will not appear.

App/src/components/MagicCodeInput.js

Lines 299 to 302 in 09a7432

    
           // We need to check the browser because, in iOS Safari, an input in a container with its opacity set to 
        
           // 0 (completely transparent) cannot handle user interaction, hence the Paste option is never shown. 
        
           // Alternate styling will be applied based on this condition. 
        
           const isMobileSafari = Browser.isMobileSafari();

We cannot use opacity: 0 because the Paste option will not show. It happens on iOS/Chrome too.
As I know, we don't expect to show the caret on magic code input.

@ginsuma, did you try building the app on physical device? Also you will need to attach the screenshots of all platforms you can just show the magic input page in other platforms so we know that the changes aren't breaking anything.

I can't build on real iOS devices.I think we need permission to allow our apple developer account.

ginsuma · 2023-06-28T10:12:37Z

Attached the videos of all platforms.

ginsuma · 2023-06-28T10:14:16Z

@thesahindia I see on iOS/Chrome cannot show Paste option too because of opacity: 0, should I fix it too?
Not regression because of this PR.

neil-marcellini · 2023-06-28T16:23:48Z

Great explanation for why we can't override the background color @ginsuma, thank you. Now I'm happy with the background transition work around. Please explain that in a comment and link to the documentation.

on mWeb if you are using the same wifi on your physical ios device and mac then you can open safari and navigate to http://<Mac-IP-Address>:<Port-Number>

Nice thanks! 👍

thesahindia · 2023-06-28T20:20:58Z

@thesahindia I see on iOS/Chrome cannot show Paste option too because of opacity: 0, should I fix it too?
Not regression because of this PR.

I not sure about it. I think I have used the paste option in chrome. I will try checking it on my physical device.

I can't build on real iOS devices.I think we need permission to allow our apple developer account.

hmm, we should test it on physical device. Can you please post about it on the channel.

ginsuma · 2023-06-29T18:39:32Z

@thesahindia I asked on Slack but no one can help or tried auto fill magic code from SMS on iOS app.
But the magicCodeInputTransparent style is only used one time and applied on iOS Safari web browser, I think there won't be any regression with iOS app.

App/src/components/MagicCodeInput.js

Line 302 in 6170bec

const isMobileSafari = Browser.isMobileSafari();

App/src/components/MagicCodeInput.js

Line 348 in 6170bec

inputStyle={[isMobileSafari ? styles.magicCodeInputTransparent : undefined]}

ginsuma · 2023-06-30T06:05:41Z

@thesahindia I have successfully built app on my iPhone. Before and after my commit, the app does not auto fill code from SMS. It's same for Android. I think we don't support it yet.

iOS Native Video

ios.native.mov

ginsuma · 2023-07-03T05:15:37Z

Quick bump for reviewing @thesahindia .

Nikhil-Vats · 2023-07-04T05:01:37Z

Hey @ginsuma, on prod app, it does work for me on iOS.

trim.A66577B0-57C4-4AB1-A145-39DE2DAFE7E2.MOV

ginsuma · 2023-07-04T05:22:07Z

@Nikhil-Vats Thank you for your update. Can you tell me where you download prod app?

Nikhil-Vats · 2023-07-04T05:22:43Z

From the App Store.

ginsuma · 2023-07-04T05:25:31Z

@Nikhil-Vats I tried to search it but only the old version https://apps.apple.com/us/app/expensify-receipts-expenses/id471713959. Can you please give me the new version link?

Nikhil-Vats · 2023-07-04T05:27:06Z

https://apps.apple.com/in/app/new-expensify/id1530278510

ginsuma · 2023-07-04T05:41:14Z

Cannot get auto fill from prod app on my iPhone.

prod.app.mp4

But my commits only applied for iOS Safari as I said above #21482 (comment).
cc: @neil-marcellini @thesahindia .

ginsuma · 2023-07-04T05:58:56Z

I changed the phone number and tried again with my build, the autofill works as normal.

my.build.mp4

cc: @neil-marcellini @thesahindia

thesahindia · 2023-07-04T06:13:25Z

Cool thanks for testing again. It works on my device as well! I will complete the review today.

thesahindia · 2023-07-04T08:22:17Z

@ginsuma, can you help with the instructions on building ios app on my physical device. I didn't find your post on slack.

ginsuma · 2023-07-04T08:25:36Z

@ginsuma, can you help with the instructions on building ios app on my physical device. I didn't find your post on slack.

Do you have an Apple developer account? In ios folder running pod install , then open NewExpensify.xcworkspace, plug in your iPhone and build by Xcode.

thesahindia · 2023-07-04T11:27:44Z

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 the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- 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 grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is 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 code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
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.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

Mobile Web - Chrome

Mobile Web - Safari

trim.9C3FB5E3-C66B-4229-B9FC-CB10DEDE2528.MOV

Desktop

iOS

trim.96FD63F3-B53E-4941-8CC8-DB7CAF17D2D4.MOV

Android

thesahindia · 2023-07-04T11:36:13Z

src/styles/styles.js

@@ -2490,6 +2490,12 @@ const styles = {

    magicCodeInputTransparent: {
        color: 'transparent',
+        caretColor: 'transparent',
+        WebkitTextFillColor: 'transparent',
+        // The input text color get background-color after set it to transparent.


Let's use this comment -

"After setting the input text color to transparent, it acquires the background-color. However, it is not possible to override the background-color directly as explained in this resource: https://developer.mozilla.org/en-US/docs/Web/CSS/:autofill. Therefore, the transition effect needs to be delayed."

I updated it. Thank you.

thesahindia

It tested well, just suggested a comment change. I have a doctor's appointment today so approving the PR.

cc: @neil-marcellini

ginsuma · 2023-07-05T07:42:50Z

Friendly bump @neil-marcellini .

neil-marcellini

🚀

OSBotify · 2023-07-05T22:05:33Z

✋ 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-07-07T10:45:45Z

🚀 Deployed to staging by https://github.com/neil-marcellini in version: 1.3.38-0 🚀

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

OSBotify · 2023-07-11T00:00:47Z

🚀 Deployed to production by https://github.com/francoisl in version: 1.3.38-7 🚀

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

mollfpr · 2023-10-20T16:51:13Z

src/styles/styles.js

+        // After setting the input text color to transparent, it acquires the background-color.
+        // However, it is not possible to override the background-color directly as explained in this resource: https://developer.mozilla.org/en-US/docs/Web/CSS/:autofill
+        // Therefore, the transition effect needs to be delayed.
+        transitionDelay: '99999s',


✋ Coming from #28340

We're calculating the height of the input based on onLayout function, but when we navigate to the new page, the layout.height will return 0. Ref: facebook/react-native#10743

After come back to 2FA page, the layout.height is 52px, but we're using transitionDelay:99999s for 2FA input here, that makes the input height is still 0

Fix OTP autofill in iOS Safari

943eda0

ginsuma requested a review from a team as a code owner June 24, 2023 10:26

melvin-bot bot requested review from grgia and removed request for a team June 24, 2023 10:27

ginsuma mentioned this pull request Jun 24, 2023

[HOLD for payment 2023-07-17] [$1000] OS Safari - All 6 digits visible in first digit slot when using OTP autofill #20642

Closed

6 tasks

grgia requested review from thesahindia and neil-marcellini and removed request for grgia June 26, 2023 11:23

neil-marcellini requested changes Jun 26, 2023

View reviewed changes

Update comment

d171f53

ginsuma requested a review from neil-marcellini June 28, 2023 19:26

thesahindia reviewed Jul 4, 2023

View reviewed changes

thesahindia approved these changes Jul 4, 2023

View reviewed changes

Update comment

9a5bb27

neil-marcellini approved these changes Jul 5, 2023

View reviewed changes

neil-marcellini merged commit db63631 into Expensify:main Jul 5, 2023

OSBotify mentioned this pull request Jul 7, 2023

Deploy Checklist: New Expensify 2023-07-07 #22399

Closed

64 tasks

tienifr mentioned this pull request Sep 28, 2023

[HOLD for payment 2023-10-21] [$500] Focus freeze on add contact method magic code on coming back from search #28340

Closed

6 tasks

mollfpr reviewed Oct 20, 2023

View reviewed changes

Fix OTP autofill in iOS Safari #21482

Fix OTP autofill in iOS Safari #21482

Conversation

ginsuma commented Jun 24, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

github-actions bot commented Jun 24, 2023 • edited Loading

melvin-bot bot commented Jun 24, 2023

ginsuma commented Jun 24, 2023

ginsuma commented Jun 24, 2023

grgia commented Jun 26, 2023

neil-marcellini left a comment

Choose a reason for hiding this comment

ginsuma commented Jun 26, 2023

ginsuma commented Jun 26, 2023

ginsuma commented Jun 27, 2023 • edited Loading

thesahindia commented Jun 28, 2023

ginsuma commented Jun 28, 2023 • edited Loading

ginsuma commented Jun 28, 2023 • edited Loading

ginsuma commented Jun 28, 2023 • edited Loading

neil-marcellini commented Jun 28, 2023

thesahindia commented Jun 28, 2023

ginsuma commented Jun 29, 2023 • edited Loading

ginsuma commented Jun 30, 2023

ginsuma commented Jul 3, 2023

Nikhil-Vats commented Jul 4, 2023

ginsuma commented Jul 4, 2023

Nikhil-Vats commented Jul 4, 2023

ginsuma commented Jul 4, 2023

Nikhil-Vats commented Jul 4, 2023

ginsuma commented Jul 4, 2023

ginsuma commented Jul 4, 2023

thesahindia commented Jul 4, 2023

thesahindia commented Jul 4, 2023

ginsuma commented Jul 4, 2023

thesahindia commented Jul 4, 2023 • edited Loading

Reviewer Checklist

Screenshots/Videos

thesahindia Jul 4, 2023

Choose a reason for hiding this comment

ginsuma Jul 4, 2023

Choose a reason for hiding this comment

thesahindia left a comment

Choose a reason for hiding this comment

ginsuma commented Jul 5, 2023

neil-marcellini left a comment

Choose a reason for hiding this comment

OSBotify commented Jul 5, 2023

OSBotify commented Jul 7, 2023

OSBotify commented Jul 11, 2023

mollfpr Oct 20, 2023

Choose a reason for hiding this comment

ginsuma commented Jun 24, 2023 •

edited

Loading

github-actions bot commented Jun 24, 2023 •

edited

Loading

ginsuma commented Jun 27, 2023 •

edited

Loading

ginsuma commented Jun 28, 2023 •

edited

Loading

ginsuma commented Jun 28, 2023 •

edited

Loading

ginsuma commented Jun 28, 2023 •

edited

Loading

ginsuma commented Jun 29, 2023 •

edited

Loading

thesahindia commented Jul 4, 2023 •

edited

Loading