-
Notifications
You must be signed in to change notification settings - Fork 24
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
#8206: Avoid Edge crash when opening user links #8211
Conversation
I wrote up the minimal playwright test scenarios on the ticket: #8206. Let's make sure we don't close out the issue until the automated test scenarios are in |
src/utils/openAllLinksInPopups.ts
Outdated
@@ -33,5 +36,5 @@ const listener = excludeAltClicksEtc((event: MouseEvent) => { | |||
* https://github.com/pixiebrix/pixiebrix-extension/issues/7809 | |||
*/ | |||
export default function openAllLinksInPopups(signal?: AbortSignal) { | |||
document.body.addEventListener("click", listener, { signal }); | |||
document.body.addEventListener("click", listener, { signal, capture: 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.
Comment on why capture is required? So we see it before it gets eaten by the ShadowDOM?
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.
+1
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.
It turns out it's not needed. I was initially clicking the "Read more" link which is not a real link and it's not affected by this bug.
I think that button has its own stopPropagation
so capture
got it.
Anyway, dropped.
Yep this PR can be merged and it doesn't close that issue. The tests are part of Future Work |
To avoid this regression: #8211
Merging so I can cut an alpha to dog food. Can help with the playwright testing today/tomorrow |
…ntent`, `CustomFormComponent` (#8200) * POC: Isolated components * Wrap `SelectionMenu` * Extract CSS, use regular element for now * Wrap `PropertyTree` * Wrap `SelectionToolPopover` * Fix tests * Fix unrelated mutation https://togithub.com/sindresorhus/eslint-plugin-unicorn/issues/1514 https://togithub.com/pixiebrix/pixiebrix-extension/pull/4486 * Update docs * Cleanup * Some components need no style; test Stylesheets component * Ok but no fonts? * Ok with fonts * Import issues * Lint * Move init to <IsolatedComponent> * Fix most issues * Extract/cleanup * /2 * Move DiscardFilePlugin config out * Update documentation * CustomFormComponent * EphemeralFormContent * DocumentView * Bad merge * Lint * Lint * Sort jest config * Ensure stylesheets are removed in IsolatedComponent * Comments * Open Shadow DOM To avoid this regression: #8211 * Test shadow DOM * Fill the frame (h-100) * fixes failing tests --------- Co-authored-by: Graham Langford <grahamlangford87@gmail.com>
What does this PR do?
I was curious as to why it wasn't catching the event and here's what I found:
capture: true
target
is the shadow DOM container even when the shadow DOM is set toopen
The fix was quick enough
Future work
Checklist