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

Fix rare crash in event handler #7525

Merged
merged 4 commits into from
Jan 10, 2024
Merged

Fix rare crash in event handler #7525

merged 4 commits into from
Jan 10, 2024

Conversation

philippotto
Copy link
Member

@philippotto philippotto commented Jan 8, 2024

addEventListenerWithDelegation created an event handler that would crash on non-Element EventTargets. Mostly, event targets are Elements and support matches(), but other event targets, such as document and window, don't. when an event handler is called for these, an error occured. TS rightfully complained about these, too.

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Steps to test:

  • I don't know how the actual bug can be reproduced reliably, but I hope we can trust TS on this one
  • also I tested that the basic interaction with the viewports still works

Issues:


(Please delete unneeded items, merge only when none are left open)

@philippotto philippotto self-assigned this Jan 8, 2024
const wrapperFunc = function (this: HTMLElement | Document, event: Event) {
for (
let { target } = event;
target && target !== this && target instanceof Element;
Copy link
Member

Choose a reason for hiding this comment

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

d.h. wenn target === document ist, dann wird der loop nicht ausgeführt / zum nächsten gesprungen?

Copy link
Member

Choose a reason for hiding this comment

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

In der PR Beschreibung geht es noch um window, dass wird hier nicht berücksichtigt, oder?

Copy link
Member Author

Choose a reason for hiding this comment

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

d.h. wenn target === document ist, dann wird der loop nicht ausgeführt / zum nächsten gesprungen?

wenn target === document und this === document ist, wird die loop abgebrochen. in dem fall wurde dann kein passendes element gefunden, auf dem der event listener triggern sollte. das verhalten sollte also in ordnung sein.

In der PR Beschreibung geht es noch um window, dass wird hier nicht berücksichtigt, oder?

sowohl window als auch document sind nicht instance of Element. diese beiden fälle werden durch den instanceof check abgedeckt.

@philippotto philippotto merged commit b274c45 into master Jan 10, 2024
2 checks passed
@philippotto philippotto deleted the fix-t-matches branch January 10, 2024 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants