script: Ensure that leaving the WebView sets the cursor back to the default cursor (#38759)

This changes makes a variety of changes to ensure that the cursor is set
back to the default cursor when it leaves the `WebView`:

1. Display list updates can come after a mouse leaves the `WebView`, so
   when refreshing the cursor after the update, base the updated cursor
   on the last hovered location in the `DocumentEventHandler`, rather
   than the compositor. This allows us to catch when the last hovered
   position is `None` (ie the cursor has left the `WebView`).
2. When handling `MouseLeftViewport` events for the cursor leaving the
   entire WebView, properly set the
   MouseLeftViewport::focus_moving_to_another_iframe` on the input event
   passed to the script thread.
3. When moving out of the `WebView` entirely, explicitly ask the
   embedder to set the cursor back to the default.

Testing: This change adds a unit test verifying this behavior.
Fixes: #38710.

Signed-off-by: Martin Robinson <mrobinson@igalia.com>

components/compositing/compositor.rs

@@ -1601,7 +1601,6 @@ impl IOCompositor {
                 .constellation_sender
                 .send(EmbedderToConstellationMessage::RefreshCursor(
                     hit_test_result.pipeline_id,
-                    hit_test_result.point_in_viewport,
                 ))
         {
             warn!("Sending event to constellation failed ({:?}).", error);

components/constellation/constellation.rs

@@ -136,8 +136,8 @@ use embedder_traits::{
     MouseButton, MouseButtonAction, MouseButtonEvent, Theme, ViewportDetails, WebDriverCommandMsg,
     WebDriverCommandResponse, WebDriverLoadStatus, WebDriverScriptCommand,
 };
+use euclid::Size2D;
 use euclid::default::Size2D as UntypedSize2D;
-use euclid::{Point2D, Size2D};
 use fonts::SystemFontServiceProxy;
 use ipc_channel::Error as IpcError;
 use ipc_channel::ipc::{self, IpcReceiver, IpcSender};
@@ -164,7 +164,6 @@ use servo_config::prefs::{self, PrefValue};
 use servo_config::{opts, pref};
 use servo_rand::{Rng, ServoRng, SliceRandom, random};
 use servo_url::{Host, ImmutableOrigin, ServoUrl};
-use style_traits::CSSPixel;
 #[cfg(feature = "webgpu")]
 use webgpu::swapchain::WGPUImageMap;
 #[cfg(feature = "webgpu")]
@@ -1465,8 +1464,8 @@ where
             EmbedderToConstellationMessage::ForwardInputEvent(webview_id, event, hit_test) => {
                 self.forward_input_event(webview_id, event, hit_test);
             },
-            EmbedderToConstellationMessage::RefreshCursor(pipeline_id, point) => {
-                self.handle_refresh_cursor(pipeline_id, point)
+            EmbedderToConstellationMessage::RefreshCursor(pipeline_id) => {
+                self.handle_refresh_cursor(pipeline_id)
             },
             EmbedderToConstellationMessage::ToggleProfiler(rate, max_duration) => {
                 for background_monitor_control_sender in &self.background_monitor_control_senders {
@@ -3440,14 +3439,14 @@ where
     }
 
     #[servo_tracing::instrument(skip_all)]
-    fn handle_refresh_cursor(&self, pipeline_id: PipelineId, point: Point2D<f32, CSSPixel>) {
+    fn handle_refresh_cursor(&self, pipeline_id: PipelineId) {
         let Some(pipeline) = self.pipelines.get(&pipeline_id) else {
             return;
         };
 
         if let Err(error) = pipeline
             .event_loop
-            .send(ScriptThreadMessage::RefreshCursor(pipeline_id, point))
+            .send(ScriptThreadMessage::RefreshCursor(pipeline_id))
         {
             warn!("Could not send RefreshCursor message to pipeline: {error:?}");
         }

components/constellation/constellation_webview.rs

@@ -98,45 +98,46 @@ impl ConstellationWebView {
             return;
         };
 
-        let mut update_hovered_browsing_context = |newly_hovered_browsing_context_id| {
-            let old_hovered_context_id = std::mem::replace(
-                &mut self.hovered_browsing_context_id,
-                newly_hovered_browsing_context_id,
-            );
-            if old_hovered_context_id == newly_hovered_browsing_context_id {
-                return;
-            }
-            let Some(old_hovered_context_id) = old_hovered_context_id else {
-                return;
-            };
-            let Some(pipeline) = browsing_contexts
-                .get(&old_hovered_context_id)
-                .and_then(|browsing_context| pipelines.get(&browsing_context.pipeline_id))
-            else {
-                return;
-            };
+        let mut update_hovered_browsing_context =
+            |newly_hovered_browsing_context_id, focus_moving_to_another_iframe: bool| {
+                let old_hovered_context_id = std::mem::replace(
+                    &mut self.hovered_browsing_context_id,
+                    newly_hovered_browsing_context_id,
+                );
+                if old_hovered_context_id == newly_hovered_browsing_context_id {
+                    return;
+                }
+                let Some(old_hovered_context_id) = old_hovered_context_id else {
+                    return;
+                };
+                let Some(pipeline) = browsing_contexts
+                    .get(&old_hovered_context_id)
+                    .and_then(|browsing_context| pipelines.get(&browsing_context.pipeline_id))
+                else {
+                    return;
+                };
 
-            let mut synthetic_mouse_leave_event = event.clone();
-            synthetic_mouse_leave_event.event =
-                InputEvent::MouseLeftViewport(MouseLeftViewportEvent {
-                    focus_moving_to_another_iframe: true,
-                });
+                let mut synthetic_mouse_leave_event = event.clone();
+                synthetic_mouse_leave_event.event =
+                    InputEvent::MouseLeftViewport(MouseLeftViewportEvent {
+                        focus_moving_to_another_iframe,
+                    });
 
-            let _ = pipeline
-                .event_loop
-                .send(ScriptThreadMessage::SendInputEvent(
-                    pipeline.id,
-                    synthetic_mouse_leave_event,
-                ));
-        };
+                let _ = pipeline
+                    .event_loop
+                    .send(ScriptThreadMessage::SendInputEvent(
+                        pipeline.id,
+                        synthetic_mouse_leave_event,
+                    ));
+            };
 
         if let InputEvent::MouseLeftViewport(_) = &event.event {
-            update_hovered_browsing_context(None);
+            update_hovered_browsing_context(None, false);
             return;
         }
 
         if let InputEvent::MouseMove(_) = &event.event {
-            update_hovered_browsing_context(Some(pipeline.browsing_context_id));
+            update_hovered_browsing_context(Some(pipeline.browsing_context_id), true);
             self.last_mouse_move_point = event
                 .hit_test_result
                 .as_ref()

components/script/dom/document_event_handler.rs

@@ -79,7 +79,7 @@ pub(crate) struct DocumentEventHandler {
     current_hover_target: MutNullableDom<Element>,
     /// The most recent mouse movement point, used for processing `mouseleave` events.
     #[no_trace]
-    most_recent_mousemove_point: Point2D<f32, CSSPixel>,
+    most_recent_mousemove_point: Cell<Option<Point2D<f32, CSSPixel>>>,
     /// The currently set [`Cursor`] or `None` if the `Document` isn't being hovered
     /// by the cursor.
     #[no_trace]
@@ -212,13 +212,15 @@ impl DocumentEventHandler {
             }));
     }
 
-    pub(crate) fn set_cursor(&self, cursor: Cursor) {
-        if Some(cursor) == self.current_cursor.get() {
+    pub(crate) fn set_cursor(&self, cursor: Option<Cursor>) {
+        if cursor == self.current_cursor.get() {
             return;
         }
-        self.current_cursor.set(Some(cursor));
-        self.window
-            .send_to_embedder(EmbedderMsg::SetCursor(self.window.webview_id(), cursor));
+        self.current_cursor.set(cursor);
+        self.window.send_to_embedder(EmbedderMsg::SetCursor(
+            self.window.webview_id(),
+            cursor.unwrap_or_default(),
+        ));
     }
 
     fn handle_mouse_left_viewport_event(
@@ -238,8 +240,9 @@ impl DocumentEventHandler {
             }
 
             if let Some(hit_test_result) = self
-                .window
-                .hit_test_from_point_in_viewport(self.most_recent_mousemove_point)
+                .most_recent_mousemove_point
+                .get()
+                .and_then(|point| self.window.hit_test_from_point_in_viewport(point))
             {
                 MouseEvent::new_simple(
                     &self.window,
@@ -263,15 +266,23 @@ impl DocumentEventHandler {
             }
         }
 
-        self.current_cursor.set(None);
-        self.current_hover_target.set(None);
-
-        // If focus is moving to another frame, it will decide what the new status text is, but if
-        // this mouse leave event is leaving the WebView entirely, then clear it.
+        // We do not want to always inform the embedder that cursor has been set to the
+        // default cursor, in order to avoid a timing issue when moving between `<iframe>`
+        // elements. There is currently no way to control which `SetCursor` message will
+        // reach the embedder first. This is safer when leaving the `WebView` entirely.
         if !mouse_leave_event.focus_moving_to_another_iframe {
+            // If focus is moving to another frame, it will decide what the new status
+            // text is, but if this mouse leave event is leaving the WebView entirely,
+            // then clear it.
             self.window
                 .send_to_embedder(EmbedderMsg::Status(self.window.webview_id(), None));
+            self.set_cursor(None);
+        } else {
+            self.current_cursor.set(None);
         }
+
+        self.current_hover_target.set(None);
+        self.most_recent_mousemove_point.set(None);
     }
 
     fn handle_mouse_enter_leave_event(
@@ -336,7 +347,7 @@ impl DocumentEventHandler {
         };
 
         // Update the cursor when the mouse moves, if it has changed.
-        self.set_cursor(hit_test_result.cursor);
+        self.set_cursor(Some(hit_test_result.cursor));
 
         let Some(new_target) = hit_test_result
             .node
@@ -451,6 +462,8 @@ impl DocumentEventHandler {
         .fire(new_target.upcast(), can_gc);
 
         self.update_current_hover_target_and_status(Some(new_target));
+        self.most_recent_mousemove_point
+            .set(Some(hit_test_result.point_in_frame));
     }
 
     fn update_current_hover_target_and_status(&self, new_hover_target: Option<DomRoot<Element>>) {
@@ -502,6 +515,21 @@ impl DocumentEventHandler {
         }
     }
 
+    pub(crate) fn handle_refresh_cursor(&self) {
+        let Some(most_recent_mousemove_point) = self.most_recent_mousemove_point.get() else {
+            return;
+        };
+
+        let Some(hit_test_result) = self
+            .window
+            .hit_test_from_point_in_viewport(most_recent_mousemove_point)
+        else {
+            return;
+        };
+
+        self.set_cursor(Some(hit_test_result.cursor));
+    }
+
     /// <https://w3c.github.io/uievents/#mouseevent-algorithms>
     /// Handles native mouse down, mouse up, mouse click.
     fn handle_native_mouse_button_event(

components/script/dom/window.rs

@@ -3067,22 +3067,6 @@ impl Window {
             node.dirty(NodeDamage::Other);
         }
     }
-
-    pub fn handle_refresh_cursor(&self, cursor_position: Point2D<f32, CSSPixel>) {
-        let layout = self.layout.borrow();
-        layout.ensure_stacking_context_tree(self.viewport_details.get());
-        let Some(hit_test_result) = layout
-            .query_elements_from_point(cursor_position.cast_unit(), ElementsFromPointFlags::empty())
-            .into_iter()
-            .nth(0)
-        else {
-            return;
-        };
-
-        self.Document()
-            .event_handler()
-            .set_cursor(hit_test_result.cursor);
-    }
 }
 
 impl Window {

components/script/script_thread.rs

@@ -91,7 +91,6 @@ use script_traits::{
 use servo_config::{opts, prefs};
 use servo_url::{ImmutableOrigin, MutableOrigin, ServoUrl};
 use style::thread_state::{self, ThreadState};
-use style_traits::CSSPixel;
 use stylo_atoms::Atom;
 use timers::{TimerEventRequest, TimerId, TimerScheduler};
 use url::Position;
@@ -1884,8 +1883,8 @@ impl ScriptThread {
                     );
                 }
             },
-            ScriptThreadMessage::RefreshCursor(pipeline_id, cursor_position) => {
-                self.handle_refresh_cursor(pipeline_id, cursor_position);
+            ScriptThreadMessage::RefreshCursor(pipeline_id) => {
+                self.handle_refresh_cursor(pipeline_id);
             },
             ScriptThreadMessage::PreferencesUpdated(updates) => {
                 let mut current_preferences = prefs::get().clone();
@@ -3996,15 +3995,11 @@ impl ScriptThread {
         ));
     }
 
-    fn handle_refresh_cursor(
-        &self,
-        pipeline_id: PipelineId,
-        cursor_position: Point2D<f32, CSSPixel>,
-    ) {
-        let Some(window) = self.documents.borrow().find_window(pipeline_id) else {
+    fn handle_refresh_cursor(&self, pipeline_id: PipelineId) {
+        let Some(document) = self.documents.borrow().find_document(pipeline_id) else {
             return;
         };
-        window.handle_refresh_cursor(cursor_position);
+        document.event_handler().handle_refresh_cursor();
     }
 }
 

components/servo/tests/common/mod.rs

@@ -57,6 +57,8 @@ pub(crate) fn run_test(
 
 pub struct ServoTest {
     servo: Servo,
+    #[allow(dead_code)]
+    pub rendering_context: Rc<dyn RenderingContext>,
 }
 
 impl Drop for ServoTest {
@@ -100,7 +102,10 @@ impl ServoTest {
             .event_loop_waker(Box::new(EventLoopWakerImpl(user_event_triggered)));
         let builder = customize(builder);
         let servo = builder.build();
-        Self { servo }
+        Self {
+            servo,
+            rendering_context,
+        }
     }
 
     pub fn servo(&self) -> &Servo {
@@ -136,11 +141,15 @@ impl ServoTest {
 #[derive(Default)]
 pub(crate) struct WebViewDelegateImpl {
     pub(crate) url_changed: Cell<bool>,
+    pub(crate) cursor_changed: Cell<bool>,
+    pub(crate) new_frame_ready: Cell<bool>,
 }
 
 impl WebViewDelegateImpl {
     pub(crate) fn reset(&self) {
         self.url_changed.set(false);
+        self.cursor_changed.set(false);
+        self.new_frame_ready.set(false);
     }
 }
 
@@ -148,6 +157,15 @@ impl WebViewDelegate for WebViewDelegateImpl {
     fn notify_url_changed(&self, _webview: servo::WebView, _url: url::Url) {
         self.url_changed.set(true);
     }
+
+    fn notify_cursor_changed(&self, _webview: WebView, _: servo::Cursor) {
+        self.cursor_changed.set(true);
+    }
+
+    fn notify_new_frame_ready(&self, webview: WebView) {
+        self.new_frame_ready.set(true);
+        webview.paint();
+    }
 }
 
 pub(crate) fn evaluate_javascript(

components/servo/tests/webview.rs

@@ -15,7 +15,11 @@ use std::rc::Rc;
 
 use anyhow::ensure;
 use common::{ServoTest, WebViewDelegateImpl, evaluate_javascript, run_api_tests};
-use servo::{JSValue, JavaScriptEvaluationError, Theme, WebViewBuilder};
+use euclid::Point2D;
+use servo::{
+    Cursor, InputEvent, JSValue, JavaScriptEvaluationError, LoadStatus, MouseLeftViewportEvent,
+    MouseMoveEvent, Theme, WebViewBuilder,
+};
 use url::Url;
 
 fn test_create_webview(servo_test: &ServoTest) -> Result<(), anyhow::Error> {
@@ -141,9 +145,54 @@ fn test_theme_change(servo_test: &ServoTest) -> Result<(), anyhow::Error> {
     Ok(())
 }
 
+fn test_cursor_change(servo_test: &ServoTest) -> Result<(), anyhow::Error> {
+    let delegate = Rc::new(WebViewDelegateImpl::default());
+    let webview = WebViewBuilder::new(servo_test.servo())
+        .delegate(delegate.clone())
+        .url(
+            Url::parse(
+                "data:text/html,<!DOCTYPE html><style> html { cursor: crosshair; margin: 0}</style><body>hello</body>",
+            )
+            .unwrap(),
+        )
+        .build();
+
+    webview.focus();
+    webview.show(true);
+    webview.move_resize(servo_test.rendering_context.size2d().to_f32().into());
+
+    let load_webview = webview.clone();
+    let _ = servo_test.spin(move || Ok(load_webview.load_status() != LoadStatus::Complete));
+
+    // Wait for at least one frame after the load completes.
+    delegate.reset();
+    let captured_delegate = delegate.clone();
+    servo_test.spin(move || Ok(!captured_delegate.new_frame_ready.get()))?;
+
+    webview.notify_input_event(InputEvent::MouseMove(MouseMoveEvent::new(Point2D::new(
+        10., 10.,
+    ))));
+
+    let captured_delegate = delegate.clone();
+    servo_test.spin(move || Ok(!captured_delegate.cursor_changed.get()))?;
+    ensure!(webview.cursor() == Cursor::Crosshair);
+
+    delegate.reset();
+    webview.notify_input_event(InputEvent::MouseLeftViewport(
+        MouseLeftViewportEvent::default(),
+    ));
+
+    let captured_delegate = delegate.clone();
+    servo_test.spin(move || Ok(!captured_delegate.cursor_changed.get()))?;
+    ensure!(webview.cursor() == Cursor::Default);
+
+    Ok(())
+}
+
 fn main() {
     run_api_tests!(
         test_create_webview,
+        test_cursor_change,
         test_evaluate_javascript_basic,
         test_evaluate_javascript_panic,
         test_theme_change,

components/shared/constellation/lib.rs

@@ -22,7 +22,6 @@ use embedder_traits::{
     CompositorHitTestResult, FocusId, InputEvent, JavaScriptEvaluationId, MediaSessionActionType,
     Theme, TraversalId, ViewportDetails, WebDriverCommandMsg, WebDriverCommandResponse,
 };
-use euclid::Point2D;
 pub use from_script_message::*;
 use ipc_channel::ipc::IpcSender;
 use malloc_size_of_derive::MallocSizeOf;
@@ -32,7 +31,6 @@ use servo_config::prefs::PrefValue;
 use servo_url::{ImmutableOrigin, ServoUrl};
 pub use structured_data::*;
 use strum_macros::IntoStaticStr;
-use style_traits::CSSPixel;
 use webrender_api::units::LayoutVector2D;
 use webrender_api::{ExternalScrollId, ImageKey};
 
@@ -78,9 +76,10 @@ pub enum EmbedderToConstellationMessage {
     BlurWebView,
     /// Forward an input event to an appropriate ScriptTask.
     ForwardInputEvent(WebViewId, InputEvent, Option<CompositorHitTestResult>),
-    /// Request that the given pipeline do a hit test at the location and reset the
-    /// cursor accordingly. This happens after a display list update is rendered.
-    RefreshCursor(PipelineId, Point2D<f32, CSSPixel>),
+    /// Request that the given pipeline refresh the cursor by doing a hit test at the most
+    /// recently hovered cursor position and resetting the cursor. This happens after a
+    /// display list update is rendered.
+    RefreshCursor(PipelineId),
     /// Enable the sampling profiler, with a given sampling rate and max total sampling duration.
     ToggleProfiler(Duration, Duration),
     /// Request to exit from fullscreen mode

components/shared/embedder/lib.rs

@@ -55,9 +55,10 @@ pub enum ShutdownState {
 /// A cursor for the window. This is different from a CSS cursor (see
 /// `CursorKind`) in that it has no `Auto` value.
 #[repr(u8)]
-#[derive(Clone, Copy, Debug, Deserialize, Eq, MallocSizeOf, PartialEq, Serialize)]
+#[derive(Clone, Copy, Debug, Default, Deserialize, Eq, MallocSizeOf, PartialEq, Serialize)]
 pub enum Cursor {
     None,
+    #[default]
     Default,
     Pointer,
     ContextMenu,

components/shared/script/lib.rs

@@ -32,7 +32,7 @@ use embedder_traits::{
     CompositorHitTestResult, FocusSequenceNumber, InputEvent, JavaScriptEvaluationId,
     MediaSessionActionType, Theme, ViewportDetails, WebDriverScriptCommand,
 };
-use euclid::{Point2D, Rect, Scale, Size2D, UnknownUnit};
+use euclid::{Rect, Scale, Size2D, UnknownUnit};
 use ipc_channel::ipc::{IpcReceiver, IpcSender};
 use keyboard_types::Modifiers;
 use malloc_size_of_derive::MallocSizeOf;
@@ -147,9 +147,10 @@ pub enum ScriptThreadMessage {
     ExitScriptThread,
     /// Sends a DOM event.
     SendInputEvent(PipelineId, ConstellationInputEvent),
-    /// Ask that the given pipeline refreshes the cursor (after a display list render) based
-    /// on the hit test at the given point.
-    RefreshCursor(PipelineId, Point2D<f32, CSSPixel>),
+    /// Request that the given pipeline refresh the cursor by doing a hit test at the most
+    /// recently hovered cursor position and resetting the cursor. This happens after a
+    /// display list update is rendered.
+    RefreshCursor(PipelineId),
     /// Notifies script of the viewport.
     Viewport(PipelineId, Rect<f32, UnknownUnit>),
     /// Requests that the script thread immediately send the constellation the title of a pipeline.