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.

Sign up for GitHub

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

[Form] Don't reset value, if the target input is non-ddg or unknown #748

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[Form] Don't reset value, if the target input is non-ddg or unknown #748

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
336906b
fix: don't reset value, if the target input is non-ddg
dbajpeyi Jan 29, 2025
148a641
Merge branch 'main' into dbajpeyi/fix/form-save-via-input
dbajpeyi Jan 31, 2025
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
10 changes: 8 additions & 2 deletions dist/autofill-debug.js
View file
Open in desktop

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

10 changes: 8 additions & 2 deletions dist/autofill.js
View file
Open in desktop

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

47 changes: 47 additions & 0 deletions integration-test/helpers/pages/addressPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
import { expect } from '@playwright/test';
import { mockedCalls } from '../harness.js';

/**
* A wrapper around interactions for `integration-test/pages/address-form.html`
*
* @param {import("@playwright/test").Page} page
*/
export function addressPage(page) {
class AddressPage {
/**
* @return {Promise<void>}
*/
async navigate() {
await page.goto('integration-test/pages/address-form.html');
}

/**
* Fill in all address form fields
*/
async fillAddressForm(data) {
await page.fill('#firstName', data.firstName);
await page.fill('#lastName', data.lastName);
await page.fill('#address1', data.addressStreet);
await page.fill('#address2', data.addressStreet2);
await page.fill('#city', data.addressCity);
await page.fill('#state', data.addressProvince);
await page.fill('#postalCode', data.addressPostalCode);
await page.fill('#country', data.addressCountryCode);
await page.fill('#phone', data.phone);
await page.fill('#notes', data.notes);
}

async submitFormViaTextbox() {
const textbox = await page.$('#notes');
await textbox?.focus();
await page.keyboard.down('Enter');
}

async shouldNotPromptToSave() {
const mockCalls = await mockedCalls(page, { names: ['storeFormData'], minCount: 0 });
expect(mockCalls.length).toBe(0);
}
}

return new AddressPage();
}
79 changes: 79 additions & 0 deletions integration-test/pages/address-form.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="UTF-8">
<meta name="viewport" content="width=device-width, initial-scale=1.0">
<title>Address Form Test</title>
<link rel="stylesheet" href="style.css">
</head>
<body>
<div class="container">
<form id="addressForm" method="POST" action="#">
<h2>Test address form, with an arbitrary text field</h2>
<p>This form is used to test the address form, with an arbitrary text field</p>

<!-- Name Fields -->
<div class="form-group">
<label for="firstName">First Name</label>
<input type="text" id="firstName" name="firstName" autocomplete="given-name" required>
</div>

<div class="form-group">
<label for="lastName">Last Name</label>
<input type="text" id="lastName" name="lastName" autocomplete="name" required>
</div>

<!-- Address Fields -->
<div class="form-group">
<label for="address1">Address Line 1</label>
<input type="text" id="address1" name="address1" autocomplete="address-line1" required>
</div>

<div class="form-group">
<label for="address2">Address Line 2</label>
<input type="text" id="address2" name="address2" autocomplete="address-line2">
</div>

<div class="form-group">
<label for="city">City</label>
<input type="text" id="city" name="city" autocomplete="address-level2" required>
</div>

<div class="form-group">
<label for="state">State/Province</label>
<input type="text" id="state" name="state" autocomplete="address-level1" required>
</div>

<div class="form-group">
<label for="postalCode">Postal Code</label>
<input type="text" id="postalCode" name="postalCode" autocomplete="postal-code" required>
</div>

<div class="form-group">
<label for="country">Country</label>
<input type="text" id="country" name="country" autocomplete="country" required>
</div>

<div class="form-group">
<label for="phone">Phone Number</label>
<input type="tel" id="phone" name="phone" autocomplete="tel">
</div>

<!-- Additional Random Text Field -->
<div class="form-group">
<label for="notes">Random Notes</label>
<textarea id="notes" name="notes"></textarea>
</div>

<button type="submit">Save Address</button>
</form>
</div>
<script type="module">
[...document.forms].forEach((form) => {
form.addEventListener("submit", (e) => {
e.preventDefault();
})
})
</script>
</body>
</html>
1 change: 1 addition & 0 deletions integration-test/pages/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
</head>
<body>
<ul>
<li><a href="address-form.html">address-form.html</a></li>
<li><a href="email-at-bottom.html">email-at-bottom.html</a></li>
<li><a href="email-at-top-left.html">email-at-top-left.html</a></li>
<li><a href="email-autofill.html">email-autofill.html</a></li>
Expand Down
29 changes: 28 additions & 1 deletion integration-test/tests/save-prompts.macos.spec.js
View file
Open in desktop
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think playwright is a very good candidate here. It's a very special user interaction that can be replicated with it, and we don't seem to have a similar form.

Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { createWebkitMocks, macosContentScopeReplacements } from '../helpers/moc
import { test as base } from '@playwright/test';
import { signupPage } from '../helpers/pages/signupPage.js';
import { loginPage } from '../helpers/pages/loginPage.js';

import { addressPage } from '../helpers/pages/addressPage.js';
/**
* Tests for various auto-fill scenarios on macos
*/
Expand Down Expand Up @@ -79,5 +79,32 @@ test.describe('macos', () => {
await login.shouldPromptToSave();
});
});
test.describe('prompting to save an address form with additional inputs', () => {
const addressData = {
firstName: 'John',
lastName: 'Doe',
addressStreet: '123 Main St',
addressStreet2: 'Apt 4B',
addressCity: 'New York',
addressCountryCode: 'US',
addressProvince: 'NY',
addressPostalCode: '10001',
phone: '5550123',
notes: 'Leave at door',
};
test('submitting address form with non-ddg inputs should not prompt', async ({ page }) => {
// enable in-terminal exceptions
await forwardConsoleMessages(page);

await createWebkitMocks().applyTo(page);
await defaultMacosScript(page);

const address = addressPage(page);
await address.navigate();
await address.fillAddressForm(addressData);
await address.submitFormViaTextbox();
await address.shouldNotPromptToSave();
});
});
});
});
10 changes: 8 additions & 2 deletions src/Form/Form.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,10 +368,16 @@ class Form {

initFormListeners() {
// This ensures we fire the handler again if the form is changed
this.addListener(this.form, 'input', () => {
this.addListener(this.form, 'input', (e) => {
if (!this.isAutofilling) {
this.submitHandlerExecuted = false;
this.resetShouldPromptToStoreData();
/** @type {import('./matching.js').SupportedTypes} **/
const inputType = e.target.getAttribute(ATTR_INPUT_TYPE);
if (inputType && inputType !== 'unknown') {
this.resetShouldPromptToStoreData();
} else {
this.shouldPromptToStoreData = false;
}
}
});

Expand Down
10 changes: 8 additions & 2 deletions swift-package/Resources/assets/autofill-debug.js
View file
Open in desktop

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

10 changes: 8 additions & 2 deletions swift-package/Resources/assets/autofill.js
View file
Open in desktop

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

Loading