Skip to content
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

Top autofill #432

Merged
merged 9 commits into from
Feb 25, 2022
Merged

Top autofill #432

merged 9 commits into from
Feb 25, 2022

Conversation

jonathanKingston
Copy link
Collaborator

@jonathanKingston jonathanKingston commented Feb 19, 2022

Task/Issue URL: https://app.asana.com/0/0/1201854459638164/f
Tech Design URL:
CC:

Description:

The code changes the UserScript in the page from rendering the autofill, to message passing into it's own WebView that is isolated from the page. This prevents rendering issues for iframes where we don't have enough space to render.

This following sequence diagram represents the 'happy path' for opening and filling a credential [t] and [c] represent top and child modes respectively.

sequenceDiagram
    autonumber
    autofill.js[c]->>WebsiteAutofillUserScript: showAutofillParent
    WebsiteAutofillUserScript->>BrowserTabViewController: autofillDisplayOverlay
    BrowserTabViewController->>ContentOverlayPopover: autofillDisplayOverlay
    ContentOverlayPopover->>OverlayAutofillUserScript: setup
    OverlayAutofillUserScript->>autofill.js[t]: setup
    autofill.js[t]->>OverlayAutofillUserScript: pmHandlerGetAutofillInitData
    Note right of autofill.js[t]: DOM render
    autofill.js[t]->>OverlayAutofillUserScript: setSize
    Note right of autofill.js[t]: User clicks item
    autofill.js[t]->>OverlayAutofillUserScript: selectedDetail
    OverlayAutofillUserScript->>WebsiteAutofillUserScript: messageSelectedCredential
    loop
        autofill.js[c]->>WebsiteAutofillUserScript: getSelectedCredentials
    end
Loading

Three non ideal parts of this code are:

  1. Positioning calculated using click offsets
    • For iframes the WebView doesn't have context of the x,y of the input
    • So we capture the x,y in both swift and JS and compare to position ContentOverlay relative to the input field.
  2. Polling to collect framed data
    • NSFrameInfo doesn't live outside the delegate lifecycle
    • So instead we have to set properties on the child autofill and poll in the child autofill.js for when the user has selected the details.
  3. Simulating a hover by passing a hover event from ContentOverlay to the autofill.js via a mouseMove custom event.
    • This is required because the input field in the primary WebView needs focus.
    • We also need to allow the user to hover the overlay elements.
    • So we capture mouse move and inject evaluated JS to simulate the normal hover event.

Steps to test this PR:

  1. Open https://stripe.dev/elements-examples/
  2. Click into the credit card field

The autofill should open for the user to fill in credit cards. This field exists within an iframe so is rendered as an overlay WebView.

image

Things to check:

  • Hovering over the credential items should work smoothly
  • Positioning should have the overlay rendered in the correct place

Ensure that auth into email continues to work:

  1. Click email protection -> disable
  2. Autofill should not have the private/public email addresses
  3. Click enable and follow the auth process
  4. Autofill should have the private/public email addresses

BrowserServicesKit dependency: duckduckgo/BrowserServicesKit#67

Testing checklist:

  • Test with Release configuration

Internal references:

Software Engineering Expectations
Technical Design Template
When ready for review, remember to post the PR in MM

@jonathanKingston jonathanKingston force-pushed the jkt/top-autofill-window2 branch 7 times, most recently from 516cfe4 to 18a6ab1 Compare February 21, 2022 18:47
@jonathanKingston jonathanKingston changed the title [draft] Top autofill Top autofill Feb 21, 2022
@bstandaert-ddg
Copy link

It seems from the Asana task that this has already been tested somewhat, but let me know if there's anything in particular you'd like me to test here and I can do that.

@samsymons
Copy link
Collaborator

@jonathanKingston I've still got plenty to go through here but honestly I think you're fine to fire up the review tasks. 🙂 I'll wrap the review up tomorrow.

@jonathanKingston jonathanKingston force-pushed the jkt/top-autofill-window2 branch 4 times, most recently from 3eebad0 to b36be44 Compare February 22, 2022 10:06
@jonathanKingston
Copy link
Collaborator Author

Just removed 6 files from the commit (some fn's that weren't required, cleared off moved files from BrowserTab and deleted the extension file that moved to BSK)

