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

Fix handler regression #392

Merged
merged 3 commits into from
Oct 3, 2023
+435 −80
Merged

Fix handler regression #392

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5f95120
Fix self-closing modals regression
GioSensation Oct 3, 2023
738e918
Fix hotel check-ins dates false matches as cc exp
GioSensation Oct 3, 2023
e6becc3
Improve comment
GioSensation Oct 3, 2023
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
34 changes: 20 additions & 14 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.

34 changes: 20 additions & 14 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.

1 change: 1 addition & 0 deletions integration-test/helpers/mocks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export const constants = {
'loginWithPoorForm': 'pages/login-poor-form.html',
'loginWithText': 'pages/login-with-text.html',
'loginWithFormInModal': 'pages/login-in-modal.html',
'signupWithFormInModal': 'pages/signup-in-modal.html',
'loginCovered': 'pages/login-covered.html',
'loginMultistep': 'pages/login-multistep.html'
},
Expand Down
6 changes: 3 additions & 3 deletions integration-test/helpers/pages.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@
const [, generatedPassword] = passwordButtonText.split('\n')

if (!generatedPassword.trim()) {
throw new Error('unreachable - password must not be empty')

Check failure on line 173 in integration-test/helpers/pages.js

View workflow job for this annotation

GitHub Actions / test

[macos] › email-autofill.macos.spec.js:196:9 › macos › auto filling a signup form › with an identity + Email Protection 

  1) [macos] › email-autofill.macos.spec.js:196:9 › macos › auto filling a signup form › with an identity + Email Protection, autofill using duck address in identity 
    Error: unreachable - password must not be empty

       at ../helpers/pages.js:173

      171 |
      172 |             if (!generatedPassword.trim()) {
    > 173 |                 throw new Error('unreachable - password must not be empty')
          |                       ^
      174 |             }
      175 |
      176 |             await passwordBtn.click({ force: true })

        at SignupPage.selectGeneratedPassword (/home/runner/work/duckduckgo-autofill/duckduckgo-autofill/integration-test/helpers/pages.js:173:23)
        at /home/runner/work/duckduckgo-autofill/duckduckgo-autofill/integration-test/tests/email-autofill.macos.spec.js:210:13
}

await passwordBtn.click({ force: true })
Expand Down Expand Up @@ -579,16 +579,16 @@
expect(mockCalls).toHaveLength(0)
}
async openDialog () {
const button = await page.waitForSelector(`button:has-text("Click here to Login")`)
const button = await page.waitForSelector(`button:has-text("Click here to open dialog")`)
await button.click({ force: true })
await this.assertDialogOpen()
}
async assertDialogClose () {
const form = await page.locator('#login')
const form = await page.locator('.dialog')
await expect(form).toBeHidden()
}
async assertDialogOpen () {
const form = await page.locator('#login')
const form = await page.locator('.dialog')
await expect(form).toBeVisible()
}
async hitEscapeKey () {
Expand Down
4 changes: 2 additions & 2 deletions integration-test/pages/login-in-modal.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
<body>
<p><a href="../index.html">[Home]</a></p>

<p id="demo"><button type="button" id="open-modal">Click here to login</button></p>
<p id="demo"><button type="button" id="open-modal">Click here to open dialog</button></p>

<p id="random-text">Some random text to use as "something outside the dialog element". Clicking here should close the dialog (if open).</p>

Expand All @@ -37,7 +37,7 @@ <h2>Log in</h2>
}, {once: true})
window.addEventListener('pointerdown', (e) => {
if (!dialogEl.contains(e.target)) {dialogEl.setAttribute('hidden', '')}
}, {once: true})
})
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 was the problem with the test and why it didn't catch the regression. Since the self-closing listener only fired once, the test was passing because this handler ran as we clicked into the field and not when clicking again in the autofill pulldown UI.

})

const form = document.forms.login;
Expand Down
53 changes: 53 additions & 0 deletions integration-test/pages/signup-in-modal.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
<!DOCTYPE html>
<html>

<head>
<meta charset="utf-8">
<meta name="viewport" content="width=device-width">
<title>Signup form within a modal</title>
<link rel="stylesheet" href="./style.css" />
</head>

<body>
<p><a href="../index.html">[Home]</a></p>

<p id="demo"><button type="button" id="open-modal">Click here to open dialog</button></p>

<p id="random-text">Some random text to use as "something outside the dialog element". Clicking here should close the dialog (if open).</p>

