Skip to content

Properly remove window mouse event listeners#2632

Merged
madsmtm merged 4 commits intorust-windowing:masterfrom
DouglasDwyer:ms-web-mouse-event
Jan 21, 2023
Merged

Properly remove window mouse event listeners#2632
madsmtm merged 4 commits intorust-windowing:masterfrom
DouglasDwyer:ms-web-mouse-event

Conversation

@DouglasDwyer
Copy link
Copy Markdown
Contributor

@DouglasDwyer DouglasDwyer commented Jan 14, 2023

  • Tested on all platforms changed
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • [N/A] Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • [N/A] Created or updated an example program if it would help users understand this functionality
  • [N/A] Updated feature matrix, if new features were added or implemented

The web backend for winit registers the onmousedown, onmouseup, and onmousemove events on the global Javascript window object. When the winit window is destroyed, however, these events are not properly removed. This is because the events are registered with the capture: true, but capture: true is not passed to the appropriate removeEventListener call. This PR updates the web backend to pass the event creation options to the event removal function. This prevents errors in which the Javascript runtime tries to invoke the event listener closures after they are destroyed.

@kchibisov kchibisov mentioned this pull request Jan 17, 2023
11 tasks
@maroider maroider added the DS - web Affects the Web backend (WebAssembly/WASM) label Jan 17, 2023
@maroider
Copy link
Copy Markdown
Member

maroider commented Jan 17, 2023

It looks like you're right about that, but I'm stumped as to how you managed to observe it in the first place. Even for seemingly obvious stuff like this, I prefer to reproduce the error and observe that the fix works as intended.

@DouglasDwyer
Copy link
Copy Markdown
Contributor Author

@maroider Absolutely! This can be reproduced by creating a Winit window on the web platform, and then dropping it, while allowing the event loop to continue. When you wave your cursor over the browser window after this happens, errors about a destroyed closure should appear in the console.

Copy link
Copy Markdown
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

I'm fine with merging this now unless @maroider wants to test it first?

@maroider
Copy link
Copy Markdown
Member

I had a go at following your instructions, but I couldn't get the error to show up in the console. It's probably fine to merge it as-is, even if I couldn't reproduce the issue, as the MDN docs clearly show we're using removeEventListener wrong anyway.

@madsmtm madsmtm merged commit b711a11 into rust-windowing:master Jan 21, 2023
@madsmtm
Copy link
Copy Markdown
Member

madsmtm commented Jan 21, 2023

Thank you @DouglasDwyer!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

DS - web Affects the Web backend (WebAssembly/WASM)

Development

Successfully merging this pull request may close these issues.

3 participants