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 crash on request_animation_frame when destroying web runner #4169

Merged
merged 4 commits into from
Mar 14, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions crates/eframe/src/web/app_runner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -165,8 +165,8 @@ impl AppRunner {
self.last_save_time = now_sec();
}

pub fn canvas_id(&self) -> &str {
self.painter.canvas_id()
pub fn canvas(&self) -> web_sys::HtmlCanvasElement {
self.painter.canvas()
}

pub fn destroy(mut self) {
Expand All @@ -182,8 +182,8 @@ impl AppRunner {
///
/// The result can be painted later with a call to [`Self::run_and_paint`] or [`Self::paint`].
pub fn logic(&mut self) {
super::resize_canvas_to_screen_size(self.canvas_id(), self.web_options.max_size_points);
let canvas_size = super::canvas_size_in_points(self.canvas_id());
super::resize_canvas_to_screen_size(&self.canvas(), self.web_options.max_size_points);
let canvas_size = super::canvas_size_in_points(&self.canvas());
let raw_input = self.input.new_frame(canvas_size);

let full_output = self.egui_ctx.run(raw_input, |egui_ctx| {
Expand Down Expand Up @@ -268,7 +268,7 @@ impl AppRunner {
self.mutable_text_under_cursor = mutable_text_under_cursor;

if self.ime != ime {
super::text_agent::move_text_cursor(ime, self.canvas_id());
super::text_agent::move_text_cursor(ime, &self.canvas());
self.ime = ime;
}
}
Expand Down
26 changes: 9 additions & 17 deletions crates/eframe/src/web/events.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,12 @@ use super::*;
/// Calls `request_animation_frame` to schedule repaint.
///
/// It will only paint if needed, but will always call `request_animation_frame` immediately.
fn paint_and_schedule(runner_ref: &WebRunner) -> Result<(), JsValue> {
pub(crate) fn paint_and_schedule(runner_ref: &WebRunner) -> Result<(), JsValue> {
// Only paint and schedule if there has been no panic
if let Some(mut runner_lock) = runner_ref.try_lock() {
paint_if_needed(&mut runner_lock);
drop(runner_lock);
request_animation_frame(runner_ref.clone())?;
runner_ref.request_animation_frame()?;
}
Ok(())
}
Expand Down Expand Up @@ -45,14 +45,6 @@ fn paint_if_needed(runner: &mut AppRunner) {
runner.auto_save_if_needed();
}

pub(crate) fn request_animation_frame(runner_ref: WebRunner) -> Result<(), JsValue> {
let window = web_sys::window().unwrap();
let closure = Closure::once(move || paint_and_schedule(&runner_ref));
window.request_animation_frame(closure.as_ref().unchecked_ref())?;
closure.forget(); // We must forget it, or else the callback is canceled on drop
Ok(())
}

// ------------------------------------------------------------------------

pub(crate) fn install_document_events(runner_ref: &WebRunner) -> Result<(), JsValue> {
Expand Down Expand Up @@ -275,7 +267,7 @@ pub(crate) fn install_color_scheme_change_event(runner_ref: &WebRunner) -> Resul
}

pub(crate) fn install_canvas_events(runner_ref: &WebRunner) -> Result<(), JsValue> {
let canvas = canvas_element(runner_ref.try_lock().unwrap().canvas_id()).unwrap();
let canvas = runner_ref.try_lock().unwrap().canvas();

{
let prevent_default_events = [
Expand Down Expand Up @@ -304,7 +296,7 @@ pub(crate) fn install_canvas_events(runner_ref: &WebRunner) -> Result<(), JsValu
let modifiers = modifiers_from_mouse_event(&event);
runner.input.raw.modifiers = modifiers;
if let Some(button) = button_from_mouse_event(&event) {
let pos = pos_from_mouse_event(runner.canvas_id(), &event);
let pos = pos_from_mouse_event(&runner.canvas(), &event);
let modifiers = runner.input.raw.modifiers;
runner.input.raw.events.push(egui::Event::PointerButton {
pos,
Expand All @@ -331,7 +323,7 @@ pub(crate) fn install_canvas_events(runner_ref: &WebRunner) -> Result<(), JsValu
|event: web_sys::MouseEvent, runner| {
let modifiers = modifiers_from_mouse_event(&event);
runner.input.raw.modifiers = modifiers;
let pos = pos_from_mouse_event(runner.canvas_id(), &event);
let pos = pos_from_mouse_event(&runner.canvas(), &event);
runner.input.raw.events.push(egui::Event::PointerMoved(pos));
runner.needs_repaint.repaint_asap();
event.stop_propagation();
Expand All @@ -343,7 +335,7 @@ pub(crate) fn install_canvas_events(runner_ref: &WebRunner) -> Result<(), JsValu
let modifiers = modifiers_from_mouse_event(&event);
runner.input.raw.modifiers = modifiers;
if let Some(button) = button_from_mouse_event(&event) {
let pos = pos_from_mouse_event(runner.canvas_id(), &event);
let pos = pos_from_mouse_event(&runner.canvas(), &event);
let modifiers = runner.input.raw.modifiers;
runner.input.raw.events.push(egui::Event::PointerButton {
pos,
Expand Down Expand Up @@ -381,7 +373,7 @@ pub(crate) fn install_canvas_events(runner_ref: &WebRunner) -> Result<(), JsValu
"touchstart",
|event: web_sys::TouchEvent, runner| {
let mut latest_touch_pos_id = runner.input.latest_touch_pos_id;
let pos = pos_from_touch_event(runner.canvas_id(), &event, &mut latest_touch_pos_id);
let pos = pos_from_touch_event(&runner.canvas(), &event, &mut latest_touch_pos_id);
runner.input.latest_touch_pos_id = latest_touch_pos_id;
runner.input.latest_touch_pos = Some(pos);
let modifiers = runner.input.raw.modifiers;
Expand All @@ -404,7 +396,7 @@ pub(crate) fn install_canvas_events(runner_ref: &WebRunner) -> Result<(), JsValu
"touchmove",
|event: web_sys::TouchEvent, runner| {
let mut latest_touch_pos_id = runner.input.latest_touch_pos_id;
let pos = pos_from_touch_event(runner.canvas_id(), &event, &mut latest_touch_pos_id);
let pos = pos_from_touch_event(&runner.canvas(), &event, &mut latest_touch_pos_id);
runner.input.latest_touch_pos_id = latest_touch_pos_id;
runner.input.latest_touch_pos = Some(pos);
runner.input.raw.events.push(egui::Event::PointerMoved(pos));
Expand Down Expand Up @@ -467,7 +459,7 @@ pub(crate) fn install_canvas_events(runner_ref: &WebRunner) -> Result<(), JsValu
});

let scroll_multiplier = match unit {
egui::MouseWheelUnit::Page => canvas_size_in_points(runner.canvas_id()).y,
egui::MouseWheelUnit::Page => canvas_size_in_points(&runner.canvas()).y,
egui::MouseWheelUnit::Line => {
#[allow(clippy::let_and_return)]
let points_per_scroll_line = 8.0; // Note that this is intentionally different from what we use in winit.
Expand Down
14 changes: 8 additions & 6 deletions crates/eframe/src/web/input.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
use super::{canvas_element, canvas_origin, AppRunner};
use super::{canvas_origin, AppRunner};

pub fn pos_from_mouse_event(canvas_id: &str, event: &web_sys::MouseEvent) -> egui::Pos2 {
let canvas = canvas_element(canvas_id).unwrap();
pub fn pos_from_mouse_event(
canvas: &web_sys::HtmlCanvasElement,
event: &web_sys::MouseEvent,
) -> egui::Pos2 {
let rect = canvas.get_bounding_client_rect();
egui::Pos2 {
x: event.client_x() as f32 - rect.left() as f32,
Expand All @@ -27,7 +29,7 @@ pub fn button_from_mouse_event(event: &web_sys::MouseEvent) -> Option<egui::Poin
/// `touch_id_for_pos` is the [`TouchId`](egui::TouchId) of the [`Touch`](web_sys::Touch) we previously used to determine the
/// pointer position.
pub fn pos_from_touch_event(
canvas_id: &str,
canvas: &web_sys::HtmlCanvasElement,
event: &web_sys::TouchEvent,
touch_id_for_pos: &mut Option<egui::TouchId>,
) -> egui::Pos2 {
Expand All @@ -47,7 +49,7 @@ pub fn pos_from_touch_event(
.or_else(|| event.touches().get(0))
.map_or(Default::default(), |touch| {
*touch_id_for_pos = Some(egui::TouchId::from(touch.identifier()));
pos_from_touch(canvas_origin(canvas_id), &touch)
pos_from_touch(canvas_origin(canvas), &touch)
})
}

Expand All @@ -59,7 +61,7 @@ fn pos_from_touch(canvas_origin: egui::Pos2, touch: &web_sys::Touch) -> egui::Po
}

pub fn push_touches(runner: &mut AppRunner, phase: egui::TouchPhase, event: &web_sys::TouchEvent) {
let canvas_origin = canvas_origin(runner.canvas_id());
let canvas_origin = canvas_origin(&runner.canvas());
for touch_idx in 0..event.changed_touches().length() {
if let Some(touch) = event.changed_touches().item(touch_idx) {
runner.input.raw.events.push(egui::Event::Touch {
Expand Down
21 changes: 10 additions & 11 deletions crates/eframe/src/web/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -100,35 +100,34 @@ fn theme_from_dark_mode(dark_mode: bool) -> Theme {
}
}

fn canvas_element(canvas_id: &str) -> Option<web_sys::HtmlCanvasElement> {
fn get_canvas_element_by_id(canvas_id: &str) -> Option<web_sys::HtmlCanvasElement> {
let document = web_sys::window()?.document()?;
let canvas = document.get_element_by_id(canvas_id)?;
canvas.dyn_into::<web_sys::HtmlCanvasElement>().ok()
}

fn canvas_element_or_die(canvas_id: &str) -> web_sys::HtmlCanvasElement {
canvas_element(canvas_id)
fn get_canvas_element_by_id_or_die(canvas_id: &str) -> web_sys::HtmlCanvasElement {
get_canvas_element_by_id(canvas_id)
.unwrap_or_else(|| panic!("Failed to find canvas with id {canvas_id:?}"))
}

fn canvas_origin(canvas_id: &str) -> egui::Pos2 {
let rect = canvas_element(canvas_id)
.unwrap()
.get_bounding_client_rect();
fn canvas_origin(canvas: &web_sys::HtmlCanvasElement) -> egui::Pos2 {
let rect = canvas.get_bounding_client_rect();
egui::pos2(rect.left() as f32, rect.top() as f32)
}

fn canvas_size_in_points(canvas_id: &str) -> egui::Vec2 {
let canvas = canvas_element(canvas_id).unwrap();
fn canvas_size_in_points(canvas: &web_sys::HtmlCanvasElement) -> egui::Vec2 {
let pixels_per_point = native_pixels_per_point();
egui::vec2(
canvas.width() as f32 / pixels_per_point,
canvas.height() as f32 / pixels_per_point,
)
}

fn resize_canvas_to_screen_size(canvas_id: &str, max_size_points: egui::Vec2) -> Option<()> {
let canvas = canvas_element(canvas_id)?;
fn resize_canvas_to_screen_size(
canvas: &web_sys::HtmlCanvasElement,
max_size_points: egui::Vec2,
) -> Option<()> {
let parent = canvas.parent_element()?;

// Prefer the client width and height so that if the parent
Expand Down
10 changes: 6 additions & 4 deletions crates/eframe/src/web/text_agent.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use std::{cell::Cell, rc::Rc};

use wasm_bindgen::prelude::*;

use super::{canvas_element, AppRunner, WebRunner};
use super::{AppRunner, WebRunner};

static AGENT_ID: &str = "egui_text_agent";

Expand Down Expand Up @@ -121,7 +121,7 @@ pub fn update_text_agent(runner: &mut AppRunner) -> Option<()> {
let window = web_sys::window()?;
let document = window.document()?;
let input: HtmlInputElement = document.get_element_by_id(AGENT_ID)?.dyn_into().unwrap();
let canvas_style = canvas_element(runner.canvas_id())?.style();
let canvas_style = runner.canvas().style();

if runner.mutable_text_under_cursor {
let is_already_editing = input.hidden();
Expand Down Expand Up @@ -205,14 +205,16 @@ fn is_mobile() -> Option<bool> {
// candidate window moves following text element (agent),
// so it appears that the IME candidate window moves with text cursor.
// On mobile devices, there is no need to do that.
pub fn move_text_cursor(ime: Option<egui::output::IMEOutput>, canvas_id: &str) -> Option<()> {
pub fn move_text_cursor(
ime: Option<egui::output::IMEOutput>,
canvas: &web_sys::HtmlCanvasElement,
) -> Option<()> {
let style = text_agent().style();
// Note: moving agent on mobile devices will lead to unpredictable scroll.
if is_mobile() == Some(false) {
ime.as_ref().and_then(|ime| {
let egui::Pos2 { x, y } = ime.cursor_rect.left_top();

let canvas = canvas_element(canvas_id)?;
let bounding_rect = text_agent().get_bounding_client_rect();
let y = (y + (canvas.scroll_top() + canvas.offset_top()) as f32)
.min(canvas.client_height() as f32 - bounding_rect.height() as f32);
Expand Down
4 changes: 2 additions & 2 deletions crates/eframe/src/web/web_painter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ pub(crate) trait WebPainter {
// where
// Self: Sized;

/// Id of the canvas in use.
fn canvas_id(&self) -> &str;
/// Reference to the canvas in use.
fn canvas(&self) -> web_sys::HtmlCanvasElement;

/// Maximum size of a texture in one direction.
fn max_texture_side(&self) -> usize;
Expand Down
13 changes: 4 additions & 9 deletions crates/eframe/src/web/web_painter_glow.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ use super::web_painter::WebPainter;

pub(crate) struct WebPainterGlow {
canvas: HtmlCanvasElement,
canvas_id: String,
painter: egui_glow::Painter,
}

Expand All @@ -20,7 +19,7 @@ impl WebPainterGlow {
}

pub async fn new(canvas_id: &str, options: &WebOptions) -> Result<Self, String> {
let canvas = super::canvas_element_or_die(canvas_id);
let canvas = super::get_canvas_element_by_id_or_die(canvas_id);

let (gl, shader_prefix) =
init_glow_context_from_canvas(&canvas, options.webgl_context_option)?;
Expand All @@ -30,11 +29,7 @@ impl WebPainterGlow {
let painter = egui_glow::Painter::new(gl, shader_prefix, None)
.map_err(|err| format!("Error starting glow painter: {err}"))?;

Ok(Self {
canvas,
canvas_id: canvas_id.to_owned(),
painter,
})
Ok(Self { canvas, painter })
}
}

Expand All @@ -43,8 +38,8 @@ impl WebPainter for WebPainterGlow {
self.painter.max_texture_side()
}

fn canvas_id(&self) -> &str {
&self.canvas_id
fn canvas(&self) -> HtmlCanvasElement {
self.canvas.clone()
}

fn paint_and_update_textures(
Expand Down
8 changes: 3 additions & 5 deletions crates/eframe/src/web/web_painter_wgpu.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ impl HasDisplayHandle for EguiWebWindow {

pub(crate) struct WebPainterWgpu {
canvas: HtmlCanvasElement,
canvas_id: String,
surface: wgpu::Surface<'static>,
surface_configuration: wgpu::SurfaceConfiguration,
render_state: Option<RenderState>,
Expand Down Expand Up @@ -163,7 +162,7 @@ impl WebPainterWgpu {
}
}

let canvas = super::canvas_element_or_die(canvas_id);
let canvas = super::get_canvas_element_by_id_or_die(canvas_id);
let surface = instance
.create_surface(wgpu::SurfaceTarget::Canvas(canvas.clone()))
.map_err(|err| format!("failed to create wgpu surface: {err}"))?;
Expand All @@ -188,7 +187,6 @@ impl WebPainterWgpu {

Ok(Self {
canvas,
canvas_id: canvas_id.to_owned(),
render_state: Some(render_state),
surface,
surface_configuration,
Expand All @@ -200,8 +198,8 @@ impl WebPainterWgpu {
}

impl WebPainter for WebPainterWgpu {
fn canvas_id(&self) -> &str {
&self.canvas_id
fn canvas(&self) -> HtmlCanvasElement {
self.canvas.clone()
}

fn max_texture_side(&self) -> usize {
Expand Down
Loading
Loading