<div class="dialog" hidden>
<form action="/signup" id="signup">
<h2>Signup</h2>
<fieldset>
<label for="email">Email</label>
<input id="email" type="email">
<label for="password">Password</label>
<input id="password" type="password">
<button type="submit">Signup</button>
</fieldset>
</form>
</div>
<script type="module">
const openModalBtn = document.getElementById('open-modal')
const dialogEl = document.querySelector('.dialog')
openModalBtn.addEventListener('click', () => {
dialogEl.removeAttribute('hidden')
window.addEventListener('keydown', (e) => {
if (e.key === 'Escape') {dialogEl.setAttribute('hidden', '')}
}, {once: true})
window.addEventListener('pointerdown', (e) => {
if (!dialogEl.contains(e.target)) {dialogEl.setAttribute('hidden', '')}
})
})

const form = document.forms.login;
form.addEventListener("submit", (e) => {
e.preventDefault();
if (form.checkValidity()) {
setTimeout(() => dialogEl.innerHTML = '<h1>Submitted!</h1>', 100)
}
})
</script>
</body>

</html>
30 changes: 29 additions & 1 deletion integration-test/tests/email-autofill.extension.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import {forwardConsoleMessages, withEmailProtectionExtensionSignedInAs} from '../helpers/harness.js'
import { test as base, expect } from '@playwright/test'
import {constants} from '../helpers/mocks.js'
import {emailAutofillPage} from '../helpers/pages.js'
import {emailAutofillPage, loginPage} from '../helpers/pages.js'
import { stripDuckExtension } from '../helpers/utils.js'
import {testContext} from '../helpers/test-context.js'

Expand Down Expand Up @@ -66,4 +66,32 @@ test.describe('chrome extension', () => {
'autofill_show', 'autofill_private_address' // second private autofill
])
})

test('should not close the modal when autofilling', async ({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.

Existing tests should have caught the regression already, but I'm adding this one specific to the extensions because when we remove all the Catalina stuff the bug won't actually happen anymore in the desktop apps, so this gives us more confidence going forward.

Side note, it will be quite an undertaking to rewrite all these integration tests when we remove the in-page Catalina pulldown 😰. Almost all macOS integration tests rely on the Catalina version of the pulldown.

const {personalAddress} = constants.fields.email

forwardConsoleMessages(page)
await withEmailProtectionExtensionSignedInAs(page, stripDuckExtension(personalAddress))
// page abstraction
const emailPage = loginPage(page)
await emailPage.navigate('signupWithFormInModal')

await emailPage.openDialog()

await emailPage.clickIntoUsernameInput()

// buttons, unique to the extension
const personalAddressBtn = await page.locator(`text=Use ${personalAddress} Block email trackers`)

// click first option
await personalAddressBtn.click()

// ensure autofill populates the field
await emailPage.assertUsernameFilled(personalAddress)

await emailPage.assertDialogOpen()

await emailPage.clickOutsideTheDialog()
await emailPage.assertDialogClose()
})
})
6 changes: 3 additions & 3 deletions src/Form/matching-config/__generated__/compiled-matching-config.js
View file
Open in desktop

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

6 changes: 3 additions & 3 deletions src/Form/matching-config/matching-config-source.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,12 +293,12 @@ const matchingConfiguration = {
cardSecurityCode: {match: 'security.?code|card.?verif|cvv|csc|cvc|cv2|card id'},
expirationMonth: {
match: '(card|\\bcc\\b)?.?(exp(iry|iration)?)?.?(month|\\bmm\\b(?![.\\s/-]yy))',
skip: 'mm[/\\s.\\-_—–]'
skip: 'mm[/\\s.\\-_—–]|check'
},
expirationYear: {match: '(card|\\bcc\\b)?.?(exp(iry|iration)?)?.?(year|yy)', skip: 'mm[/\\s.\\-_—–]'},
expirationYear: {match: '(card|\\bcc\\b)?.?(exp(iry|iration)?)?.?(year|yy)', skip: 'mm[/\\s.\\-_—–]|check'},
expiration: {
match: '(\\bmm\\b|\\b\\d\\d\\b)[/\\s.\\-_—–](\\byy|\\bjj|\\baa|\\b\\d\\d)|\\bexp|\\bvalid(idity| through| until)',
skip: 'invalid|^dd/'
skip: 'invalid|^dd/|check'
},

// Identities
Expand Down
Loading
Loading