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

Revert mutation observer for forms #396

Merged
merged 2 commits into from
Oct 10, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 7 additions & 35 deletions dist/autofill-debug.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

42 changes: 7 additions & 35 deletions dist/autofill.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion integration-test/tests/mutating-form.macos.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {test as base} from '@playwright/test'
*/
const test = base.extend({})

test.describe('Mutating form page', () => {
test.describe.skip('Mutating form page', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Just switching the test off for now.

async function applyScript (page) {
await createAutofillScript()
.replaceAll(macosContentScopeReplacements())
Expand Down
34 changes: 0 additions & 34 deletions src/Form/Form.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import {constants} from '../constants.js'
const {
ATTR_AUTOFILL,
ATTR_INPUT_TYPE,
MAX_FORM_MUT_OBS_COUNT,
MAX_INPUTS_PER_FORM
} = constants

Expand Down Expand Up @@ -77,30 +76,6 @@ class Form {
}
})

this.mutObsCount = 0
this.mutObsConfig = { childList: true, subtree: true }
this.mutObs = new MutationObserver(
(records) => {
const anythingRemoved = records.some(record => record.removedNodes.length > 0)
if (anythingRemoved) {
// Must check for inputs because a parent may be removed and not show up in record.removedNodes
if ([...this.inputs.all].some(input => !input.isConnected)) {
// If any known input has been removed from the DOM, reanalyze the whole form
window.requestIdleCallback(() => {
this.formAnalyzer = new FormAnalyzer(this.form, input, this.matching)
this.recategorizeAllInputs()
})

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

Copy link
Member Author

Choose a reason for hiding this comment

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

This wasn't directly related to the breakage, but I'm dropping it now before investigating things further in the next few days.

// This ensures we fire the handler again if the form is changed
this.addListener(form, 'input', () => {
if (!this.isAutofilling) {
Expand All @@ -110,7 +85,6 @@ class Form {
})

this.categorizeInputs()
this.mutObs.observe(this.form, this.mutObsConfig)

this.logFormInfo()

Expand Down Expand Up @@ -356,7 +330,6 @@ class Form {
this.removeAllDecorations()
this.removeTooltip()
this.forgetAllInputs()
this.mutObs.disconnect()
this.matching.clear()
this.intObs = null
}
Expand Down Expand Up @@ -444,13 +417,6 @@ class Form {
return this
}

// When new inputs are added after the initial scan, reanalyze the whole form
if (this.initialScanComplete) {
this.formAnalyzer = new FormAnalyzer(this.form, input, this.matching)
this.recategorizeAllInputs()
return this
}
Copy link
Member Author

@GioSensation GioSensation Oct 10, 2023

Choose a reason for hiding this comment

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

This is what was causing the problem. Basically, in certain edge cases the container of the field (what here we call this.form) could become the body. When that happened, any new field was hitting this conditional thus dropping everything and starting from scratch. We were doing this for every single added field, which in Asana could be hundreds at a time as you scroll through large projects. This wasn't always hitting the failsafes because we kept destroying and re-building the form data.

The failsafe we built for the form-specific mutation observer wasn't running here because the trigger comes from outside (the scanner), not from the internal observer.

In our browser the slowdown was limited (still massive), but on Firefox it could hang for several seconds and show the warning.


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

Expand Down
9 changes: 7 additions & 2 deletions src/Scanner.js
Original file line number Diff line number Diff line change
Expand Up @@ -226,8 +226,13 @@ class DefaultScanner {
const parentForm = this.getParentForm(input)

if (parentForm instanceof HTMLFormElement && this.forms.has(parentForm)) {
// We've met the form, add the input
this.forms.get(parentForm)?.addInput(input)
const foundForm = this.forms.get(parentForm)
// We've met the form, add the input provided it's below the max input limit
if (foundForm && foundForm.inputs.all.size < MAX_INPUTS_PER_FORM) {
foundForm.addInput(input)
} else {
this.stopScanner('The form has too many inputs, destroying.')
}
Copy link
Member Author

Choose a reason for hiding this comment

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

I've moved this check in the scanner as well for added security. I have a few follow up tasks in Asana to try and understand this flow better and see how we can make it more resilient to breakage.

return
}

Expand Down
42 changes: 7 additions & 35 deletions swift-package/Resources/assets/autofill-debug.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading