Ema/fix mutating forms #358

GioSensation · 2023-07-12T09:51:12Z

Reviewer: @shakyShane
Asana: https://app.asana.com/0/0/1205015898566987/f

Description

Observe form mutations and re-scan the form when input fields are removed. This won't catch all possible forms of mutations, but it's a good first step with reduced overhead.

Steps to test

Test case added. You can test this live on these sites:

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>

… ema/fix-mutating-forms

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>

GioSensation · 2023-07-12T09:52:36Z

src/Form/Form.js

@@ -45,9 +43,6 @@ class Form {
 this.form = form
 this.matching = matching || createMatching()
 this.formAnalyzer = new FormAnalyzer(form, input, matching)
- this.isLogin = this.formAnalyzer.isLogin
- this.isSignup = this.formAnalyzer.isSignup
- this.isHybrid = this.formAnalyzer.isHybrid


These now are all getters, so if we change the formAnalyzer results, the changes are reflected here.

GioSensation · 2023-07-12T09:53:19Z

src/Form/Form.js

+ this.mutObs = new MutationObserver(
+ (records) => {
+ const anythingRemoved = records.some(record => record.removedNodes.length > 0)
+ if (anythingRemoved) {


By running only when elements are removed we reduce the overhead.

GioSensation · 2023-07-12T09:55:46Z

src/Form/FormAnalyzer.js

@@ -9,6 +9,8 @@ const signupRegex = new RegExp(
 )
 const conservativeSignupRegex = new RegExp(/sign.?up|join|register|enroll|newsletter|subscri(be|ption)|settings|preferences|profile|update/i)
 const strictSignupRegex = new RegExp(/sign.?up|join|register|(create|new).+account|enroll|settings|preferences|profile|update/i)
+const resetPasswordLink = new RegExp(/(forgot(ten)?|reset|don't remember) (your )?password|password forgotten/i)
+const loginProvidersRegex = new RegExp(/ with /i)


Changes here are unrelated to the mutating forms. Just improving accuracy and splitting these two regexes so we can use them independently.

… ema/fix-mutating-forms Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com> # Conflicts: # dist/autofill-debug.js # dist/autofill.js # swift-package/Resources/assets/autofill-debug.js # swift-package/Resources/assets/autofill.js

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>

GioSensation · 2023-07-21T10:15:15Z

src/Form/Form.js

+ // If the form mutates too much, disconnect to avoid performance issues
+ if (this.mutObsCount >= MAX_FORM_MUT_OBS_COUNT) {
+ this.mutObs.disconnect()
+ }


Added this bit to disconnect the observer when form mutations force too many re-scans. This prevents infinite loops and other performance issues.

GioSensation · 2023-07-21T10:16:30Z

src/Form/Form.js

@@ -323,8 +362,14 @@ class Form {
 if (this.form.matches(selector)) {
 this.addInput(this.form)
 } else {
- this.form.querySelectorAll(selector).forEach(input => this.addInput(input))
+ const foundInputs = this.form.querySelectorAll(selector)


This is a further performance safeguard. If the form contains too many inputs, we stop right away during the initial scan.

GioSensation · 2023-07-21T10:17:51Z

src/Form/Form.js

+ if (this.inputs.all.has(input)) return this
+
+ // If the form has too many inputs, destroy everything to avoid performance issues
+ if (this.inputs.all.size > MAX_INPUTS_PER_FORM) {


And here we destroy() the form, removing all listeners and icons, if the form grows over a certain threshold.

GioSensation · 2023-07-21T10:18:37Z

src/Form/Form.js

 // Nothing to do with 1-character fields
 if (input.maxLength === 1) return this

- if (this.inputs.all.has(input)) return this


Moved to the top to avoid unnecessary work.

GioSensation · 2023-07-21T10:19:22Z

src/Scanner.js

- maxInputsOnPage: 100
+ maxInputsPerPage: MAX_INPUTS_PER_PAGE,
+ maxFormsPerPage: MAX_FORMS_PER_PAGE,
+ maxInputsPerForm: MAX_INPUTS_PER_FORM


Centralizing these defaults in the constants.js file.

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>

GioSensation · 2023-07-21T11:19:28Z

src/UI/HTMLTooltip.js

- ? `transform: translate(${left}px, ${top}px) rotate(180deg); transform-origin: 16px !important;`
- : `transform: translate(${left}px, ${top}px) !important;`
+ ? `transform: translate(${Math.floor(left)}px, ${Math.floor(top)}px) rotate(180deg); transform-origin: 18px !important;`
+ : `transform: translate(${Math.floor(left)}px, ${Math.floor(top)}px) !important;`


All this stuff and other CSS changes are of course unrelated. Just squeezing in a 2px reduction in icon size requested by the design folks. The floor is just to avoid weird artefacts in edge cases.

shakyShane

Looks great - I think we just need to be conservative with logging, and then this is good to go :)

shakyShane · 2023-07-21T12:33:49Z

src/Form/Form.js

+ if (foundInputs.length < MAX_INPUTS_PER_FORM) {
+ foundInputs.forEach(input => this.addInput(input))
+ } else {
+ console.log('The form has too many inputs, bailing.')


needs wrapping in a debug check

shakyShane · 2023-07-21T12:34:18Z

src/Form/Form.js

+
+ // If the form has too many inputs, destroy everything to avoid performance issues
+ if (this.inputs.all.size > MAX_INPUTS_PER_FORM) {
+ console.log('The form has too many inputs, destroying.')


needs wrapping in a debug check?

shakyShane · 2023-07-21T12:36:11Z

src/Scanner.js

+ if (this.forms.size < this.options.maxFormsPerPage) {
+ this.forms.set(parentForm, new Form(parentForm, input, this.device, this.matching, this.shouldAutoprompt))
+ } else {
+ console.log('The page has too many forms, stop adding them.')


wrap in debug check?

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>

GioSensation added 4 commits July 12, 2023 10:21

Minor accuracy improvements

ca3755f

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>

Use getters for formAnalyzer results

d3aba2d

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>

Merge branch 'main' of github.com:duckduckgo/duckduckgo-autofill into…

8f12612

… ema/fix-mutating-forms

Add mutation observer to forms

2ff4879

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>

GioSensation self-assigned this Jul 12, 2023

GioSensation commented Jul 12, 2023

View reviewed changes

GioSensation marked this pull request as ready for review July 12, 2023 09:56

GioSensation requested a review from shakyShane July 12, 2023 10:01

GioSensation added 2 commits July 21, 2023 11:17

Add performance safeguards

55f0910

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>

GioSensation commented Jul 21, 2023

View reviewed changes

Fix icon sizing

12399f7

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>

GioSensation commented Jul 21, 2023

View reviewed changes

shakyShane requested changes Jul 21, 2023

View reviewed changes

Guard logs with shouldLog

e4c0181

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>

GioSensation requested a review from shakyShane July 21, 2023 12:45

shakyShane approved these changes Jul 21, 2023

View reviewed changes

GioSensation merged commit 802438a into main Jul 21, 2023
1 check passed

GioSensation deleted the ema/fix-mutating-forms branch July 21, 2023 12:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ema/fix mutating forms #358

Ema/fix mutating forms #358

GioSensation commented Jul 12, 2023 •

edited

Loading

GioSensation Jul 12, 2023

GioSensation Jul 12, 2023

GioSensation Jul 12, 2023

GioSensation Jul 21, 2023

shakyShane Jul 21, 2023

GioSensation Jul 21, 2023 •

edited

Loading

GioSensation Jul 21, 2023

shakyShane Jul 21, 2023

GioSensation Jul 21, 2023

GioSensation Jul 21, 2023

GioSensation Jul 21, 2023

shakyShane left a comment

shakyShane Jul 21, 2023

shakyShane Jul 21, 2023

shakyShane Jul 21, 2023

Ema/fix mutating forms #358

Ema/fix mutating forms #358

Conversation

GioSensation commented Jul 12, 2023 • edited Loading

Description

Steps to test

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GioSensation Jul 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shakyShane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GioSensation commented Jul 12, 2023 •

edited

Loading

GioSensation Jul 21, 2023 •

edited

Loading