@jonathanKingston jonathanKingston force-pushed the jkt/top-autofill-window2 branch 2 times, most recently from 9f4f556 to 4c2e2e7 Compare February 22, 2022 18:10
@@ -563,6 +564,7 @@ extension Tab: UserContentControllerDelegate {
userScripts.autofillScript.topView = self.delegate
userScripts.autofillScript.emailDelegate = emailManager
userScripts.autofillScript.vaultDelegate = vaultManager
self.autofillScript = userScripts.autofillScript
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Storing this is to allow for wiring of the clickTriggered call, it's non ideal as they could get out of sync but I'd rather not keep refactoring all this.

@jonathanKingston
Copy link
Collaborator Author

It seems from the Asana task that this has already been tested somewhat, but let me know if there's anything in particular you'd like me to test here and I can do that.

Hey sorry @bstandaert-ddg I missed this. I think verifying that we don't break things with these refactors I'm having to do to keep up with the changes landing would be super helpful.

We have a design task: https://app.asana.com/0/0/1201404545287651/f
Test sites: https://app.asana.com/0/0/1201378637302055/f

Happy to hop on a call and walk through some of the manual checking I've been doing. I think we're good but certainly before release we should test it some more.

@bstandaert-ddg
Copy link

Thanks for clarifying. I ran through the set of ecommerce sites from https://app.asana.com/0/72649045549333/1201499187083535/f with a saved credit card, comparing the latest release build with this branch, and I'm seeing two things that are different:

  1. On Walmart's checkout, it sometimes takes two clicks in the CC field for the autofill popup to appear:
Screen.Recording.2022-02-22.at.9.40.03.PM.mov

(existing behavior followed by new)

  1. On Best Buy, the CC autofill popup doesn't show up any more (I do have a card saved in both instances):
Screen.Recording.2022-02-22.at.9.54.05.PM.mov

I don't have a lot of time to schedule a call this week, but if there's any other specific testing steps you want me to follow let me know.

@jonathanKingston
Copy link
Collaborator Author

On Walmart's checkout, it sometimes takes two clicks in the CC field for the autofill popup to appear:

Right so this is a new regression, I think we can fix this but I need to double check. We can fix this purely in the JS layer if it's possible as it's the not capturing the click.

On Best Buy, the CC autofill popup doesn't show up any more (I do have a card saved in both instances):

So the field:

<input autocomplete="cc-number" class="tb-input " id="optimized-cc-card-number" type="tel" maxlength="19" pattern="^(?:4[0-9]{12}(?:[0-9]{3})?|5[1-5][0-9]{14}|3[47][0-9]{13}|3(?:0[0-5]|[68][0-9])[0-9]{11}|6(?:011|5[0-9]{2})[0-9]{12}|(?:2131|1800|35\d{3})\d{11})$" required="" value="" aria-describedby="card-number">

And surrounding markup:

<section class="credit-card-form credit-card-form--none"><h2 class="payment__cc-label">Credit or Debit Card</h2><div class="v-m-vertical-s checkout-input-field-styles v-fw-medium credit-card-form__number-wrap"><span class="sr-only"><label for="optimized-cc-card-number"><span>Credit Card or Debit Card Number</span></label></span><div class="clearFloat has-error"><input autocomplete="cc-number" class="tb-input " id="optimized-cc-card-number" type="tel" maxlength="19" pattern="^(?:4[0-9]{12}(?:[0-9]{3})?|5[1-5][0-9]{14}|3[47][0-9]{13}|3(?:0[0-5]|[68][0-9])[0-9]{11}|6(?:011|5[0-9]{2})[0-9]{12}|(?:2131|1800|35\d{3})\d{11})$" required="" value="" aria-describedby="card-number"><i alt="none" class="credit-card-form__type-image-icon credit-card-form__type-image-icon--none"><span class="sr-only">none</span></i><span class="help-block" id="card-number"><svg aria-hidden="true" role="img" viewBox="0 0 100 100" fill="#bb0628"><svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 32 32" xml:space="preserve"><path d="M16 7.31c.54 0 .98.44.98.98v11.64a.98.98 0 1 1-1.96 0V8.29c0-.54.44-.98.98-.98zm-1.47 16.51a1.45 1.45 0 1 0 2.9 0 1.45 1.45 0 0 0-2.9 0zm17.33 4.32a1 1 0 0 1-.85.5H.98a.98.98 0 0 1-.84-1.48l15.01-26a.98.98 0 0 1 1.7 0l15.01 26c.18.3.18.68 0 .98zm-2.54-1.46L16 3.6 2.68 26.68h26.64z"></path></svg></svg>Please enter a valid card number.</span></div></div><div><div class="credit-card-form__card-spread"><i class="card-logo card-logo--bestbuyconsumer"><span class="sr-only"><span>My Best Buy Credit Card</span>&nbsp;</span></i><i class="card-logo card-logo--bestbuycobrand"><span class="sr-only"><span>My Best Buy Visa</span>&nbsp;</span></i><i class="card-logo card-logo--mastercard"><span class="sr-only"><span>mastercard</span>&nbsp;</span></i><i class="card-logo card-logo--visa"><span class="sr-only"><span>visa</span>&nbsp;</span></i><i class="card-logo card-logo--discover"><span class="sr-only"><span>Discover</span>&nbsp;</span></i><i class="card-logo card-logo--amex"><span class="sr-only"><span>American Express</span>&nbsp;</span></i><i class="card-logo card-logo--unionpay"><span class="sr-only"><span>UnionPay</span>&nbsp;</span></i><i class="card-logo card-logo--jcb"><span class="sr-only"><span>JCB</span>&nbsp;</span></i></div><div class="credit-card-form__bestbuycobrand-promotion"><div><span>Use a <strong>My Best Buy® Credit Card</strong> and get 5% Back¹ in Rewards</span></div></div></div><div></div></section>

Isn't being classified correctly; we've made follow up fixes here but perhaps this is a new regression. I'll raise a bug and shouldn't block this change.

@jonathanKingston jonathanKingston force-pushed the jkt/top-autofill-window2 branch 2 times, most recently from 650de22 to 1a9bd67 Compare February 23, 2022 15:36
@samsymons
Copy link
Collaborator

I'm also seeing leaked user scripts in the memory graph debugger, but haven't confirmed that that isn't an issue with a recent develop merge yet.

Copy link
Collaborator

@bwaresiak bwaresiak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few notes from my recent testing:

  1. There is inconsistency between how form is filled (for separated fields) when I tap on CC number field, expiration date or CVC code - I'd expect all field to be filled/updated when I choose an entry.

  2. Pressing field "imperfectly" does not bring up the suggestion window but can focus the field itself - this left me confused a bit feeling that it sometimes works and sometimes doesn't.

  3. Start the app, navigate to https://stripe.dev/elements-examples/ - script is often calling "close" when just scrolling the webpage.

@jonathanKingston
Copy link
Collaborator Author

jonathanKingston commented Feb 24, 2022

  1. This is actually an issue with the form itself, because of the iframes we rely on them propagating back the way. We could potentially resolve this but it's non trivial as we'd need to pass the whole DOM tree into swift. Happy to discuss but I think we can punt as a follow up whatever the solution.
  2. Sometimes labels are clicked and other DOM, I have a patch I'm testing to improve/resolve this. (Ben actually mentioned the same issue above too in his 2. issue) fixed: Look for closest label on click duckduckgo-autofill#73
  3. Fire less top close duckduckgo-autofill#72 this resolves it, I meant to PR this the other day.

@jonathanKingston jonathanKingston force-pushed the jkt/top-autofill-window2 branch 4 times, most recently from 581067f to 4733d9a Compare February 25, 2022 03:45
Copy link
Collaborator

@bwaresiak bwaresiak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job @jonathanKingston , tested this on Mac and also did check iOS 👍

@jonathanKingston jonathanKingston merged commit 2f3aeaa into develop Feb 25, 2022
@jonathanKingston jonathanKingston deleted the jkt/top-autofill-window2 branch February 25, 2022 18:57
samsymons added a commit that referenced this pull request Feb 25, 2022
# By Sam Macbeth (2) and others
# Via GitHub
* develop:
  Top autofill (#432)
  Option to add new notes or edit existing is disabled (#446)
  Use our own autoconsent fork (#444)
  New Feedback Form (#424)
  Update privacy dashboard (#440)
  Fix crash when background tabs trigger cookie popup (#439)
  Update clickToLoadConfig.json (#435)
  rename weakAssign to assign(to:onWeaklyHeld:) (#442)
  Improve Safari favorite importing (#436)

# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
#	DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
samsymons added a commit that referenced this pull request Mar 2, 2022
# By Tomas Strba (6) and others
# Via GitHub (1) and Tomas Strba (1)
* develop:
  Logins+ authentication (#438)
  Update privacy dashboard to main (#451)
  Version 0.19.1
  Remove mouse event listeners on browser close (#448)
  Find in Page Hotfix + refactoring of forced unwrapping (#450)
  Memory leak fixed (#449)
  Version 0.19.0
  Top autofill (#432)
  Option to add new notes or edit existing is disabled (#446)
  Use our own autoconsent fork (#444)
  New Feedback Form (#424)
  Update privacy dashboard (#440)

# Conflicts:
#	DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
#	DuckDuckGo/SecureVault/View/PasswordManager.storyboard
samsymons added a commit that referenced this pull request Mar 4, 2022
* develop:
  Logins+ authentication (#438)
  Update privacy dashboard to main (#451)
  Version 0.19.1
  Remove mouse event listeners on browser close (#448)
  Find in Page Hotfix + refactoring of forced unwrapping (#450)
  Memory leak fixed (#449)
  Version 0.19.0
  Top autofill (#432)
  Option to add new notes or edit existing is disabled (#446)
  Use our own autoconsent fork (#444)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants