-
Notifications
You must be signed in to change notification settings - Fork 11
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
Fix handler regression #392
Conversation
Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>
Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>
}, {once: true}) | ||
}) |
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.
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.
@@ -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}) => { |
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.
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.
} else { | ||
// Pointerdown is needed here to avoid self-closing modals disappearing because this even happens in the page | ||
window.addEventListener('pointerdown', this, true) | ||
} |
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.
And this is the fix itself. Basically, we keep the pointerup
event for native apps pulldown (when it's rendered in the top frame), but use pointerdown
when we render in the page.
I think I could have used click
instead of pointerup
but didn't want to mess up the existing behaviour or spend time testing. The upside would just be a more usual event type, so 🤷♂️.
Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>
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.
This is great - thanks so much for going to the effort to reproduce in the tests 🙏🏻
Task/Issue URL: https://app.asana.com/0/1205691418983700/1205691418983700 Autofill Release: https://github.com/duckduckgo/duckduckgo-autofill/releases/tag/8.4.2 ## Description Updates Autofill to version [8.4.2](https://github.com/duckduckgo/duckduckgo-autofill/releases/tag/8.4.2). ### Autofill 8.4.2 release notes ## What's Changed * Ema/perf followups by @GioSensation in duckduckgo/duckduckgo-autofill#386 * Fix handler regression by @GioSensation in duckduckgo/duckduckgo-autofill#392 * Bump markdown-it from 13.0.1 to 13.0.2 by @dependabot in duckduckgo/duckduckgo-autofill#388 * Revert mutation observer for forms by @GioSensation in duckduckgo/duckduckgo-autofill#396 **Full Changelog**: duckduckgo/duckduckgo-autofill@8.4.1...8.4.2 ## Steps to test This release has been tested during autofill development. For smoke test steps see [this task](https://app.asana.com/0/1198964220583541/1200583647142330/f). Co-authored-by: GioSensation <GioSensation@users.noreply.github.com>
Task/Issue URL: https://app.asana.com/0/1205691418983706/1205691418983706 Autofill Release: https://github.com/duckduckgo/duckduckgo-autofill/releases/tag/8.4.2 BSK PR: duckduckgo/BrowserServicesKit#530 ## Description Updates Autofill to version [8.4.2](https://github.com/duckduckgo/duckduckgo-autofill/releases/tag/8.4.2). ### Autofill 8.4.2 release notes ## What's Changed * Ema/perf followups by @GioSensation in duckduckgo/duckduckgo-autofill#386 * Fix handler regression by @GioSensation in duckduckgo/duckduckgo-autofill#392 * Bump markdown-it from 13.0.1 to 13.0.2 by @dependabot in duckduckgo/duckduckgo-autofill#388 * Revert mutation observer for forms by @GioSensation in duckduckgo/duckduckgo-autofill#396 **Full Changelog**: duckduckgo/duckduckgo-autofill@8.4.1...8.4.2 ## Steps to test This release has been tested during autofill development. For smoke test steps see [this task](https://app.asana.com/0/1198964220583541/1200583647142330/f). --------- Co-authored-by: GioSensation <GioSensation@users.noreply.github.com> Co-authored-by: Graeme Arthur <2030310+graeme@users.noreply.github.com>
Task/Issue URL: https://app.asana.com/0/1205691418983700/1205691418983700 Autofill Release: https://github.com/duckduckgo/duckduckgo-autofill/releases/tag/8.4.2 ## Description Updates Autofill to version [8.4.2](https://github.com/duckduckgo/duckduckgo-autofill/releases/tag/8.4.2). ### Autofill 8.4.2 release notes ## What's Changed * Ema/perf followups by @GioSensation in duckduckgo/duckduckgo-autofill#386 * Fix handler regression by @GioSensation in duckduckgo/duckduckgo-autofill#392 * Bump markdown-it from 13.0.1 to 13.0.2 by @dependabot in duckduckgo/duckduckgo-autofill#388 * Revert mutation observer for forms by @GioSensation in duckduckgo/duckduckgo-autofill#396 **Full Changelog**: duckduckgo/duckduckgo-autofill@8.4.1...8.4.2 ## Steps to test This release has been tested during autofill development. For smoke test steps see [this task](https://app.asana.com/0/1198964220583541/1200583647142330/f). Co-authored-by: GioSensation <GioSensation@users.noreply.github.com>
Reviewer: @shakyShane
Asana: https://app.asana.com/0/1200930669568058/1205581668483223/f
Description
We recently introduced a regression on extensions where we were no longer protecting against self-closing modals, meaning trying to autofill would also close the modal in certain cases. This fixes the regression and the tests that didn't catch it 🙃. I've also slipped in a minor accuracy update.
Steps to test
Tests now actually work. To see this in action, you can try this in https://privacy-test-pages.site/autofill/autoprompt/2-form-in-modal.html using the extension. The production extension will close the modal, but this version won't.