From 3813240e46064f45b7778ad99e67dd582c56115a Mon Sep 17 00:00:00 2001 From: Alvin Wong Date: Sat, 19 Sep 2020 00:01:41 +0800 Subject: [PATCH 1/7] web-sys: Impl. event listeners removal for canvas --- src/platform_impl/web/web_sys/canvas.rs | 52 ++++++++------- .../web/web_sys/canvas/mouse_handler.rs | 12 ++-- .../web/web_sys/canvas/pointer_handler.rs | 12 ++-- src/platform_impl/web/web_sys/event_handle.rs | 65 +++++++++++++++++++ src/platform_impl/web/web_sys/mod.rs | 1 + 5 files changed, 107 insertions(+), 35 deletions(-) create mode 100644 src/platform_impl/web/web_sys/event_handle.rs diff --git a/src/platform_impl/web/web_sys/canvas.rs b/src/platform_impl/web/web_sys/canvas.rs index 81e152c1e9..49023527c1 100644 --- a/src/platform_impl/web/web_sys/canvas.rs +++ b/src/platform_impl/web/web_sys/canvas.rs @@ -1,4 +1,5 @@ use super::event; +use super::event_handle::EventListenerHandle; use crate::dpi::{LogicalPosition, PhysicalPosition, PhysicalSize}; use crate::error::OsError as RootOE; use crate::event::{ModifiersState, MouseButton, MouseScrollDelta, ScanCode, VirtualKeyCode}; @@ -18,13 +19,13 @@ mod pointer_handler; pub struct Canvas { common: Common, - on_focus: Option>, - on_blur: Option>, - on_keyboard_release: Option>, - on_keyboard_press: Option>, - on_received_character: Option>, - on_mouse_wheel: Option>, - on_fullscreen_change: Option>, + on_focus: Option>, + on_blur: Option>, + on_keyboard_release: Option>, + on_keyboard_press: Option>, + on_received_character: Option>, + on_mouse_wheel: Option>, + on_fullscreen_change: Option>, on_dark_mode: Option>, mouse_state: MouseState, } @@ -292,7 +293,11 @@ impl Canvas { } impl Common { - fn add_event(&self, event_name: &str, mut handler: F) -> Closure + fn add_event( + &self, + event_name: &'static str, + mut handler: F, + ) -> EventListenerHandle where E: 'static + AsRef + wasm_bindgen::convert::FromWasmAbi, F: 'static + FnMut(E), @@ -307,17 +312,19 @@ impl Common { handler(event); }) as Box); - self.raw - .add_event_listener_with_callback(event_name, &closure.as_ref().unchecked_ref()) - .expect("Failed to add event listener with callback"); + let listener = EventListenerHandle::new(&self.raw, event_name, closure); - closure + listener } // The difference between add_event and add_user_event is that the latter has a special meaning // for browser security. A user event is a deliberate action by the user (like a mouse or key // press) and is the only time things like a fullscreen request may be successfully completed.) - fn add_user_event(&self, event_name: &str, mut handler: F) -> Closure + fn add_user_event( + &self, + event_name: &'static str, + mut handler: F, + ) -> EventListenerHandle where E: 'static + AsRef + wasm_bindgen::convert::FromWasmAbi, F: 'static + FnMut(E), @@ -343,9 +350,9 @@ impl Common { // handling to control event propagation. fn add_window_mouse_event( &self, - event_name: &str, + event_name: &'static str, mut handler: F, - ) -> Closure + ) -> EventListenerHandle where F: 'static + FnMut(MouseEvent), { @@ -364,15 +371,14 @@ impl Common { } }) as Box); - window - .add_event_listener_with_callback_and_add_event_listener_options( - event_name, - &closure.as_ref().unchecked_ref(), - AddEventListenerOptions::new().capture(true), - ) - .expect("Failed to add event listener with callback and options"); + let listener = EventListenerHandle::with_options( + &window, + event_name, + closure, + AddEventListenerOptions::new().capture(true), + ); - closure + listener } pub fn request_fullscreen(&self) { diff --git a/src/platform_impl/web/web_sys/canvas/mouse_handler.rs b/src/platform_impl/web/web_sys/canvas/mouse_handler.rs index 37fdd018e3..2afd2ce36d 100644 --- a/src/platform_impl/web/web_sys/canvas/mouse_handler.rs +++ b/src/platform_impl/web/web_sys/canvas/mouse_handler.rs @@ -1,19 +1,19 @@ use super::event; +use super::EventListenerHandle; use crate::dpi::PhysicalPosition; use crate::event::{ModifiersState, MouseButton}; use std::cell::RefCell; use std::rc::Rc; -use wasm_bindgen::closure::Closure; use web_sys::{EventTarget, MouseEvent}; pub(super) struct MouseHandler { - on_mouse_leave: Option>, - on_mouse_enter: Option>, - on_mouse_move: Option>, - on_mouse_press: Option>, - on_mouse_release: Option>, + on_mouse_leave: Option>, + on_mouse_enter: Option>, + on_mouse_move: Option>, + on_mouse_press: Option>, + on_mouse_release: Option>, on_mouse_leave_handler: Rc>>>, mouse_capture_state: Rc>, } diff --git a/src/platform_impl/web/web_sys/canvas/pointer_handler.rs b/src/platform_impl/web/web_sys/canvas/pointer_handler.rs index f9787dd5d5..b381c45e1b 100644 --- a/src/platform_impl/web/web_sys/canvas/pointer_handler.rs +++ b/src/platform_impl/web/web_sys/canvas/pointer_handler.rs @@ -1,16 +1,16 @@ use super::event; +use super::EventListenerHandle; use crate::dpi::PhysicalPosition; use crate::event::{ModifiersState, MouseButton}; -use wasm_bindgen::closure::Closure; use web_sys::PointerEvent; pub(super) struct PointerHandler { - on_cursor_leave: Option>, - on_cursor_enter: Option>, - on_cursor_move: Option>, - on_pointer_press: Option>, - on_pointer_release: Option>, + on_cursor_leave: Option>, + on_cursor_enter: Option>, + on_cursor_move: Option>, + on_pointer_press: Option>, + on_pointer_release: Option>, } impl PointerHandler { diff --git a/src/platform_impl/web/web_sys/event_handle.rs b/src/platform_impl/web/web_sys/event_handle.rs new file mode 100644 index 0000000000..08130d7598 --- /dev/null +++ b/src/platform_impl/web/web_sys/event_handle.rs @@ -0,0 +1,65 @@ +use wasm_bindgen::{prelude::Closure, JsCast}; +use web_sys::{AddEventListenerOptions, EventTarget}; + +pub(super) struct EventListenerHandle { + target: EventTarget, + event_type: &'static str, + listener: Closure, +} + +impl EventListenerHandle { + pub fn new(target: &U, event_type: &'static str, listener: Closure) -> Self + where + U: Clone + Into, + { + let target = target.clone().into(); + target + .add_event_listener_with_callback(event_type, listener.as_ref().unchecked_ref()) + .expect("Failed to add event listener"); + EventListenerHandle { + target, + event_type, + listener, + } + } + + pub fn with_options( + target: &U, + event_type: &'static str, + listener: Closure, + options: &AddEventListenerOptions, + ) -> Self + where + U: Clone + Into, + { + let target = target.clone().into(); + target + .add_event_listener_with_callback_and_add_event_listener_options( + event_type, + listener.as_ref().unchecked_ref(), + options, + ) + .expect("Failed to add event listener"); + EventListenerHandle { + target, + event_type, + listener, + } + } +} + +impl Drop for EventListenerHandle { + fn drop(&mut self) { + self.target + .remove_event_listener_with_callback( + self.event_type, + self.listener.as_ref().unchecked_ref(), + ) + .unwrap_or_else(|e| { + web_sys::console::error_2( + &format!("Error removing event listener {}", self.event_type).into(), + &e, + ) + }); + } +} diff --git a/src/platform_impl/web/web_sys/mod.rs b/src/platform_impl/web/web_sys/mod.rs index 338869acd3..a8a70517af 100644 --- a/src/platform_impl/web/web_sys/mod.rs +++ b/src/platform_impl/web/web_sys/mod.rs @@ -1,5 +1,6 @@ mod canvas; mod event; +mod event_handle; mod scaling; mod timeout; From 58e105306f0effd78bbdbf8a9f55fb29432c605e Mon Sep 17 00:00:00 2001 From: Alvin Wong Date: Sat, 19 Sep 2020 16:53:33 +0800 Subject: [PATCH 2/7] web-sys: Impl. media query listeners cleanup --- src/platform_impl/web/web_sys/canvas.rs | 25 +++------ .../web/web_sys/media_query_handle.rs | 56 +++++++++++++++++++ src/platform_impl/web/web_sys/mod.rs | 1 + src/platform_impl/web/web_sys/scaling.rs | 51 +++++------------ 4 files changed, 79 insertions(+), 54 deletions(-) create mode 100644 src/platform_impl/web/web_sys/media_query_handle.rs diff --git a/src/platform_impl/web/web_sys/canvas.rs b/src/platform_impl/web/web_sys/canvas.rs index 49023527c1..bedfbf49c7 100644 --- a/src/platform_impl/web/web_sys/canvas.rs +++ b/src/platform_impl/web/web_sys/canvas.rs @@ -1,5 +1,6 @@ use super::event; use super::event_handle::EventListenerHandle; +use super::media_query_handle::MediaQueryListHandle; use crate::dpi::{LogicalPosition, PhysicalPosition, PhysicalSize}; use crate::error::OsError as RootOE; use crate::event::{ModifiersState, MouseButton, MouseScrollDelta, ScanCode, VirtualKeyCode}; @@ -26,7 +27,7 @@ pub struct Canvas { on_received_character: Option>, on_mouse_wheel: Option>, on_fullscreen_change: Option>, - on_dark_mode: Option>, + on_dark_mode: Option, mouse_state: MouseState, } @@ -265,22 +266,12 @@ impl Canvas { where F: 'static + FnMut(bool), { - let window = web_sys::window().expect("Failed to obtain window"); - - self.on_dark_mode = window - .match_media("(prefers-color-scheme: dark)") - .ok() - .flatten() - .and_then(|media| { - let closure = Closure::wrap(Box::new(move |event: MediaQueryListEvent| { - handler(event.matches()) - }) as Box); - - media - .add_listener_with_opt_callback(Some(&closure.as_ref().unchecked_ref())) - .map(|_| closure) - .ok() - }); + let closure = + Closure::wrap( + Box::new(move |event: MediaQueryListEvent| handler(event.matches())) + as Box, + ); + self.on_dark_mode = MediaQueryListHandle::new("(prefers-color-scheme: dark)", closure); } pub fn request_fullscreen(&self) { diff --git a/src/platform_impl/web/web_sys/media_query_handle.rs b/src/platform_impl/web/web_sys/media_query_handle.rs new file mode 100644 index 0000000000..c2ab40da77 --- /dev/null +++ b/src/platform_impl/web/web_sys/media_query_handle.rs @@ -0,0 +1,56 @@ +use wasm_bindgen::{prelude::Closure, JsCast}; +use web_sys::{MediaQueryList, MediaQueryListEvent}; + +pub(super) struct MediaQueryListHandle { + mql: MediaQueryList, + listener: Option>, +} + +impl MediaQueryListHandle { + pub fn new( + media_query: &str, + listener: Closure, + ) -> Option { + let window = web_sys::window().expect("Failed to obtain window"); + let mql = window + .match_media(media_query) + .ok() + .flatten() + .and_then(|mql| { + mql.add_listener_with_opt_callback(Some(&listener.as_ref().unchecked_ref())) + .map(|_| mql) + .ok() + }); + mql.map(|mql| Self { + mql, + listener: Some(listener), + }) + } + + pub fn mql(&self) -> &MediaQueryList { + &self.mql + } + + /// Removes the listener and returns the original listener closure, which + /// can be reused. + pub fn remove(mut self) -> Closure { + let listener = self.listener.take().unwrap_or_else(|| unreachable!()); + remove_listener(&self.mql, &listener); + listener + } +} + +impl Drop for MediaQueryListHandle { + fn drop(&mut self) { + if let Some(listener) = self.listener.take() { + remove_listener(&self.mql, &listener); + } + } +} + +fn remove_listener(mql: &MediaQueryList, listener: &Closure) { + mql.remove_listener_with_opt_callback(Some(listener.as_ref().unchecked_ref())) + .unwrap_or_else(|e| { + web_sys::console::error_2(&"Error removing media query listener".into(), &e) + }); +} diff --git a/src/platform_impl/web/web_sys/mod.rs b/src/platform_impl/web/web_sys/mod.rs index a8a70517af..74d900350c 100644 --- a/src/platform_impl/web/web_sys/mod.rs +++ b/src/platform_impl/web/web_sys/mod.rs @@ -1,6 +1,7 @@ mod canvas; mod event; mod event_handle; +mod media_query_handle; mod scaling; mod timeout; diff --git a/src/platform_impl/web/web_sys/scaling.rs b/src/platform_impl/web/web_sys/scaling.rs index 5018257fa5..1925d5f97e 100644 --- a/src/platform_impl/web/web_sys/scaling.rs +++ b/src/platform_impl/web/web_sys/scaling.rs @@ -1,8 +1,9 @@ use super::super::ScaleChangeArgs; +use super::media_query_handle::MediaQueryListHandle; use std::{cell::RefCell, rc::Rc}; -use wasm_bindgen::{prelude::Closure, JsCast}; -use web_sys::{MediaQueryList, MediaQueryListEvent}; +use wasm_bindgen::prelude::Closure; +use web_sys::MediaQueryListEvent; pub struct ScaleChangeDetector(Rc>); @@ -19,8 +20,7 @@ impl ScaleChangeDetector { /// changes of the `devicePixelRatio`. struct ScaleChangeDetectorInternal { callback: Box, - closure: Option>, - mql: Option, + mql: Option, last_scale: f64, } @@ -32,7 +32,6 @@ impl ScaleChangeDetectorInternal { let current_scale = super::scale_factor(); let new_self = Rc::new(RefCell::new(Self { callback: Box::new(handler), - closure: None, mql: None, last_scale: current_scale, })); @@ -42,17 +41,17 @@ impl ScaleChangeDetectorInternal { cloned_self.borrow_mut().handler(event) }) as Box); - let mql = Self::create_mql(&closure); + let mql = Self::create_mql(closure); { let mut borrowed_self = new_self.borrow_mut(); - borrowed_self.closure = Some(closure); borrowed_self.mql = mql; } new_self } - fn create_mql(closure: &Closure) -> Option { - let window = web_sys::window().expect("Failed to obtain window"); + fn create_mql( + closure: Closure, + ) -> Option { let current_scale = super::scale_factor(); // This media query initially matches the current `devicePixelRatio`. // We add 0.0001 to the lower and upper bounds such that it won't fail @@ -62,30 +61,20 @@ impl ScaleChangeDetectorInternal { current_scale - 0.0001, current_scale + 0.0001, ); - window - .match_media(&media_query) - .ok() - .flatten() - .and_then(|mql| { - assert_eq!(mql.matches(), true); - mql.add_listener_with_opt_callback(Some(&closure.as_ref().unchecked_ref())) - .map(|_| mql) - .ok() - }) + let mql = MediaQueryListHandle::new(&media_query, closure); + if let Some(mql) = &mql { + assert_eq!(mql.mql().matches(), true); + } + mql } fn handler(&mut self, event: MediaQueryListEvent) { assert_eq!(event.matches(), false); - let closure = self - .closure - .as_ref() - .expect("DevicePixelRatioChangeDetector::closure should not be None"); let mql = self .mql .take() .expect("DevicePixelRatioChangeDetector::mql should not be None"); - mql.remove_listener_with_opt_callback(Some(closure.as_ref().unchecked_ref())) - .expect("Failed to remove listener from MediaQueryList"); + let closure = mql.remove(); let new_scale = super::scale_factor(); (self.callback)(ScaleChangeArgs { old_scale: self.last_scale, @@ -96,15 +85,3 @@ impl ScaleChangeDetectorInternal { self.last_scale = new_scale; } } - -impl Drop for ScaleChangeDetectorInternal { - fn drop(&mut self) { - match (self.closure.as_ref(), self.mql.as_ref()) { - (Some(closure), Some(mql)) => { - let _ = - mql.remove_listener_with_opt_callback(Some(closure.as_ref().unchecked_ref())); - } - _ => {} - } - } -} From a773054d0496e4f9cc0aad5cc21b52d3ef10ad8f Mon Sep 17 00:00:00 2001 From: Alvin Wong Date: Sat, 19 Sep 2020 22:13:01 +0800 Subject: [PATCH 3/7] web: Emit WindowEvent::Destroyed after Window is dropped --- src/platform_impl/web/event_loop/runner.rs | 39 ++++++++++++++++++++++ src/platform_impl/web/window.rs | 13 ++++++++ 2 files changed, 52 insertions(+) diff --git a/src/platform_impl/web/event_loop/runner.rs b/src/platform_impl/web/event_loop/runner.rs index 0ccd7862c2..f0ae78529f 100644 --- a/src/platform_impl/web/event_loop/runner.rs +++ b/src/platform_impl/web/event_loop/runner.rs @@ -26,6 +26,7 @@ pub struct Execution { id: RefCell, all_canvases: RefCell>, redraw_pending: RefCell>, + destroy_pending: RefCell>, scale_change_detector: RefCell>, } @@ -88,6 +89,7 @@ impl Shared { id: RefCell::new(0), all_canvases: RefCell::new(Vec::new()), redraw_pending: RefCell::new(HashSet::new()), + destroy_pending: RefCell::new(VecDeque::new()), scale_change_detector: RefCell::new(None), })) } @@ -96,6 +98,10 @@ impl Shared { self.0.all_canvases.borrow_mut().push((id, canvas)); } + pub fn notify_destroy_window(&self, id: WindowId) { + self.0.destroy_pending.borrow_mut().push_back(id); + } + // Set the event callback to use for the event loop runner // This the event callback is a fairly thin layer over the user-provided callback that closes // over a RootEventLoopWindowTarget reference @@ -206,6 +212,26 @@ impl Shared { self.run_until_cleared(events); } + // Process the destroy-pending windows. This should only be called from + // `run_until_cleared` and `handle_scale_changed`, somewhere between emitting + // `NewEvents` and `MainEventsCleared`. + fn process_destroy_pending_windows(&self, control: &mut root::ControlFlow) { + while let Some(id) = self.0.destroy_pending.borrow_mut().pop_front() { + self.0 + .all_canvases + .borrow_mut() + .retain(|&(item_id, _)| item_id != id); + self.handle_event( + Event::WindowEvent { + window_id: id, + event: crate::event::WindowEvent::Destroyed, + }, + control, + ); + self.0.redraw_pending.borrow_mut().remove(&id); + } + } + // Given the set of new events, run the event loop until the main events and redraw events are // cleared // @@ -215,6 +241,7 @@ impl Shared { for event in events { self.handle_event(event, &mut control); } + self.process_destroy_pending_windows(&mut control); self.handle_event(Event::MainEventsCleared, &mut control); // Collect all of the redraw events to avoid double-locking the RefCell @@ -233,6 +260,11 @@ impl Shared { } pub fn handle_scale_changed(&self, old_scale: f64, new_scale: f64) { + // If there aren't any windows, then there is nothing to do here. + if self.0.all_canvases.borrow().is_empty() { + return; + } + let start_cause = match (self.0.runner.borrow().as_ref()) .unwrap_or_else(|| unreachable!("`scale_changed` should not happen without a runner")) .maybe_start_cause() @@ -246,6 +278,11 @@ impl Shared { // Handle the start event and all other events in the queue. self.handle_event(Event::NewEvents(start_cause), &mut control); + // It is possible for windows to be dropped before this point. We don't + // want to send `ScaleFactorChanged` for destroyed windows, so we process + // the destroy-pending windows here. + self.process_destroy_pending_windows(&mut control); + // Now handle the `ScaleFactorChanged` events. for &(id, ref canvas) in &*self.0.all_canvases.borrow() { // First, we send the `ScaleFactorChanged` event: @@ -277,6 +314,8 @@ impl Shared { ); } + // Process the destroy-pending windows again. + self.process_destroy_pending_windows(&mut control); self.handle_event(Event::MainEventsCleared, &mut control); // Discard all the pending redraw as we shall just redraw all windows. diff --git a/src/platform_impl/web/window.rs b/src/platform_impl/web/window.rs index 165661fafa..2df4768560 100644 --- a/src/platform_impl/web/window.rs +++ b/src/platform_impl/web/window.rs @@ -17,6 +17,7 @@ pub struct Window { previous_pointer: RefCell<&'static str>, id: Id, register_redraw_request: Box, + destroy_fn: Option>, } impl Window { @@ -35,11 +36,15 @@ impl Window { target.register(&mut canvas, id); + let runner = target.runner.clone(); + let destroy_fn = Box::new(move || runner.notify_destroy_window(RootWI(id))); + let window = Window { canvas, previous_pointer: RefCell::new("auto"), id, register_redraw_request, + destroy_fn: Some(destroy_fn), }; window.set_inner_size(attr.inner_size.unwrap_or(Size::Logical(LogicalSize { @@ -277,6 +282,14 @@ impl Window { } } +impl Drop for Window { + fn drop(&mut self) { + if let Some(destroy_fn) = self.destroy_fn.take() { + destroy_fn(); + } + } +} + #[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] pub struct Id(pub(crate) u32); From b1e94423a3380ca576e630ff49a6887086ea59ad Mon Sep 17 00:00:00 2001 From: Alvin Wong Date: Mon, 21 Sep 2020 01:38:31 +0800 Subject: [PATCH 4/7] web-sys: Fix unload event closure being dropped early --- src/platform_impl/web/event_loop/runner.rs | 5 ++++- src/platform_impl/web/stdweb/mod.rs | 4 +++- src/platform_impl/web/web_sys/mod.rs | 15 ++++++++++----- 3 files changed, 17 insertions(+), 7 deletions(-) diff --git a/src/platform_impl/web/event_loop/runner.rs b/src/platform_impl/web/event_loop/runner.rs index f0ae78529f..7326ede4f7 100644 --- a/src/platform_impl/web/event_loop/runner.rs +++ b/src/platform_impl/web/event_loop/runner.rs @@ -28,6 +28,7 @@ pub struct Execution { redraw_pending: RefCell>, destroy_pending: RefCell>, scale_change_detector: RefCell>, + unload_event_handle: RefCell>, } struct Runner { @@ -91,6 +92,7 @@ impl Shared { redraw_pending: RefCell::new(HashSet::new()), destroy_pending: RefCell::new(VecDeque::new()), scale_change_detector: RefCell::new(None), + unload_event_handle: RefCell::new(None), })) } @@ -113,7 +115,8 @@ impl Shared { self.init(); let close_instance = self.clone(); - backend::on_unload(move || close_instance.handle_unload()); + *self.0.unload_event_handle.borrow_mut() = + Some(backend::on_unload(move || close_instance.handle_unload())); } pub(crate) fn set_on_scale_change(&self, handler: F) diff --git a/src/platform_impl/web/stdweb/mod.rs b/src/platform_impl/web/stdweb/mod.rs index 2f38c178a8..57b496a4d2 100644 --- a/src/platform_impl/web/stdweb/mod.rs +++ b/src/platform_impl/web/stdweb/mod.rs @@ -26,7 +26,9 @@ pub fn exit_fullscreen() { document().exit_fullscreen(); } -pub fn on_unload(mut handler: impl FnMut() + 'static) { +pub type UnloadEventHandle = (); + +pub fn on_unload(mut handler: impl FnMut() + 'static) -> UnloadEventHandle { window().add_event_listener(move |_: BeforeUnloadEvent| handler()); } diff --git a/src/platform_impl/web/web_sys/mod.rs b/src/platform_impl/web/web_sys/mod.rs index 74d900350c..8057f7756a 100644 --- a/src/platform_impl/web/web_sys/mod.rs +++ b/src/platform_impl/web/web_sys/mod.rs @@ -12,7 +12,7 @@ pub use self::timeout::{AnimationFrameRequest, Timeout}; use crate::dpi::{LogicalSize, Size}; use crate::platform::web::WindowExtWebSys; use crate::window::Window; -use wasm_bindgen::{closure::Closure, JsCast}; +use wasm_bindgen::closure::Closure; use web_sys::{window, BeforeUnloadEvent, Element, HtmlCanvasElement}; pub fn throw(msg: &str) { @@ -26,16 +26,21 @@ pub fn exit_fullscreen() { document.exit_fullscreen(); } -pub fn on_unload(mut handler: impl FnMut() + 'static) { +pub struct UnloadEventHandle { + _listener: event_handle::EventListenerHandle, +} + +pub fn on_unload(mut handler: impl FnMut() + 'static) -> UnloadEventHandle { let window = web_sys::window().expect("Failed to obtain window"); let closure = Closure::wrap( Box::new(move |_: BeforeUnloadEvent| handler()) as Box ); - window - .add_event_listener_with_callback("beforeunload", &closure.as_ref().unchecked_ref()) - .expect("Failed to add close listener"); + let listener = event_handle::EventListenerHandle::new(&window, "beforeunload", closure); + UnloadEventHandle { + _listener: listener, + } } impl WindowExtWebSys for Window { From 8f9e779537a42ba450b859eee3cbf83771880d1b Mon Sep 17 00:00:00 2001 From: Alvin Wong Date: Mon, 21 Sep 2020 01:57:55 +0800 Subject: [PATCH 5/7] web: Impl. cleanup on ControlFlow::Exit - Drops the Runner, which causes the event handler closure to be dropped. - (web-sys only:) Remove event listeners from DOM. --- src/platform_impl/web/event_loop/runner.rs | 135 ++++++++++++++---- .../web/event_loop/window_target.rs | 9 +- src/platform_impl/web/stdweb/canvas.rs | 4 + src/platform_impl/web/web_sys/canvas.rs | 15 ++ .../web/web_sys/canvas/mouse_handler.rs | 9 ++ .../web/web_sys/canvas/pointer_handler.rs | 8 ++ src/platform_impl/web/web_sys/scaling.rs | 6 +- src/platform_impl/web/window.rs | 42 +++--- 8 files changed, 175 insertions(+), 53 deletions(-) diff --git a/src/platform_impl/web/event_loop/runner.rs b/src/platform_impl/web/event_loop/runner.rs index 7326ede4f7..af46e02472 100644 --- a/src/platform_impl/web/event_loop/runner.rs +++ b/src/platform_impl/web/event_loop/runner.rs @@ -9,7 +9,7 @@ use std::{ clone::Clone, collections::{HashSet, VecDeque}, iter, - rc::Rc, + rc::{Rc, Weak}, }; pub struct Shared(Rc>); @@ -21,16 +21,36 @@ impl Clone for Shared { } pub struct Execution { - runner: RefCell>>, + runner: RefCell>, events: RefCell>>, id: RefCell, - all_canvases: RefCell>, + all_canvases: RefCell>)>>, redraw_pending: RefCell>, destroy_pending: RefCell>, scale_change_detector: RefCell>, unload_event_handle: RefCell>, } +enum RunnerEnum { + /// The `EventLoop` is created but not being run. + Pending, + /// The `EventLoop` is being run. + Running(Runner), + /// The `EventLoop` is exited after being started with `EventLoop::run`. Since + /// `EventLoop::run` takes ownership of the `EventLoop`, we can be certain + /// that this event loop will never be run again. + Destroyed, +} + +impl RunnerEnum { + fn maybe_runner(&self) -> Option<&Runner> { + match self { + RunnerEnum::Running(runner) => Some(runner), + _ => None, + } + } +} + struct Runner { state: State, is_busy: bool, @@ -85,7 +105,7 @@ impl Runner { impl Shared { pub fn new() -> Self { Shared(Rc::new(Execution { - runner: RefCell::new(None), + runner: RefCell::new(RunnerEnum::Pending), events: RefCell::new(VecDeque::new()), id: RefCell::new(0), all_canvases: RefCell::new(Vec::new()), @@ -96,8 +116,11 @@ impl Shared { })) } - pub fn add_canvas(&self, id: WindowId, canvas: backend::RawCanvasType) { - self.0.all_canvases.borrow_mut().push((id, canvas)); + pub fn add_canvas(&self, id: WindowId, canvas: &Rc>) { + self.0 + .all_canvases + .borrow_mut() + .push((id, Rc::downgrade(canvas))); } pub fn notify_destroy_window(&self, id: WindowId) { @@ -111,7 +134,11 @@ impl Shared { &self, event_handler: Box, &mut root::ControlFlow)>, ) { - self.0.runner.replace(Some(Runner::new(event_handler))); + { + let mut runner = self.0.runner.borrow_mut(); + assert!(matches!(*runner, RunnerEnum::Pending)); + *runner = RunnerEnum::Running(Runner::new(event_handler)); + } self.init(); let close_instance = self.clone(); @@ -178,18 +205,23 @@ impl Shared { } // If we can run the event processing right now, or need to queue this and wait for later let mut process_immediately = true; - if let Some(ref runner) = &*self.0.runner.borrow() { - // If we're currently polling, queue this and wait for the poll() method to be called - if let State::Poll { .. } = runner.state { - process_immediately = false; + match &*self.0.runner.borrow() { + RunnerEnum::Running(ref runner) => { + // If we're currently polling, queue this and wait for the poll() method to be called + if let State::Poll { .. } = runner.state { + process_immediately = false; + } + // If the runner is busy, queue this and wait for it to process it later + if runner.is_busy { + process_immediately = false; + } } - // If the runner is busy, queue this and wait for it to process it later - if runner.is_busy { + RunnerEnum::Pending => { + // The runner still hasn't been attached: queue this event and wait for it to be process_immediately = false; } - } else { - // The runner still hasn't been attached: queue this event and wait for it to be - process_immediately = false; + // This is unreachable since `self.is_closed() == true`. + RunnerEnum::Destroyed => unreachable!(), } if !process_immediately { // Queue these events to look at later @@ -198,7 +230,7 @@ impl Shared { } // At this point, we know this is a fresh set of events // Now we determine why new events are incoming, and handle the events - let start_cause = match (self.0.runner.borrow().as_ref()) + let start_cause = match (self.0.runner.borrow().maybe_runner()) .unwrap_or_else(|| { unreachable!("The runner cannot process events when it is not attached") }) @@ -258,7 +290,7 @@ impl Shared { // If the event loop is closed, it has been closed this iteration and now the closing // event should be emitted if self.is_closed() { - self.handle_event(Event::LoopDestroyed, &mut control); + self.handle_loop_destroyed(&mut control); } } @@ -268,7 +300,7 @@ impl Shared { return; } - let start_cause = match (self.0.runner.borrow().as_ref()) + let start_cause = match (self.0.runner.borrow().maybe_runner()) .unwrap_or_else(|| unreachable!("`scale_changed` should not happen without a runner")) .maybe_start_cause() { @@ -288,6 +320,11 @@ impl Shared { // Now handle the `ScaleFactorChanged` events. for &(id, ref canvas) in &*self.0.all_canvases.borrow() { + let canvas = match canvas.upgrade() { + Some(rc) => rc.borrow().raw().clone(), + // This shouldn't happen, but just in case... + None => continue, + }; // First, we send the `ScaleFactorChanged` event: let current_size = crate::dpi::PhysicalSize { width: canvas.width() as u32, @@ -307,7 +344,7 @@ impl Shared { ); // Then we resize the canvas to the new size and send a `Resized` event: - backend::set_canvas_size(canvas, crate::dpi::Size::Physical(new_size)); + backend::set_canvas_size(&canvas, crate::dpi::Size::Physical(new_size)); self.handle_single_event_sync( Event::WindowEvent { window_id: id, @@ -332,13 +369,15 @@ impl Shared { // If the event loop is closed, it has been closed this iteration and now the closing // event should be emitted if self.is_closed() { - self.handle_event(Event::LoopDestroyed, &mut control); + self.handle_loop_destroyed(&mut control); } } fn handle_unload(&self) { self.apply_control_flow(root::ControlFlow::Exit); let mut control = self.current_control_flow(); + // We don't call `handle_loop_destroyed` here because we don't need to + // perform cleanup when the web browser is going to destroy the page. self.handle_event(Event::LoopDestroyed, &mut control); } @@ -350,7 +389,7 @@ impl Shared { *control = root::ControlFlow::Exit; } match *self.0.runner.borrow_mut() { - Some(ref mut runner) => { + RunnerEnum::Running(ref mut runner) => { runner.handle_single_event(event, control); } _ => panic!("Cannot handle event synchronously without a runner"), @@ -365,19 +404,21 @@ impl Shared { *control = root::ControlFlow::Exit; } match *self.0.runner.borrow_mut() { - Some(ref mut runner) => { + RunnerEnum::Running(ref mut runner) => { runner.handle_single_event(event, control); } // If an event is being handled without a runner somehow, add it to the event queue so // it will eventually be processed - _ => self.0.events.borrow_mut().push_back(event), + RunnerEnum::Pending => self.0.events.borrow_mut().push_back(event), + // If the Runner has been destroyed, there is nothing to do. + RunnerEnum::Destroyed => return, } let is_closed = *control == root::ControlFlow::Exit; // Don't take events out of the queue if the loop is closed or the runner doesn't exist // If the runner doesn't exist and this method recurses, it will recurse infinitely - if !is_closed && self.0.runner.borrow().is_some() { + if !is_closed && self.0.runner.borrow().maybe_runner().is_some() { // Take an event out of the queue and handle it // Make sure not to let the borrow_mut live during the next handle_event let event = { self.0.events.borrow_mut().pop_front() }; @@ -424,26 +465,58 @@ impl Shared { }; match *self.0.runner.borrow_mut() { - Some(ref mut runner) => { + RunnerEnum::Running(ref mut runner) => { runner.state = new_state; } - None => (), + _ => (), + } + } + + fn handle_loop_destroyed(&self, control: &mut root::ControlFlow) { + self.handle_event(Event::LoopDestroyed, control); + let all_canvases = std::mem::take(&mut *self.0.all_canvases.borrow_mut()); + *self.0.scale_change_detector.borrow_mut() = None; + *self.0.unload_event_handle.borrow_mut() = None; + // Dropping the `Runner` drops the event handler closure, which will in + // turn drop all `Window`s moved into the closure. + *self.0.runner.borrow_mut() = RunnerEnum::Destroyed; + for (_, canvas) in all_canvases { + // In case any remaining `Window`s are still not dropped, we will need + // to explicitly remove the event handlers associated with their canvases. + if let Some(canvas) = canvas.upgrade() { + let mut canvas = canvas.borrow_mut(); + canvas.remove_listeners(); + } } + // At this point, the `self.0` `Rc` should only be strongly referenced + // by the following: + // * `self`, i.e. the item which triggered this event loop wakeup, which + // is usually a `wasm-bindgen` `Closure`, which will be dropped after + // returning to the JS glue code. + // * The `EventLoopWindowTarget` leaked inside `EventLoop::run` due to the + // JS exception thrown at the end. + // * For each undropped `Window`: + // * The `register_redraw_request` closure. + // * The `destroy_fn` closure. } // Check if the event loop is currently closed fn is_closed(&self) -> bool { match *self.0.runner.borrow() { - Some(ref runner) => runner.state.is_exit(), - None => false, // If the event loop is None, it has not been intialised yet, so it cannot be closed + RunnerEnum::Running(ref runner) => runner.state.is_exit(), + // The event loop is not closed since it is not initialized. + RunnerEnum::Pending => false, + // The event loop is closed since it has been destroyed. + RunnerEnum::Destroyed => true, } } // Get the current control flow state fn current_control_flow(&self) -> root::ControlFlow { match *self.0.runner.borrow() { - Some(ref runner) => runner.state.control_flow(), - None => root::ControlFlow::Poll, + RunnerEnum::Running(ref runner) => runner.state.control_flow(), + RunnerEnum::Pending => root::ControlFlow::Poll, + RunnerEnum::Destroyed => root::ControlFlow::Exit, } } } diff --git a/src/platform_impl/web/event_loop/window_target.rs b/src/platform_impl/web/event_loop/window_target.rs index f26f984a65..7818ac0d37 100644 --- a/src/platform_impl/web/event_loop/window_target.rs +++ b/src/platform_impl/web/event_loop/window_target.rs @@ -4,8 +4,10 @@ use crate::event::{DeviceId, ElementState, Event, KeyboardInput, TouchPhase, Win use crate::event_loop::ControlFlow; use crate::monitor::MonitorHandle as RootMH; use crate::window::{Theme, WindowId}; +use std::cell::RefCell; use std::clone::Clone; use std::collections::{vec_deque::IntoIter as VecDequeIter, VecDeque}; +use std::rc::Rc; pub struct WindowTarget { pub(crate) runner: runner::Shared, @@ -42,11 +44,12 @@ impl WindowTarget { window::Id(self.runner.generate_id()) } - pub fn register(&self, canvas: &mut backend::Canvas, id: window::Id) { - let runner = self.runner.clone(); + pub fn register(&self, canvas: &Rc>, id: window::Id) { + self.runner.add_canvas(WindowId(id), canvas); + let mut canvas = canvas.borrow_mut(); canvas.set_attribute("data-raw-handle", &id.0.to_string()); - runner.add_canvas(WindowId(id), canvas.raw().clone()); + let runner = self.runner.clone(); canvas.on_blur(move || { runner.send_event(Event::WindowEvent { window_id: WindowId(id), diff --git a/src/platform_impl/web/stdweb/canvas.rs b/src/platform_impl/web/stdweb/canvas.rs index 1fdc7182da..bc57bd59f6 100644 --- a/src/platform_impl/web/stdweb/canvas.rs +++ b/src/platform_impl/web/stdweb/canvas.rs @@ -306,4 +306,8 @@ impl Canvas { pub fn is_fullscreen(&self) -> bool { super::is_fullscreen(&self.raw) } + + pub fn remove_listeners(&mut self) { + // TODO: Stub, unimplemented (see web_sys for reference). + } } diff --git a/src/platform_impl/web/web_sys/canvas.rs b/src/platform_impl/web/web_sys/canvas.rs index bedfbf49c7..e86ba77fe5 100644 --- a/src/platform_impl/web/web_sys/canvas.rs +++ b/src/platform_impl/web/web_sys/canvas.rs @@ -281,6 +281,21 @@ impl Canvas { pub fn is_fullscreen(&self) -> bool { self.common.is_fullscreen() } + + pub fn remove_listeners(&mut self) { + self.on_focus = None; + self.on_blur = None; + self.on_keyboard_release = None; + self.on_keyboard_press = None; + self.on_received_character = None; + self.on_mouse_wheel = None; + self.on_fullscreen_change = None; + self.on_dark_mode = None; + match &mut self.mouse_state { + MouseState::HasPointerEvent(h) => h.remove_listeners(), + MouseState::NoPointerEvent(h) => h.remove_listeners(), + } + } } impl Common { diff --git a/src/platform_impl/web/web_sys/canvas/mouse_handler.rs b/src/platform_impl/web/web_sys/canvas/mouse_handler.rs index 2afd2ce36d..fcfb88b991 100644 --- a/src/platform_impl/web/web_sys/canvas/mouse_handler.rs +++ b/src/platform_impl/web/web_sys/canvas/mouse_handler.rs @@ -200,4 +200,13 @@ impl MouseHandler { }, )); } + + pub fn remove_listeners(&mut self) { + self.on_mouse_leave = None; + self.on_mouse_enter = None; + self.on_mouse_move = None; + self.on_mouse_press = None; + self.on_mouse_release = None; + *self.on_mouse_leave_handler.borrow_mut() = None; + } } diff --git a/src/platform_impl/web/web_sys/canvas/pointer_handler.rs b/src/platform_impl/web/web_sys/canvas/pointer_handler.rs index b381c45e1b..4764e03c7d 100644 --- a/src/platform_impl/web/web_sys/canvas/pointer_handler.rs +++ b/src/platform_impl/web/web_sys/canvas/pointer_handler.rs @@ -100,4 +100,12 @@ impl PointerHandler { }, )); } + + pub fn remove_listeners(&mut self) { + self.on_cursor_leave = None; + self.on_cursor_enter = None; + self.on_cursor_move = None; + self.on_pointer_press = None; + self.on_pointer_release = None; + } } diff --git a/src/platform_impl/web/web_sys/scaling.rs b/src/platform_impl/web/web_sys/scaling.rs index 1925d5f97e..2129a165d7 100644 --- a/src/platform_impl/web/web_sys/scaling.rs +++ b/src/platform_impl/web/web_sys/scaling.rs @@ -36,9 +36,11 @@ impl ScaleChangeDetectorInternal { last_scale: current_scale, })); - let cloned_self = new_self.clone(); + let weak_self = Rc::downgrade(&new_self); let closure = Closure::wrap(Box::new(move |event: MediaQueryListEvent| { - cloned_self.borrow_mut().handler(event) + if let Some(rc_self) = weak_self.upgrade() { + rc_self.borrow_mut().handler(event); + } }) as Box); let mql = Self::create_mql(closure); diff --git a/src/platform_impl/web/window.rs b/src/platform_impl/web/window.rs index 2df4768560..586da040c1 100644 --- a/src/platform_impl/web/window.rs +++ b/src/platform_impl/web/window.rs @@ -8,12 +8,13 @@ use raw_window_handle::web::WebHandle; use super::{backend, monitor, EventLoopWindowTarget}; -use std::cell::RefCell; +use std::cell::{Ref, RefCell}; use std::collections::vec_deque::IntoIter as VecDequeIter; use std::collections::VecDeque; +use std::rc::Rc; pub struct Window { - canvas: backend::Canvas, + canvas: Rc>, previous_pointer: RefCell<&'static str>, id: Id, register_redraw_request: Box, @@ -30,7 +31,8 @@ impl Window { let id = target.generate_id(); - let mut canvas = backend::Canvas::create(platform_attr)?; + let canvas = backend::Canvas::create(platform_attr)?; + let mut canvas = Rc::new(RefCell::new(canvas)); let register_redraw_request = Box::new(move || runner.request_redraw(RootWI(id))); @@ -59,12 +61,12 @@ impl Window { Ok(window) } - pub fn canvas(&self) -> &backend::Canvas { - &self.canvas + pub fn canvas<'a>(&'a self) -> Ref<'a, backend::Canvas> { + self.canvas.borrow() } pub fn set_title(&self, title: &str) { - self.canvas.set_attribute("alt", title); + self.canvas.borrow().set_attribute("alt", title); } pub fn set_visible(&self, _visible: bool) { @@ -76,7 +78,11 @@ impl Window { } pub fn outer_position(&self) -> Result, NotSupportedError> { - Ok(self.canvas.position().to_physical(self.scale_factor())) + Ok(self + .canvas + .borrow() + .position() + .to_physical(self.scale_factor())) } pub fn inner_position(&self) -> Result, NotSupportedError> { @@ -87,14 +93,15 @@ impl Window { pub fn set_outer_position(&self, position: Position) { let position = position.to_logical::(self.scale_factor()); - self.canvas.set_attribute("position", "fixed"); - self.canvas.set_attribute("left", &position.x.to_string()); - self.canvas.set_attribute("top", &position.y.to_string()); + let canvas = self.canvas.borrow(); + canvas.set_attribute("position", "fixed"); + canvas.set_attribute("left", &position.x.to_string()); + canvas.set_attribute("top", &position.y.to_string()); } #[inline] pub fn inner_size(&self) -> PhysicalSize { - self.canvas.size() + self.canvas.borrow().size() } #[inline] @@ -105,7 +112,7 @@ impl Window { #[inline] pub fn set_inner_size(&self, size: Size) { - backend::set_canvas_size(self.canvas.raw(), size); + backend::set_canvas_size(self.canvas.borrow().raw(), size); } #[inline] @@ -170,7 +177,7 @@ impl Window { CursorIcon::RowResize => "row-resize", }; *self.previous_pointer.borrow_mut() = text; - backend::set_canvas_style_property(self.canvas.raw(), "cursor", text); + backend::set_canvas_style_property(self.canvas.borrow().raw(), "cursor", text); } #[inline] @@ -186,9 +193,10 @@ impl Window { #[inline] pub fn set_cursor_visible(&self, visible: bool) { if !visible { - self.canvas.set_attribute("cursor", "none"); + self.canvas.borrow().set_attribute("cursor", "none"); } else { self.canvas + .borrow() .set_attribute("cursor", *self.previous_pointer.borrow()); } } @@ -205,7 +213,7 @@ impl Window { #[inline] pub fn fullscreen(&self) -> Option { - if self.canvas.is_fullscreen() { + if self.canvas.borrow().is_fullscreen() { Some(Fullscreen::Borderless(self.current_monitor_inner())) } else { None @@ -215,8 +223,8 @@ impl Window { #[inline] pub fn set_fullscreen(&self, monitor: Option) { if monitor.is_some() { - self.canvas.request_fullscreen(); - } else if self.canvas.is_fullscreen() { + self.canvas.borrow().request_fullscreen(); + } else if self.canvas.borrow().is_fullscreen() { backend::exit_fullscreen(); } } From 15a112f23c6bbc335fadd9afefe303d3613e421d Mon Sep 17 00:00:00 2001 From: Alvin Wong Date: Mon, 21 Sep 2020 02:05:06 +0800 Subject: [PATCH 6/7] web: Do not remove canvas from DOM when dropping Window The canvas was inserted by the user, so it should be up to the user whether the canvas should be removed. --- src/platform_impl/web/stdweb/canvas.rs | 6 ------ src/platform_impl/web/web_sys/canvas.rs | 6 ------ 2 files changed, 12 deletions(-) diff --git a/src/platform_impl/web/stdweb/canvas.rs b/src/platform_impl/web/stdweb/canvas.rs index bc57bd59f6..b9e0c9d015 100644 --- a/src/platform_impl/web/stdweb/canvas.rs +++ b/src/platform_impl/web/stdweb/canvas.rs @@ -37,12 +37,6 @@ pub struct Canvas { wants_fullscreen: Rc>, } -impl Drop for Canvas { - fn drop(&mut self) { - self.raw.remove(); - } -} - impl Canvas { pub fn create(attr: PlatformSpecificWindowBuilderAttributes) -> Result { let canvas = match attr.canvas { diff --git a/src/platform_impl/web/web_sys/canvas.rs b/src/platform_impl/web/web_sys/canvas.rs index e86ba77fe5..46d9531157 100644 --- a/src/platform_impl/web/web_sys/canvas.rs +++ b/src/platform_impl/web/web_sys/canvas.rs @@ -37,12 +37,6 @@ struct Common { wants_fullscreen: Rc>, } -impl Drop for Common { - fn drop(&mut self) { - self.raw.remove(); - } -} - impl Canvas { pub fn create(attr: PlatformSpecificWindowBuilderAttributes) -> Result { let canvas = match attr.canvas { From 05a451cd6f8d6c6637a337fc760c5b7e45ae7c8d Mon Sep 17 00:00:00 2001 From: Alvin Wong Date: Mon, 21 Sep 2020 02:22:49 +0800 Subject: [PATCH 7/7] Update changelog --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0cc84b1aa5..547af0329c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,11 @@ - On Android, bump `ndk`, `ndk-sys` and `ndk-glue` to 0.2. Checkout the new ndk-glue main proc attribute. - **Breaking:** Prefixed virtual key codes `Add`, `Multiply`, `Divide`, `Decimal`, and `Subtract` with `Numpad`. - Added `Asterisk` and `Plus` virtual key codes. +- On Web (web-sys only), the `Event::LoopDestroyed` event is correctly emitted when leaving the page. +- On Web, the `WindowEvent::Destroyed` event now gets emitted when a `Window` is dropped. +- On Web (web-sys only), the event listeners are now removed when a `Window` is dropped or when the event loop is destroyed. +- On Web, the event handler closure passed to `EventLoop::run` now gets dropped after the event loop is destroyed. +- **Breaking:** On Web, the canvas element associated to a `Window` is no longer removed from the DOM when the `Window` is dropped. # 0.22.2 (2020-05-16)