From 8e2d2bde6f2132ce291543afa1cf8f8872a9ff44 Mon Sep 17 00:00:00 2001 From: batu_hoang <55729155+longvatrong111@users.noreply.github.com> Date: Tue, 15 Jul 2025 18:41:50 +0800 Subject: [PATCH] Webdriver GoBack and GoForward commands wait for navigation complete (#37950) After sending `GoBack` or `GoForward` command, webdriver wait for the navigation complete. It can be achieved by waiting for `WebViewDelegate::notify_history_changed` Testing: `tests/wpt/meta/webdriver/tests/classic/back/back.py` `tests/wpt/meta/webdriver/tests/classic/forward/forward.py` --------- Signed-off-by: batu_hoang Signed-off-by: Josh Matthews Signed-off-by: batu_hoang Co-authored-by: Josh Matthews --- Cargo.lock | 1 + components/constellation/constellation.rs | 11 +++++++- components/constellation/tracing.rs | 1 + components/servo/lib.rs | 7 +++++ components/servo/webview.rs | 16 +++++++---- components/servo/webview_delegate.rs | 4 ++- components/shared/constellation/lib.rs | 4 +-- components/shared/embedder/Cargo.toml | 1 + components/shared/embedder/lib.rs | 14 ++++++++++ components/shared/embedder/webdriver.rs | 4 +-- components/webdriver_server/lib.rs | 14 +++++++--- ports/servoshell/desktop/app.rs | 22 +++++++++++---- ports/servoshell/desktop/app_state.rs | 28 +++++++++++++++++-- .../preload-resource-match.https.html.ini | 11 +++++--- .../webdriver/tests/classic/back/back.py.ini | 10 ------- .../tests/classic/forward/forward.py.ini | 6 ++-- 16 files changed, 114 insertions(+), 40 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5bc68049704..ea53aac4d8e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2261,6 +2261,7 @@ dependencies = [ "stylo", "stylo_traits", "url", + "uuid", "webdriver", "webrender_api", ] diff --git a/components/constellation/constellation.rs b/components/constellation/constellation.rs index fb5022a925e..dfff7bd70d3 100644 --- a/components/constellation/constellation.rs +++ b/components/constellation/constellation.rs @@ -1378,8 +1378,17 @@ where self.embedder_proxy.send(EmbedderMsg::WebViewBlurred); }, // Handle a forward or back request - EmbedderToConstellationMessage::TraverseHistory(webview_id, direction) => { + EmbedderToConstellationMessage::TraverseHistory( + webview_id, + direction, + traversal_id, + ) => { self.handle_traverse_history_msg(webview_id, direction); + self.embedder_proxy + .send(EmbedderMsg::HistoryTraversalComplete( + webview_id, + traversal_id, + )); }, EmbedderToConstellationMessage::ChangeViewportDetails( webview_id, diff --git a/components/constellation/tracing.rs b/components/constellation/tracing.rs index 24ea7d7bbf2..9c6395b01b2 100644 --- a/components/constellation/tracing.rs +++ b/components/constellation/tracing.rs @@ -214,6 +214,7 @@ mod from_script { Self::SetCursor(..) => target_variant!("SetCursor"), Self::NewFavicon(..) => target_variant!("NewFavicon"), Self::HistoryChanged(..) => target_variant!("HistoryChanged"), + Self::HistoryTraversalComplete(..) => target_variant!("HistoryTraversalComplete"), Self::GetWindowRect(..) => target_variant!("GetWindowRect"), Self::GetScreenMetrics(..) => target_variant!("GetScreenMetrics"), Self::NotifyFullscreenStateChanged(..) => { diff --git a/components/servo/lib.rs b/components/servo/lib.rs index 81b43a3792b..21100e5bf10 100644 --- a/components/servo/lib.rs +++ b/components/servo/lib.rs @@ -801,6 +801,13 @@ impl Servo { webview.set_load_status(load_status); } }, + EmbedderMsg::HistoryTraversalComplete(webview_id, traversal_id) => { + if let Some(webview) = self.get_webview_handle(webview_id) { + webview + .delegate() + .notify_traversal_complete(webview.clone(), traversal_id); + } + }, EmbedderMsg::HistoryChanged(webview_id, urls, current_index) => { if let Some(webview) = self.get_webview_handle(webview_id) { let urls: Vec<_> = urls.into_iter().map(ServoUrl::into_url).collect(); diff --git a/components/servo/webview.rs b/components/servo/webview.rs index f71f31634a9..7ec4644fa67 100644 --- a/components/servo/webview.rs +++ b/components/servo/webview.rs @@ -14,7 +14,7 @@ use constellation_traits::{EmbedderToConstellationMessage, TraversalDirection}; use dpi::PhysicalSize; use embedder_traits::{ Cursor, InputEvent, JSValue, JavaScriptEvaluationError, LoadStatus, MediaSessionActionType, - ScreenGeometry, Theme, ViewportDetails, + ScreenGeometry, Theme, TraversalId, ViewportDetails, }; use euclid::{Point2D, Scale, Size2D}; use servo_geometry::DeviceIndependentPixel; @@ -416,22 +416,28 @@ impl WebView { .send(EmbedderToConstellationMessage::Reload(self.id())) } - pub fn go_back(&self, amount: usize) { + pub fn go_back(&self, amount: usize) -> TraversalId { + let traversal_id = TraversalId::new(); self.inner() .constellation_proxy .send(EmbedderToConstellationMessage::TraverseHistory( self.id(), TraversalDirection::Back(amount), - )) + traversal_id.clone(), + )); + traversal_id } - pub fn go_forward(&self, amount: usize) { + pub fn go_forward(&self, amount: usize) -> TraversalId { + let traversal_id = TraversalId::new(); self.inner() .constellation_proxy .send(EmbedderToConstellationMessage::TraverseHistory( self.id(), TraversalDirection::Forward(amount), - )) + traversal_id.clone(), + )); + traversal_id } /// Ask the [`WebView`] to scroll web content. Note that positive scroll offsets reveal more diff --git a/components/servo/webview_delegate.rs b/components/servo/webview_delegate.rs index d8c1601356b..2b9b9bae677 100644 --- a/components/servo/webview_delegate.rs +++ b/components/servo/webview_delegate.rs @@ -10,7 +10,7 @@ use embedder_traits::{ AllowOrDeny, AuthenticationResponse, ContextMenuResult, Cursor, FilterPattern, GamepadHapticEffectType, InputMethodType, KeyboardEvent, LoadStatus, MediaSessionEvent, Notification, PermissionFeature, RgbColor, ScreenGeometry, SelectElementOptionOrOptgroup, - SimpleDialog, WebResourceRequest, WebResourceResponse, WebResourceResponseMsg, + SimpleDialog, TraversalId, WebResourceRequest, WebResourceResponse, WebResourceResponseMsg, }; use ipc_channel::ipc::IpcSender; use serde::Serialize; @@ -438,6 +438,8 @@ pub trait WebViewDelegate { /// The history state has changed. // changed pattern; maybe wasteful if embedder doesn’t care? fn notify_history_changed(&self, _webview: WebView, _: Vec, _: usize) {} + /// A history traversal operation is complete. + fn notify_traversal_complete(&self, _webview: WebView, _: TraversalId) {} /// Page content has closed this [`WebView`] via `window.close()`. It's the embedder's /// responsibility to remove the [`WebView`] from the interface when this notification /// occurs. diff --git a/components/shared/constellation/lib.rs b/components/shared/constellation/lib.rs index 225b6ab5f17..751b5f52d07 100644 --- a/components/shared/constellation/lib.rs +++ b/components/shared/constellation/lib.rs @@ -20,7 +20,7 @@ use base::cross_process_instant::CrossProcessInstant; use base::id::{MessagePortId, PipelineId, WebViewId}; use embedder_traits::{ CompositorHitTestResult, Cursor, InputEvent, JavaScriptEvaluationId, MediaSessionActionType, - Theme, ViewportDetails, WebDriverCommandMsg, WebDriverCommandResponse, + Theme, TraversalId, ViewportDetails, WebDriverCommandMsg, WebDriverCommandResponse, }; pub use from_script_message::*; use ipc_channel::ipc::IpcSender; @@ -48,7 +48,7 @@ pub enum EmbedderToConstellationMessage { /// Clear the network cache. ClearCache, /// Request to traverse the joint session history of the provided browsing context. - TraverseHistory(WebViewId, TraversalDirection), + TraverseHistory(WebViewId, TraversalDirection, TraversalId), /// Inform the Constellation that a `WebView`'s [`ViewportDetails`] have changed. ChangeViewportDetails(WebViewId, ViewportDetails, WindowSizeType), /// Inform the constellation of a theme change. diff --git a/components/shared/embedder/Cargo.toml b/components/shared/embedder/Cargo.toml index 77734043b0c..22764934808 100644 --- a/components/shared/embedder/Cargo.toml +++ b/components/shared/embedder/Cargo.toml @@ -37,6 +37,7 @@ strum_macros = { workspace = true } stylo_traits = { workspace = true } stylo = { workspace = true } url = { workspace = true } +uuid = { workspace = true } webdriver = { workspace = true } webrender_api = { workspace = true } servo_geometry = { path = "../../geometry" } diff --git a/components/shared/embedder/lib.rs b/components/shared/embedder/lib.rs index 96d2e6dc49e..51de4e580fb 100644 --- a/components/shared/embedder/lib.rs +++ b/components/shared/embedder/lib.rs @@ -37,6 +37,7 @@ use strum_macros::IntoStaticStr; use style::queries::values::PrefersColorScheme; use style_traits::CSSPixel; use url::Url; +use uuid::Uuid; use webrender_api::units::{DeviceIntPoint, DeviceIntRect, DeviceIntSize, DevicePixel}; pub use crate::input_events::*; @@ -318,6 +319,17 @@ pub struct ScreenMetrics { pub available_size: DeviceIndependentIntSize, } +/// An opaque identifier for a single history traversal operation. +#[derive(Clone, Deserialize, PartialEq, Serialize)] +pub struct TraversalId(String); + +impl TraversalId { + #[allow(clippy::new_without_default)] + pub fn new() -> Self { + Self(Uuid::new_v4().to_string()) + } +} + #[derive(Deserialize, IntoStaticStr, Serialize)] pub enum EmbedderMsg { /// A status message to be displayed by the browser chrome. @@ -372,6 +384,8 @@ pub enum EmbedderMsg { NewFavicon(WebViewId, ServoUrl), /// The history state has changed. HistoryChanged(WebViewId, Vec, usize), + /// A history traversal operation completed. + HistoryTraversalComplete(WebViewId, TraversalId), /// Get the device independent window rectangle. GetWindowRect(WebViewId, IpcSender), /// Get the device independent screen size and available size. diff --git a/components/shared/embedder/webdriver.rs b/components/shared/embedder/webdriver.rs index 897289da901..9144dfc584d 100644 --- a/components/shared/embedder/webdriver.rs +++ b/components/shared/embedder/webdriver.rs @@ -49,9 +49,9 @@ pub enum WebDriverCommandMsg { /// Refresh the top-level browsing context with the given ID. Refresh(WebViewId, IpcSender), /// Navigate the webview with the given ID to the previous page in the browsing context's history. - GoBack(WebViewId), + GoBack(WebViewId, IpcSender), /// Navigate the webview with the given ID to the next page in the browsing context's history. - GoForward(WebViewId), + GoForward(WebViewId, IpcSender), /// Pass a webdriver command to the script thread of the current pipeline /// of a browsing context. ScriptCommand(BrowsingContextId, WebDriverScriptCommand), diff --git a/components/webdriver_server/lib.rs b/components/webdriver_server/lib.rs index e93d19a0101..e7aa82900ff 100644 --- a/components/webdriver_server/lib.rs +++ b/components/webdriver_server/lib.rs @@ -946,8 +946,11 @@ impl Handler { // return error with error code no such window. self.verify_top_level_browsing_context_is_open(webview_id)?; - self.send_message_to_embedder(WebDriverCommandMsg::GoBack(webview_id))?; - Ok(WebDriverResponse::Void) + self.send_message_to_embedder(WebDriverCommandMsg::GoBack( + webview_id, + self.load_status_sender.clone(), + ))?; + self.wait_for_load() } fn handle_go_forward(&self) -> WebDriverResult { @@ -956,8 +959,11 @@ impl Handler { // return error with error code no such window. self.verify_top_level_browsing_context_is_open(webview_id)?; - self.send_message_to_embedder(WebDriverCommandMsg::GoForward(webview_id))?; - Ok(WebDriverResponse::Void) + self.send_message_to_embedder(WebDriverCommandMsg::GoForward( + webview_id, + self.load_status_sender.clone(), + ))?; + self.wait_for_load() } fn handle_refresh(&self) -> WebDriverResult { diff --git a/ports/servoshell/desktop/app.rs b/ports/servoshell/desktop/app.rs index e264e62ff0b..5a880ec815b 100644 --- a/ports/servoshell/desktop/app.rs +++ b/ports/servoshell/desktop/app.rs @@ -462,24 +462,34 @@ impl App { }, WebDriverCommandMsg::LoadUrl(webview_id, url, load_status_sender) => { if let Some(webview) = running_state.webview_by_id(webview_id) { - webview.load(url.into_url()); running_state.set_load_status_sender(webview_id, load_status_sender); + webview.load(url.into_url()); } }, WebDriverCommandMsg::Refresh(webview_id, load_status_sender) => { if let Some(webview) = running_state.webview_by_id(webview_id) { - webview.reload(); running_state.set_load_status_sender(webview_id, load_status_sender); + webview.reload(); } }, - WebDriverCommandMsg::GoBack(webview_id) => { + WebDriverCommandMsg::GoBack(webview_id, load_status_sender) => { if let Some(webview) = running_state.webview_by_id(webview_id) { - webview.go_back(1); + let traversal_id = webview.go_back(1); + running_state.set_pending_traversal( + webview_id, + traversal_id, + load_status_sender, + ); } }, - WebDriverCommandMsg::GoForward(webview_id) => { + WebDriverCommandMsg::GoForward(webview_id, load_status_sender) => { if let Some(webview) = running_state.webview_by_id(webview_id) { - webview.go_forward(1); + let traversal_id = webview.go_forward(1); + running_state.set_pending_traversal( + webview_id, + traversal_id, + load_status_sender, + ); } }, // Key events don't need hit test so can be forwarded to constellation for now diff --git a/ports/servoshell/desktop/app_state.rs b/ports/servoshell/desktop/app_state.rs index b8b439db23a..ad4b6ca164a 100644 --- a/ports/servoshell/desktop/app_state.rs +++ b/ports/servoshell/desktop/app_state.rs @@ -4,6 +4,7 @@ use std::cell::{Ref, RefCell, RefMut}; use std::collections::HashMap; +use std::collections::hash_map::Entry; use std::path::PathBuf; use std::rc::Rc; @@ -19,8 +20,8 @@ use servo::webrender_api::units::{DeviceIntPoint, DeviceIntSize}; use servo::{ AllowOrDenyRequest, AuthenticationRequest, FilterPattern, FormControl, GamepadHapticEffectType, KeyboardEvent, LoadStatus, PermissionRequest, Servo, ServoDelegate, ServoError, SimpleDialog, - WebDriverCommandMsg, WebDriverJSResult, WebDriverJSValue, WebDriverLoadStatus, WebView, - WebViewBuilder, WebViewDelegate, + TraversalId, WebDriverCommandMsg, WebDriverJSResult, WebDriverJSValue, WebDriverLoadStatus, + WebView, WebViewBuilder, WebViewDelegate, }; use url::Url; @@ -44,6 +45,7 @@ pub(crate) enum AppState { struct WebDriverSenders { pub load_status_senders: HashMap>, pub script_evaluation_interrupt_sender: Option>, + pub pending_traversals: HashMap)>, } pub(crate) struct RunningAppState { @@ -430,6 +432,18 @@ impl RunningAppState { }); } + pub(crate) fn set_pending_traversal( + &self, + webview_id: WebViewId, + traversal_id: TraversalId, + sender: IpcSender, + ) { + self.webdriver_senders + .borrow_mut() + .pending_traversals + .insert(webview_id, (traversal_id, sender)); + } + pub(crate) fn set_load_status_sender( &self, webview_id: WebViewId, @@ -511,6 +525,16 @@ impl WebViewDelegate for RunningAppState { } } + fn notify_traversal_complete(&self, webview: servo::WebView, traversal_id: TraversalId) { + let mut webdriver_state = self.webdriver_senders.borrow_mut(); + if let Entry::Occupied(entry) = webdriver_state.pending_traversals.entry(webview.id()) { + if entry.get().0 == traversal_id { + let (_, sender) = entry.remove(); + let _ = sender.send(WebDriverLoadStatus::Complete); + } + } + } + fn request_move_to(&self, _: servo::WebView, new_position: DeviceIntPoint) { self.inner().window.set_position(new_position); } diff --git a/tests/wpt/meta/preload/preload-resource-match.https.html.ini b/tests/wpt/meta/preload/preload-resource-match.https.html.ini index 31f9bda5354..d21644d47ee 100644 --- a/tests/wpt/meta/preload/preload-resource-match.https.html.ini +++ b/tests/wpt/meta/preload/preload-resource-match.https.html.ini @@ -73,16 +73,16 @@ expected: FAIL [Loading script (anonymous) with link (no-cors) should discard the preloaded response] - expected: FAIL + expected: TIMEOUT [Loading script (anonymous) with link (use-credentials) should discard the preloaded response] - expected: FAIL + expected: NOTRUN [Loading script (use-credentials) with link (no-cors) should discard the preloaded response] - expected: FAIL + expected: NOTRUN [Loading script (use-credentials) with link (anonymous) should discard the preloaded response] - expected: TIMEOUT + expected: NOTRUN [Loading script (use-credentials) with link (use-credentials) should reuse the preloaded response] expected: NOTRUN @@ -146,3 +146,6 @@ [Loading style (use-credentials) with link (use-credentials) should reuse the preloaded response] expected: NOTRUN + + [Loading script (anonymous) with link (anonymous) should reuse the preloaded response] + expected: NOTRUN diff --git a/tests/wpt/meta/webdriver/tests/classic/back/back.py.ini b/tests/wpt/meta/webdriver/tests/classic/back/back.py.ini index 10d35cc69e3..074e67aef1b 100644 --- a/tests/wpt/meta/webdriver/tests/classic/back/back.py.ini +++ b/tests/wpt/meta/webdriver/tests/classic/back/back.py.ini @@ -1,11 +1,4 @@ [back.py] - disabled: consistent panic - [test_no_top_browsing_context] - expected: FAIL - - [test_no_browsing_context] - expected: ERROR - [test_seen_nodes[http\]] expected: FAIL @@ -14,6 +7,3 @@ [test_seen_nodes[https coop\]] expected: FAIL - - [test_history_pushstate] - expected: FAIL diff --git a/tests/wpt/meta/webdriver/tests/classic/forward/forward.py.ini b/tests/wpt/meta/webdriver/tests/classic/forward/forward.py.ini index 13032a4e96e..6c40e30fe01 100644 --- a/tests/wpt/meta/webdriver/tests/classic/forward/forward.py.ini +++ b/tests/wpt/meta/webdriver/tests/classic/forward/forward.py.ini @@ -1,7 +1,4 @@ [forward.py] - [test_basic] - expected: FAIL - [test_seen_nodes[http\]] expected: FAIL @@ -13,3 +10,6 @@ [test_removed_iframe] expected: FAIL + + [test_basic] + expected: FAIL