webdriver: Use take_screenshot() API in Take (Element) Screenshot (#39587)

WPT tests require us to reliably take screenshots when test pages are
fully loaded and testing is complete, and the WPT runner uses
[test-wait.js](a2f551eb2d/tests/wpt/tests/tools/wptrunner/wptrunner/executors/test-wait.js)
to do this in userland. when testing Servo with [--product
servo](a2f551eb2d/tests/wpt/tests/tools/wptrunner/wptrunner/executors/executorservo.py),
we use servoshell’s --output option, which backs that up with more
reliable waiting in Servo. but when testing Servo with [--product
servodriver](a2f551eb2d/tests/wpt/tests/tools/wptrunner/wptrunner/executors/executorservodriver.py),
we use the WebDriver Take Screenshot action, which currently takes the
screenshot immediately. we think this might be a source of regressions.

this patch makes the WebDriver actions Take Screenshot and Take Element
Screenshot use the same new WebView::take_screenshot() API as
servoshell’s --output option, such that those actions now wait for [a
variety of
conditions](a2f551eb2d/components/servo/webview.rs (L596-L602))
that may affect test output. it’s not clear if this is
[conformant](https://w3c.github.io/webdriver/#screen-capture), so we may
want to refine this to only wait when running tests at some point. other
changes:

- we remove the retry loop where we try to take a screenshot every
second for up to 30 seconds
- we send the result as a image::RgbaImage over crossbeam without shared
memory (it’s not cross-process)
- we now handle the zero-sized element case directly in the WebDriver
server

Testing: This should fix some flaky tests.
Fixes: #36715.
Fixes: (partially) #39180.
Fixes: (partially) #34683.

Signed-off-by: Delan Azabani <dazabani@igalia.com>
Co-authored-by: Martin Robinson <mrobinson@igalia.com>

Cargo.lock

@@ -2384,6 +2384,7 @@ dependencies = [
  "euclid",
  "http 1.3.1",
  "hyper_serde",
+ "image",
  "ipc-channel",
  "keyboard-types",
  "log",

components/compositing/compositor.rs

@@ -28,11 +28,10 @@ use dpi::PhysicalSize;
 use embedder_traits::{
     CompositorHitTestResult, InputEvent, ScreenshotCaptureError, ShutdownState, ViewportDetails,
 };
-use euclid::{Point2D, Rect, Scale, Size2D, Transform3D};
+use euclid::{Point2D, Scale, Size2D, Transform3D};
 use image::RgbaImage;
 use ipc_channel::ipc::{self, IpcSharedMemory};
 use log::{debug, info, trace, warn};
-use pixels::{CorsStatus, ImageFrame, ImageMetadata, PixelFormat, RasterImage};
 use profile_traits::mem::{
     ProcessReports, ProfilerRegistration, Report, ReportKind, perform_memory_report,
 };
@@ -1184,58 +1183,6 @@ impl IOCompositor {
         self.needs_repaint.set(RepaintReason::empty());
     }
 
-    /// Render the WebRender scene to the shared memory, without updating other state of this
-    /// [`IOCompositor`]. If succesful return the output image in shared memory.
-    pub fn render_to_shared_memory(
-        &mut self,
-        webview_id: WebViewId,
-        page_rect: Option<Rect<f32, CSSPixel>>,
-    ) -> Option<RasterImage> {
-        self.render_inner();
-
-        let size = self.rendering_context.size2d().to_i32();
-        let rect = if let Some(rect) = page_rect {
-            let scale = self
-                .webview_renderers
-                .get(webview_id)
-                .map(WebViewRenderer::device_pixels_per_page_pixel)
-                .unwrap_or_else(|| Scale::new(1.0));
-            let rect = scale.transform_rect(&rect);
-
-            let x = rect.origin.x as i32;
-            // We need to convert to the bottom-left origin coordinate
-            // system used by OpenGL
-            // If dpi > 1, y can be computed to be -1 due to rounding issue, resulting in panic.
-            // https://github.com/servo/servo/issues/39306#issuecomment-3342204869
-            let y = 0.max((size.height as f32 - rect.origin.y - rect.size.height) as i32);
-            let w = rect.size.width as i32;
-            let h = rect.size.height as i32;
-
-            DeviceIntRect::from_origin_and_size(Point2D::new(x, y), Size2D::new(w, h))
-        } else {
-            DeviceIntRect::from_origin_and_size(Point2D::origin(), size)
-        };
-
-        self.rendering_context
-            .read_to_image(rect)
-            .map(|image| RasterImage {
-                metadata: ImageMetadata {
-                    width: image.width(),
-                    height: image.height(),
-                },
-                format: PixelFormat::RGBA8,
-                frames: vec![ImageFrame {
-                    delay: None,
-                    byte_range: 0..image.len(),
-                    width: image.width(),
-                    height: image.height(),
-                }],
-                bytes: ipc::IpcSharedMemory::from_bytes(&image),
-                id: None,
-                cors_status: CorsStatus::Safe,
-            })
-    }
-
     #[servo_tracing::instrument(skip_all)]
     fn render_inner(&mut self) {
         if let Err(err) = self.rendering_context.make_current() {
@@ -1584,6 +1531,16 @@ impl IOCompositor {
         }
     }
 
+    pub fn device_pixels_per_page_pixel(
+        &self,
+        webview_id: WebViewId,
+    ) -> Scale<f32, CSSPixel, DevicePixel> {
+        self.webview_renderers
+            .get(webview_id)
+            .map(WebViewRenderer::device_pixels_per_page_pixel)
+            .unwrap_or_default()
+    }
+
     fn webrender_document(&self) -> DocumentId {
         self.global.borrow().webrender_document
     }
@@ -1644,10 +1601,11 @@ impl IOCompositor {
     pub fn request_screenshot(
         &self,
         webview_id: WebViewId,
+        rect: Option<DeviceRect>,
         callback: Box<dyn FnOnce(Result<RgbaImage, ScreenshotCaptureError>) + 'static>,
     ) {
         self.screenshot_taker
-            .request_screenshot(webview_id, callback);
+            .request_screenshot(webview_id, rect, callback);
         let _ = self.global.borrow().constellation_sender.send(
             EmbedderToConstellationMessage::RequestScreenshotReadiness(webview_id),
         );

components/compositing/screenshot.rs

@@ -8,16 +8,19 @@ use std::rc::Rc;
 use base::Epoch;
 use base::id::{PipelineId, WebViewId};
 use embedder_traits::ScreenshotCaptureError;
+use euclid::{Point2D, Size2D};
 use image::RgbaImage;
 use rustc_hash::FxHashMap;
+use webrender_api::units::{DeviceIntRect, DeviceRect};
 
 use crate::IOCompositor;
 use crate::compositor::RepaintReason;
 
 pub(crate) struct ScreenshotRequest {
     webview_id: WebViewId,
-    phase: ScreenshotRequestPhase,
+    rect: Option<DeviceRect>,
     callback: Box<dyn FnOnce(Result<RgbaImage, ScreenshotCaptureError>) + 'static>,
+    phase: ScreenshotRequestPhase,
 }
 
 /// Screenshots requests happen in three phases:
@@ -63,12 +66,14 @@ impl ScreenshotTaker {
     pub(crate) fn request_screenshot(
         &self,
         webview_id: WebViewId,
+        rect: Option<DeviceRect>,
         callback: Box<dyn FnOnce(Result<RgbaImage, ScreenshotCaptureError>) + 'static>,
     ) {
         self.requests.borrow_mut().push(ScreenshotRequest {
             webview_id,
-            phase: ScreenshotRequestPhase::ConstellationRequest,
+            rect,
             callback,
+            phase: ScreenshotRequestPhase::ConstellationRequest,
         });
     }
 
@@ -175,9 +180,25 @@ impl ScreenshotTaker {
                     return None;
                 };
 
+                let viewport_rect = webview_renderer.rect.to_i32();
+                let viewport_size = viewport_rect.size();
+                let rect = screenshot_request.rect.map_or(viewport_rect, |rect| {
+                    // We need to convert to the bottom-left origin coordinate
+                    // system used by OpenGL
+                    // If dpi > 1, y can be computed to be -1 due to rounding issue, resulting in panic.
+                    // https://github.com/servo/servo/issues/39306#issuecomment-3342204869
+                    let x = rect.min.x as i32;
+                    let y = 0.max(
+                        (viewport_size.height as f32 - rect.min.y - rect.size().height) as i32,
+                    );
+                    let w = rect.size().width as i32;
+                    let h = rect.size().height as i32;
+
+                    DeviceIntRect::from_origin_and_size(Point2D::new(x, y), Size2D::new(w, h))
+                });
                 let result = renderer
                     .rendering_context()
-                    .read_to_image(webview_renderer.rect.to_i32())
+                    .read_to_image(rect)
                     .ok_or(ScreenshotCaptureError::CouldNotReadImage);
                 callback(result);
                 None

components/servo/lib.rs

@@ -82,13 +82,14 @@ use fonts::SystemFontService;
 use gaol::sandbox::{ChildSandbox, ChildSandboxMethods};
 pub use gleam::gl;
 use gleam::gl::RENDERER;
+pub use image::RgbaImage;
 use ipc_channel::ipc::{self, IpcSender};
 use javascript_evaluator::JavaScriptEvaluator;
 pub use keyboard_types::{
     Code, CompositionEvent, CompositionState, Key, KeyState, Location, Modifiers, NamedKey,
 };
 use layout::LayoutFactoryImpl;
-use log::{Log, Metadata, Record, debug, error, warn};
+use log::{Log, Metadata, Record, debug, warn};
 use media::{GlApi, NativeDisplay, WindowGLContext};
 use net::protocols::ProtocolRegistry;
 use net::resource_thread::new_resource_threads;
@@ -1059,30 +1060,9 @@ impl Servo {
     }
 
     pub fn execute_webdriver_command(&self, command: WebDriverCommandMsg) {
-        if let WebDriverCommandMsg::TakeScreenshot(webview_id, page_rect, response_sender) = command
-        {
-            if let Some(ref rect) = page_rect {
-                if rect.height() == 0.0 || rect.width() == 0.0 {
-                    error!("Taking screenshot of bounding box with zero area");
-                    if let Err(e) = response_sender.send(Err(())) {
-                        error!("Sending reply to create png failed {e:?}");
-                    }
-                    return;
-                }
-            }
-
-            let img = self
-                .compositor
-                .borrow_mut()
-                .render_to_shared_memory(webview_id, page_rect);
-            if let Err(e) = response_sender.send(Ok(img)) {
-                error!("Sending reply to create png failed ({:?}).", e);
-            }
-        } else {
         self.constellation_proxy
             .send(EmbedderToConstellationMessage::WebDriverCommand(command));
     }
-    }
 
     pub fn set_preference(&self, name: &str, value: PrefValue) {
         let mut preferences = prefs::get().clone();

components/servo/webview.rs

@@ -20,6 +20,7 @@ use embedder_traits::{
 use euclid::{Point2D, Scale, Size2D};
 use image::RgbaImage;
 use servo_geometry::DeviceIndependentPixel;
+use style_traits::CSSPixel;
 use url::Url;
 use webrender_api::ScrollLocation;
 use webrender_api::units::{DeviceIntPoint, DevicePixel, DeviceRect};
@@ -526,6 +527,13 @@ impl WebView {
             .set_pinch_zoom(self.id(), new_pinch_zoom);
     }
 
+    pub fn device_pixels_per_css_pixel(&self) -> Scale<f32, CSSPixel, DevicePixel> {
+        self.inner()
+            .compositor
+            .borrow()
+            .device_pixels_per_page_pixel(self.id())
+    }
+
     pub fn exit_fullscreen(&self) {
         self.inner()
             .constellation_proxy
@@ -589,9 +597,11 @@ impl WebView {
         );
     }
 
-    /// Asynchronously take a screenshot of the [`WebView`] contents. This method will
+    /// Asynchronously take a screenshot of the [`WebView`] contents, given a `rect` or the whole
-    /// wait until the [`WebView`] is ready before the screenshot is taken. This includes
+    /// viewport, if no `rect` is given.
-    /// waiting for:
+    ///
+    /// This method will wait until the [`WebView`] is ready before the screenshot is taken.
+    /// This includes waiting for:
     ///
     ///  - all frames to fire their `load` event.
     ///  - all render blocking elements, such as stylesheets included via the `<link>`
@@ -606,12 +616,13 @@ impl WebView {
     /// operation.
     pub fn take_screenshot(
         &self,
+        rect: Option<DeviceRect>,
         callback: impl FnOnce(Result<RgbaImage, ScreenshotCaptureError>) + 'static,
     ) {
         self.inner()
             .compositor
             .borrow()
-            .request_screenshot(self.id(), Box::new(callback));
+            .request_screenshot(self.id(), rect, Box::new(callback));
     }
 }
 

components/shared/embedder/Cargo.toml

@@ -23,6 +23,7 @@ crossbeam-channel = { workspace = true }
 euclid = { workspace = true }
 http = { workspace = true }
 hyper_serde = { workspace = true }
+image = { workspace = true }
 ipc-channel = { workspace = true }
 keyboard-types = { workspace = true }
 log = { workspace = true }

components/shared/embedder/webdriver.rs

@@ -9,12 +9,13 @@ use std::collections::HashMap;
 use base::generic_channel::GenericSender;
 use base::id::{BrowsingContextId, WebViewId};
 use cookie::Cookie;
+use crossbeam_channel::Sender;
 use euclid::default::Rect as UntypedRect;
 use euclid::{Rect, Size2D};
 use hyper_serde::Serde;
+use image::RgbaImage;
 use ipc_channel::ipc::IpcSender;
 use keyboard_types::{CompositionEvent, KeyboardEvent};
-use pixels::RasterImage;
 use rustc_hash::FxHashMap;
 use serde::{Deserialize, Serialize};
 use servo_geometry::DeviceIndependentIntRect;
@@ -23,7 +24,7 @@ use style_traits::CSSPixel;
 use webdriver::error::ErrorStatus;
 use webrender_api::units::DevicePixel;
 
-use crate::{JSValue, MouseButton, MouseButtonAction, TraversalId};
+use crate::{JSValue, MouseButton, MouseButtonAction, ScreenshotCaptureError, TraversalId};
 
 #[derive(Clone, Copy, Debug, Deserialize, PartialEq, Serialize)]
 pub struct WebDriverMessageId(pub usize);
@@ -73,7 +74,7 @@ impl WebDriverUserPromptAction {
 }
 
 /// Messages to the constellation originating from the WebDriver server.
-#[derive(Debug, Deserialize, Serialize)]
+#[derive(Debug)]
 pub enum WebDriverCommandMsg {
     /// Used in the initialization of the WebDriver server to set the sender for sending responses
     /// back to the WebDriver client. It is set to constellation for now
@@ -144,7 +145,7 @@ pub enum WebDriverCommandMsg {
     TakeScreenshot(
         WebViewId,
         Option<Rect<f32, CSSPixel>>,
-        IpcSender<Result<Option<RasterImage>, ()>>,
+        Sender<Result<RgbaImage, ScreenshotCaptureError>>,
     ),
     /// Create a new webview that loads about:blank. The embedder will use
     /// the provided channels to return the top level browsing context id

components/webdriver_server/lib.rs

@@ -34,12 +34,11 @@ use embedder_traits::{
 };
 use euclid::{Point2D, Rect, Size2D};
 use http::method::Method;
-use image::{DynamicImage, ImageFormat, RgbaImage};
+use image::{DynamicImage, ImageFormat};
 use ipc_channel::ipc::{self, IpcReceiver};
 use keyboard_types::webdriver::{Event as DispatchStringEvent, KeyInputState, send_keys};
 use keyboard_types::{Code, Key, KeyState, KeyboardEvent, Location, NamedKey};
 use log::{debug, error, info};
-use pixels::PixelFormat;
 use serde::de::{Deserializer, MapAccess, Visitor};
 use serde::ser::Serializer;
 use serde::{Deserialize, Serialize};
@@ -2317,61 +2316,33 @@ impl Handler {
             script: "requestAnimationFrame(() => arguments[0]());".to_string(),
             args: None,
         });
+        if rect.as_ref().is_some_and(Rect::is_empty) {
+            return Err(WebDriverError::new(
+                ErrorStatus::UnknownError,
+                "The requested `rect` has zero width and/or height",
+            ));
+        }
         let webview_id = self.webview_id()?;
-        let mut img = None;
+        let (sender, receiver) = crossbeam_channel::unbounded();
-
-        let interval = 1000;
-        let iterations = 30000 / interval;
-
-        for _ in 0..iterations {
-            let (sender, receiver) = ipc::channel().unwrap();
         self.send_message_to_embedder(WebDriverCommandMsg::TakeScreenshot(
             webview_id, rect, sender,
         ))?;
 
-            match wait_for_ipc_response(receiver)? {
+        let Ok(result) = receiver.recv() else {
-                Ok(output_img) => {
-                    if let Some(x) = output_img {
-                        img = Some(x);
-                        break;
-                    }
-                },
-                Err(()) => {
             return Err(WebDriverError::new(
                 ErrorStatus::UnknownError,
-                        "The bounding box of element has either 0 width or 0 height",
+                "Failed to receive TakeScreenshot response",
             ));
-                },
         };
-
+        let image = result.map_err(|error| {
-            thread::sleep(Duration::from_millis(interval));
+            WebDriverError::new(
-        }
+                ErrorStatus::UnknownError,
-
+                format!("Failed to take screenshot: {error:?}"),
-        let img = match img {
-            Some(img) => img,
-            None => {
-                return Err(WebDriverError::new(
-                    ErrorStatus::Timeout,
-                    "Taking screenshot timed out",
-                ));
-            },
-        };
-
-        // The compositor always sends RGBA pixels.
-        assert_eq!(
-            img.format,
-            PixelFormat::RGBA8,
-            "Unexpected screenshot pixel format"
-        );
-
-        let rgb = RgbaImage::from_raw(
-            img.metadata.width,
-            img.metadata.height,
-            img.first_frame().bytes.to_vec(),
             )
-        .unwrap();
+        })?;
+
         let mut png_data = Cursor::new(Vec::new());
-        DynamicImage::ImageRgba8(rgb)
+        DynamicImage::ImageRgba8(image)
             .write_to(&mut png_data, ImageFormat::Png)
             .unwrap();
 

ports/servoshell/desktop/app.rs

@@ -24,8 +24,8 @@ use servo::user_content_manager::{UserContentManager, UserScript};
 use servo::webrender_api::ScrollLocation;
 use servo::{
     EmbedderToConstellationMessage, EventLoopWaker, ImeEvent, InputEvent, KeyboardEvent,
-    MouseButtonEvent, MouseMoveEvent, WebDriverCommandMsg, WebDriverScriptCommand,
+    MouseButtonEvent, MouseMoveEvent, ScreenshotCaptureError, WebDriverCommandMsg,
-    WebDriverUserPromptAction, WheelDelta, WheelEvent, WheelMode,
+    WebDriverScriptCommand, WebDriverUserPromptAction, WheelDelta, WheelEvent, WheelMode,
 };
 use url::Url;
 use winit::application::ApplicationHandler;
@@ -613,8 +613,22 @@ impl App {
                 WebDriverCommandMsg::SendAlertText(webview_id, text) => {
                     running_state.set_alert_text_of_newest_dialog(webview_id, text);
                 },
-                WebDriverCommandMsg::TakeScreenshot(..) => {
+                WebDriverCommandMsg::TakeScreenshot(webview_id, rect, result_sender) => {
-                    running_state.servo().execute_webdriver_command(msg);
+                    let Some(webview) = running_state.webview_by_id(webview_id) else {
+                        if let Err(error) =
+                            result_sender.send(Err(ScreenshotCaptureError::WebViewDoesNotExist))
+                        {
+                            warn!("Failed to send response to TakeScreenshot: {error}");
+                        }
+                        continue;
+                    };
+                    let rect =
+                        rect.map(|rect| rect.to_box2d() * webview.device_pixels_per_css_pixel());
+                    webview.take_screenshot(rect, move |result| {
+                        if let Err(error) = result_sender.send(result) {
+                            warn!("Failed to send response to TakeScreenshot: {error}");
+                        }
+                    });
                 },
             };
         }

ports/servoshell/desktop/app_state.rs

@@ -505,7 +505,7 @@ impl RunningAppState {
             return;
         }
 
-        webview.take_screenshot(move |image| {
+        webview.take_screenshot(None, move |image| {
             achieved_stable_image.set(true);
 
             let Some(output_path) = output_path else {

ports/servoshell/egl/app_state.rs

@@ -973,7 +973,7 @@ impl RunningAppState {
             return;
         }
 
-        webview.take_screenshot(move |image| {
+        webview.take_screenshot(None, move |image| {
             achieved_stable_image.set(true);
 
             let Some(output_path) = output_path else {

tests/wpt/meta/webdriver/tests/classic/take_element_screenshot/scroll_into_view.py.ini

@@ -1,3 +0,0 @@
-[scroll_into_view.py]
-  [test_scroll_into_view]
-    expected: FAIL