From cfa9e711c58db96d834e170f89ffe7ccf84f0ab7 Mon Sep 17 00:00:00 2001 From: shuppy Date: Wed, 1 Oct 2025 20:37:00 +0800 Subject: [PATCH] webdriver: Use `take_screenshot()` API in Take (Element) Screenshot (#39587) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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](https://github.com/servo/servo/blob/a2f551eb2dda6ca35262d1c4a3b3f8d27f9ffdb5/tests/wpt/tests/tools/wptrunner/wptrunner/executors/test-wait.js) to do this in userland. when testing Servo with [--product servo](https://github.com/servo/servo/blob/a2f551eb2dda6ca35262d1c4a3b3f8d27f9ffdb5/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](https://github.com/servo/servo/blob/a2f551eb2dda6ca35262d1c4a3b3f8d27f9ffdb5/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](https://github.com/servo/servo/blob/a2f551eb2dda6ca35262d1c4a3b3f8d27f9ffdb5/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 Co-authored-by: Martin Robinson --- Cargo.lock | 1 + components/compositing/compositor.rs | 68 ++++------------- components/compositing/screenshot.rs | 27 ++++++- components/servo/lib.rs | 28 +------ components/servo/webview.rs | 19 ++++- components/shared/embedder/Cargo.toml | 1 + components/shared/embedder/webdriver.rs | 9 ++- components/webdriver_server/lib.rs | 75 ++++++------------- ports/servoshell/desktop/app.rs | 22 +++++- ports/servoshell/desktop/app_state.rs | 2 +- ports/servoshell/egl/app_state.rs | 2 +- .../scroll_into_view.py.ini | 3 - 12 files changed, 106 insertions(+), 151 deletions(-) delete mode 100644 tests/wpt/meta/webdriver/tests/classic/take_element_screenshot/scroll_into_view.py.ini diff --git a/Cargo.lock b/Cargo.lock index 2c0b37af6f8..2cf1308c582 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2384,6 +2384,7 @@ dependencies = [ "euclid", "http 1.3.1", "hyper_serde", + "image", "ipc-channel", "keyboard-types", "log", diff --git a/components/compositing/compositor.rs b/components/compositing/compositor.rs index 4a80b6f5048..09475084c57 100644 --- a/components/compositing/compositor.rs +++ b/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>, - ) -> Option { - 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 { + 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, callback: Box) + '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), ); diff --git a/components/compositing/screenshot.rs b/components/compositing/screenshot.rs index 287058415b2..f62339014aa 100644 --- a/components/compositing/screenshot.rs +++ b/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, callback: Box) + '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, callback: Box) + '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 diff --git a/components/servo/lib.rs b/components/servo/lib.rs index 5cade9e9641..160fd2dd6a3 100644 --- a/components/servo/lib.rs +++ b/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,29 +1060,8 @@ 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)); - } + self.constellation_proxy + .send(EmbedderToConstellationMessage::WebDriverCommand(command)); } pub fn set_preference(&self, name: &str, value: PrefValue) { diff --git a/components/servo/webview.rs b/components/servo/webview.rs index 2af9479432d..017690b0087 100644 --- a/components/servo/webview.rs +++ b/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 { + 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 - /// wait until the [`WebView`] is ready before the screenshot is taken. This includes - /// waiting for: + /// Asynchronously take a screenshot of the [`WebView`] contents, given a `rect` or the whole + /// viewport, if no `rect` is given. + /// + /// 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 `` @@ -606,12 +616,13 @@ impl WebView { /// operation. pub fn take_screenshot( &self, + rect: Option, callback: impl FnOnce(Result) + 'static, ) { self.inner() .compositor .borrow() - .request_screenshot(self.id(), Box::new(callback)); + .request_screenshot(self.id(), rect, Box::new(callback)); } } diff --git a/components/shared/embedder/Cargo.toml b/components/shared/embedder/Cargo.toml index b0a33453a8c..6430fc8f870 100644 --- a/components/shared/embedder/Cargo.toml +++ b/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 } diff --git a/components/shared/embedder/webdriver.rs b/components/shared/embedder/webdriver.rs index 45c888b032f..87f1a42e76b 100644 --- a/components/shared/embedder/webdriver.rs +++ b/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>, - IpcSender, ()>>, + Sender>, ), /// Create a new webview that loads about:blank. The embedder will use /// the provided channels to return the top level browsing context id diff --git a/components/webdriver_server/lib.rs b/components/webdriver_server/lib.rs index 7e882fd1b73..ac64bb738a6 100644 --- a/components/webdriver_server/lib.rs +++ b/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, }); - let webview_id = self.webview_id()?; - let mut img = None; - - 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)? { - 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", - )); - }, - }; - - thread::sleep(Duration::from_millis(interval)); + 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 (sender, receiver) = crossbeam_channel::unbounded(); + self.send_message_to_embedder(WebDriverCommandMsg::TakeScreenshot( + webview_id, rect, sender, + ))?; - let img = match img { - Some(img) => img, - None => { - return Err(WebDriverError::new( - ErrorStatus::Timeout, - "Taking screenshot timed out", - )); - }, + let Ok(result) = receiver.recv() else { + return Err(WebDriverError::new( + ErrorStatus::UnknownError, + "Failed to receive TakeScreenshot response", + )); }; + let image = result.map_err(|error| { + WebDriverError::new( + ErrorStatus::UnknownError, + format!("Failed to take screenshot: {error:?}"), + ) + })?; - // 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(); diff --git a/ports/servoshell/desktop/app.rs b/ports/servoshell/desktop/app.rs index fcb5bd1f4b9..31f499966e7 100644 --- a/ports/servoshell/desktop/app.rs +++ b/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, - WebDriverUserPromptAction, WheelDelta, WheelEvent, WheelMode, + MouseButtonEvent, MouseMoveEvent, ScreenshotCaptureError, WebDriverCommandMsg, + 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(..) => { - running_state.servo().execute_webdriver_command(msg); + WebDriverCommandMsg::TakeScreenshot(webview_id, rect, result_sender) => { + 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}"); + } + }); }, }; } diff --git a/ports/servoshell/desktop/app_state.rs b/ports/servoshell/desktop/app_state.rs index d46d449b1ac..57f0114d526 100644 --- a/ports/servoshell/desktop/app_state.rs +++ b/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 { diff --git a/ports/servoshell/egl/app_state.rs b/ports/servoshell/egl/app_state.rs index 26443dfe039..050d884c028 100644 --- a/ports/servoshell/egl/app_state.rs +++ b/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 { diff --git a/tests/wpt/meta/webdriver/tests/classic/take_element_screenshot/scroll_into_view.py.ini b/tests/wpt/meta/webdriver/tests/classic/take_element_screenshot/scroll_into_view.py.ini deleted file mode 100644 index 3c21fb97f05..00000000000 --- a/tests/wpt/meta/webdriver/tests/classic/take_element_screenshot/scroll_into_view.py.ini +++ /dev/null @@ -1,3 +0,0 @@ -[scroll_into_view.py] - [test_scroll_into_view] - expected: FAIL