-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[SG-1026 / PM-1125] - Document / Improve Form Detection in Notification Bar #4798
[SG-1026 / PM-1125] - Document / Improve Form Detection in Notification Bar #4798
Conversation
…ies for specific sites as part of research to identify areas for possible improvement + added types where appropriate.
…d-research-autofill-notification-bar
…d-research-autofill-notification-bar
…com), even if the pageDetails contained a valid form to watch, the loadPasswordFields method parameter for fillNewPassword being false for inputs with autoCompleteType of "new-password" would cause the account creation form to not be watched (null form data returned to notification bar). Setting this to true will help capture more account creations in the above specified scenario.
…d change form on talkshoe.com: (1) Update autofill.service getFormsWithPasswordFields(...) method to group autofill.js found password type fields under a single form in a very specific scenario where the most likely case is that it is a password change form with poorly designed mark up in a SPA (2) Notification bar - when listening to a form, we must use both the loginButtonNames and the changePasswordButton names as we don't know what type of form we are listening to (3) Notification bar - on page change, we must empty out the watched forms array to prevent forms w/ the same opId being added to the array on SPA url change (4) Notification bar - getSubmitButton update - If we cannot find a submit button within a form, try going up one level to the parent element and searching again (+ added save to changePasswordButtonNames). (5) Notification bar - when listening to a form with a submit button, we can attach the formOpId to the button so we can only have DOM traversal in one location and retrieve the form off the button later on in the form submission logic. For now, I'm just adding it as a fallback, but it could be the primary approach with more testing.
…ould start observing the DOM immediately so we properly catch rendered forms instead of waiting for a second. This was especially prevelant on refreshing the password change form page on talkshoe.com.
…ollectPageDetailsIfNeeded (now handlePageChange), the mutation observer could get setup late and miss forms loading (ex: refreshing a password change page on talkshoe.com). DOM observation is now setup as fast as possible on page load for SPAs/Non SPAs and on change for SPAs by having the mutation observer itself detect page change and deterministically calling handlePageChange(). However, with these changes, page detail collection still only occurs after a minimum of ~1 second whether or not it was triggered from the mutation observer detecting forms being injected onto the page or the scheduleHandlePageChange running (which has a theoretical maximum time to page detail collection of ~1.999 seconds but this does require the mutation observer to miss the page change in a SPA which shouldn't happen).
…d-research-autofill-notification-bar
…d-research-autofill-notification-bar
…ll service which prevents multi-step account creation forms from being returned to the notification-bar content script from the notification.background.ts script.
…multi-step login form (email then password on https://login.live.com/login.srf) with next button that gets swapped out for a true submit button in order to prompt for saving user credentials if not in Bitwarden. This logic works *sometimes* as the submit button page change often stops the submit button event listeners from being able to fire and send the login to the background script. However, that is a separate issue to be solved, and sometimes is better than never. This type of logic might be useful in solving the multi-step account creation form on https://signup.live.com/signup but that will require additional changes to the autofill service which current intercepts forms without passwords and prevents them from reaching the notification-bar.ts content script.
…d-research-autofill-notification-bar
…' of https://github.com/bitwarden/clients into SG-1026-document-and-research-autofill-notification-bar
Kudos, SonarCloud Quality Gate passed!
|
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.
Couple of comments; the main one being some concerns with the tree walker changes potentially slowing pages down.
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.
Fantastic work on this @JaredSnider-Bitwarden !
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.
@JaredSnider-Bitwarden: Outstanding work 🚀
Breaking sections down, documenting them, renaming things to clarify what is going on and then starting to add some tweaks for a better detection 💪 :bitwarden-love
This is a huge step, to aid the AutoFill team to start working on this.
Thank you so much for looking into this!
QA tested and approved to merge. |
Type of change
Objectives
SG-1026 / PM-1125
notification-bar.ts
to provide additional clarity in preparation for handoff to new autofill team.Code changes
This might be easier to review by commits, but I'll break down the largest changes here as well:
apps/browser/src/autofill/services/autofill.service.ts:
pageDetails
contained a valid form to watch, the loadPasswordFields method parameter forfillNewPassword
being false for inputs with autoCompleteType of "new-password" would cause the account creation form to not be watched (null form data returned to notification bar). Setting this to true will help capture more account creations in the above specified scenario.AutofillService.loadPasswordFields(pageDetails, true, true, false, **true**);
apps/browser/src/autofill/content/notification-bar.ts:
handlePageChange(...)
method on SPA page change vs. just depending on a scheduled call to the same method.loginButtonNames
and thechangePasswordButton
names as we don't know what type of form we are listening to.formSubmitted
logicBefore you submit