-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Add FXIOS-5694 [v111] credit card autofill / capture PoC #12787
Conversation
@issammani is this ready for review as I see WIP but is not a draft? |
334b86b
to
f8c2807
Compare
FYI, build failed due to swiftlint errors described here. |
let response: [String: Any] = [ | ||
"data": [ | ||
"cc-name": "Jane Doe", | ||
"cc-number": "5555555555554444", | ||
"cc-exp-month": "05", | ||
"cc-exp-year": "2028", | ||
], | ||
"id": request["id"]!, | ||
] |
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.
Since this is a POC we should leave this empty and fill it in when we are actually testing
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.
Commented and added a todo in 3c9d580
@@ -1768,6 +1768,9 @@ extension BrowserViewController: TabDelegate { | |||
tab.addContentScript(logins, name: LoginsHelper.name()) | |||
} | |||
|
|||
let creditCardHelper = CreditCardHelper(tab: tab) |
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.
We should wrap this around a feature flag as this is a POC and shouldn't get merged directly to Firefox iOS unless its feature is complete.
Since it might take multiple PRs to reach there I would recommend adding a comment with Jira ticket and commenting out this piece of code.
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.
Commented and added a todo in 3c9d580
I also didn't see the use of |
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 for the POC @issammani Although this is good but let's clean this up so it's mergeable.
Ideally, we should add a component to encapsulate all details into a single area if possible but I understand that it might not be feasible in this PR.
I would also recommend moving the POC over to SwiftFox where you can test your scripts much more freely as compared to Firefox iOS
Will ping you in slack for further discussion.
@nbhasin2 following our discussion on the webpack plugin. This is needed for desktop imports that use uris. For instance, in this file:
|
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.
@issammani Skipped the js files as those will get updated later on but added a few nits to streamline the code
You can either take my suggestions or modify it yourself and after BR is green we are good to push :)
@nbhasin2 thanks for the suggestions and updating the PR. Looks good from my side. BR is failing though. I am tempted to push an empty commit just to have run once again. Do we have a better way to do so ? |
@issammani https://app.bitrise.io/build/54a6820e-666f-413d-997c-0310b39c3ade I started another build, no idea why it failed 🤷🏼♂️ Try if you can run it locally as this passed before. |
Client.app: Coverage: 25.72
Generated by 🚫 Danger Swift against a7476f3 |
The failed test has nothing to do with this PR. Will look into it tomorrow but most likely merge it in as it could be a random failure from BR |
This PR:
✅ Adds two way communication between ios and js.
✅ Adds webpack plugin to resolve custom resource uris.
✅ Extracts necessary logic from desktop to capture credit card fields.
Client/Assets/CC_Script/CreditCardAutofill.js
andClient/Frontend/UserContent/UserScripts/AllFrames/AtDocumentStart/CreditCardHelper.js
contain minimal custom code extracted from desktop to make the poc work. These will eventually be replaced to use shared code from desktop. All other files inAssets/CC_Script
are shared as is from desktop, so eventually we can pull these using the github action.The code will not work with all credit card forms. We need to migrate fathom eventually to iOS to test things properly. But for now I used this [https://mozilla.github.io/form-fill-examples/basic_cc.html] for testing and I think it's enough for a PoC.
Current State of the PoC
@nbhasin2 will eventually add a UI to select credit cards, but for now I just fill a mock credit card in the form.
Screen.Recording.2023-01-17.at.12.15.26.mov