From 77ae3a0eb3917c36ea1b283c7bf3fb82bd764fe3 Mon Sep 17 00:00:00 2001 From: Kenzie Raditya Tirtarahardja Date: Sun, 21 Sep 2025 13:42:16 +0800 Subject: [PATCH] webdriver: Improve documentations for `actions.rs` and update TODOs (#39391) Add specification quote for each steps, remove some irrelevant TODO, and move function position closer to related function. Testing: No behaviour change --------- Signed-off-by: PotatoCP Signed-off-by: Euclid Ye Co-authored-by: Euclid Ye --- components/webdriver_server/actions.rs | 294 ++++++++++++++----------- components/webdriver_server/lib.rs | 5 - ports/servoshell/desktop/app.rs | 1 - 3 files changed, 166 insertions(+), 134 deletions(-) diff --git a/components/webdriver_server/actions.rs b/components/webdriver_server/actions.rs index 0b6532ef443..8e5c3e192b6 100644 --- a/components/webdriver_server/actions.rs +++ b/components/webdriver_server/actions.rs @@ -368,11 +368,16 @@ impl Handler { _ => unreachable!(), }; + // Step 3. If the source's pressed property contains button return success with data null. if pointer_input_state.pressed.contains(&action.button) { return; } - pointer_input_state.pressed.insert(action.button); + // Step 6. Add button to the set corresponding to source's pressed property + pointer_input_state.pressed.insert(action.button); + // Step 7 - 15: Variable namings already done. + // Step 16. Perform implementation-specific action dispatch steps + // TODO: We have not considered pen/touch pointer type self.increment_num_pending_actions(); let msg_id = self.current_action_id.get(); let cmd_msg = WebDriverCommandMsg::MouseButtonAction( @@ -384,6 +389,8 @@ impl Handler { msg_id, ); let _ = self.send_message_to_embedder(cmd_msg); + + // Step 17. Return success with data null. } /// @@ -396,9 +403,12 @@ impl Handler { _ => unreachable!(), }; + // Step 3. If the source's pressed property does not contain button, return success with data null. if !pointer_input_state.pressed.contains(&action.button) { return; } + + // Step 6. Remove button from the set corresponding to source's pressed property, pointer_input_state.pressed.remove(&action.button); // Remove matching pointerUp(must be unique) from `[input_cancel_list]` due to bugs in spec @@ -419,6 +429,7 @@ impl Handler { } } + // Step 7. Perform implementation-specific action dispatch steps self.increment_num_pending_actions(); let msg_id = self.current_action_id.get(); let cmd_msg = WebDriverCommandMsg::MouseButtonAction( @@ -430,45 +441,8 @@ impl Handler { msg_id, ); let _ = self.send_message_to_embedder(cmd_msg); - } - /// - fn get_origin_relative_coordinates( - &self, - origin: &PointerOrigin, - x_offset: f64, - y_offset: f64, - source_id: &str, - ) -> Result<(f64, f64), ErrorStatus> { - match origin { - PointerOrigin::Viewport => Ok((x_offset, y_offset)), - PointerOrigin::Pointer => { - // Step 1. Let start x be equal to the x property of source. - // Step 2. Let start y be equal to the y property of source. - let (start_x, start_y) = match self - .session() - .unwrap() - .input_state_table() - .get(source_id) - .unwrap() - { - InputSourceState::Pointer(pointer_input_state) => { - (pointer_input_state.x, pointer_input_state.y) - }, - _ => unreachable!(), - }; - // Step 3. Let x equal start x + x offset and y equal start y + y offset. - Ok((start_x + x_offset, start_y + y_offset)) - }, - PointerOrigin::Element(web_element) => { - // Steps 1 - 2: Check "no such element", covered in script thread handler. - - // Step 3. Let x element and y element be the result of calculating the in-view center point of element. - let (x_element, y_element) = self.get_element_in_view_center_point(web_element)?; - // Step 4. Let x equal x element + x offset, and y equal y element + y offset. - Ok((x_element as f64 + x_offset, y_element as f64 + y_offset)) - }, - } + // Step 8. Return success with data null. } /// @@ -482,6 +456,7 @@ impl Handler { // Step 1. Let x offset be equal to the x property of action object. let x_offset = action.x; + // Step 2. Let y offset be equal to the y property of action object. let y_offset = action.y; @@ -493,7 +468,10 @@ impl Handler { let (x, y) = self.get_origin_relative_coordinates(origin, x_offset, y_offset, source_id)?; - // Step 5 - 6 + // Step 5. If x is less than 0 or greater than the width of the viewport in CSS pixels, + // then return error with error code move target out of bounds. + // Step 6. If y is less than 0 or greater than the height of the viewport in CSS pixels, + // then return error with error code move target out of bounds. self.check_viewport_bound(x, y)?; // Step 7. Let duration be equal to action object's duration property @@ -523,9 +501,12 @@ impl Handler { }; // Step 9 - 18 + // Perform a pointer move with arguments source, global key state, duration, start x, start y, + // x, y, width, height, pressure, tangentialPressure, tiltX, tiltY, twist, altitudeAngle, azimuthAngle. + // TODO: We have not considered pen/touch pointer type self.perform_pointer_move(source_id, duration, start_x, start_y, x, y, tick_start); - // Step 19 + // Step 19. Return success with data null. Ok(()) } @@ -549,20 +530,27 @@ impl Handler { }; loop { - // Step 1 + // Step 1. Let time delta be the time since the beginning of the + // current tick, measured in milliseconds on a monotonic clock. let time_delta = tick_start.elapsed().as_millis(); - // Step 2 + // Step 2. Let duration ratio be the ratio of time delta and duration, + // if duration is greater than 0, or 1 otherwise. let duration_ratio = if duration > 0 { time_delta as f64 / duration as f64 } else { 1.0 }; - // Step 3 + // Step 3. If duration ratio is 1, or close enough to 1 that the + // implementation will not further subdivide the move action, + // let last be true. Otherwise let last be false. let last = 1.0 - duration_ratio < 0.001; - // Step 4 + // Step 4. If last is true, let x equal target x and y equal target y. + // Otherwise + // let x equal an approximation to duration ratio × (target x - start x) + start x, + // and y equal an approximation to duration ratio × (target y - start y) + start y. let (x, y) = if last { (target_x, target_y) } else { @@ -572,15 +560,18 @@ impl Handler { ) }; - // Steps 5 - 6 + // Step 5. Let current x equal the x property of input state. let current_x = pointer_input_state.x; + + // Step 6. Let current y equal the y property of input state. let current_y = pointer_input_state.y; - // Step 7 - // Actually "last" should not be checked here based on spec. + // Step 7. If x != current x or y != current y, run the following steps: + // FIXME: Actually "last" should not be checked here based on spec. // However, we need to send the webdriver id at the final perform. if x != current_x || y != current_y || last { - // Step 7.2 + // Step 7.1. Let buttons be equal to input state's buttons property. + // Step 7.2. Perform implementation-specific action dispatch steps let msg_id = if last { self.increment_num_pending_actions(); self.current_action_id.get() @@ -594,18 +585,23 @@ impl Handler { msg_id, ); let _ = self.send_message_to_embedder(cmd_msg); - // Step 7.3 + // Step 7.3. Let input state's x property equal x and y property equal y. pointer_input_state.x = x; pointer_input_state.y = y; } - // Step 8 + // Step 8. If last is true, return. if last { return; } - // Step 9 + // Step 9. Run the following substeps in parallel: + // Step 9.1. Asynchronously wait for an implementationdefined amount of time to pass thread::sleep(Duration::from_millis(POINTERMOVE_INTERVAL)); + + // Step 9.2. Perform a pointer move with arguments + // input state, duration, start x, start y, target x, target y. + // Notice that this simply repeat what we have done until last is true. } } @@ -616,10 +612,7 @@ impl Handler { action: &WheelScrollAction, tick_duration: u64, ) -> Result<(), ErrorStatus> { - // Note: We have not implemented `extract an action sequence` which will calls - // `process a wheel action` that validate many of the variable used here. - // Hence, we do all the checking here until those functions is properly - // implemented. + // TODO: We should verify each variable when processing a wheel action. // let tick_start = Instant::now(); @@ -649,7 +642,10 @@ impl Handler { let (x, y) = self.get_origin_relative_coordinates(origin, x_offset as _, y_offset as _, source_id)?; - // Step 5 - 6 + // Step 5. If x is less than 0 or greater than the width of the viewport in CSS pixels, + // then return error with error code move target out of bounds. + // Step 6. If y is less than 0 or greater than the height of the viewport in CSS pixels, + // then return error with error code move target out of bounds. self.check_viewport_bound(x, y)?; // Step 7. Let delta x be equal to the deltaX property of action object. @@ -687,7 +683,7 @@ impl Handler { tick_start, ); - // Step 12 + // Step 12. Return success with data null. Ok(()) } @@ -704,79 +700,78 @@ impl Handler { mut curr_delta_y: f64, tick_start: Instant, ) { - // Step 1. Let time delta be the time since the beginning of the current tick, - // measured in milliseconds on a monotonic clock. - let time_delta = tick_start.elapsed().as_millis(); + loop { + // Step 1. Let time delta be the time since the beginning of the current tick, + // measured in milliseconds on a monotonic clock. + let time_delta = tick_start.elapsed().as_millis(); - // Step 2. Let duration ratio be the ratio of time delta and duration, - // if duration is greater than 0, or 1 otherwise. - let duration_ratio = if duration > 0 { - time_delta as f64 / duration as f64 - } else { - 1.0 - }; - - // Step 3. If duration ratio is 1, or close enough to 1 that - // the implementation will not further subdivide the move action, - // let last be true. Otherwise let last be false. - let last = 1.0 - duration_ratio < 0.001; - - // Step 4 - let (delta_x, delta_y) = if last { - (target_delta_x - curr_delta_x, target_delta_y - curr_delta_y) - } else { - ( - duration_ratio * target_delta_x - curr_delta_x, - duration_ratio * target_delta_y - curr_delta_y, - ) - }; - - // Step 5 - // Actually "last" should not be checked here based on spec. - // However, we need to send the webdriver id at the final perform. - if delta_x != 0.0 || delta_y != 0.0 || last { - // Perform implementation-specific action dispatch steps - let msg_id = if last { - self.increment_num_pending_actions(); - self.current_action_id.get() + // Step 2. Let duration ratio be the ratio of time delta and duration, + // if duration is greater than 0, or 1 otherwise. + let duration_ratio = if duration > 0 { + time_delta as f64 / duration as f64 } else { - None + 1.0 }; - let cmd_msg = WebDriverCommandMsg::WheelScrollAction( - self.verified_webview_id(), - x, - y, - delta_x, - delta_y, - msg_id, - ); - let _ = self.send_message_to_embedder(cmd_msg); - curr_delta_x += delta_x; - curr_delta_y += delta_y; + // Step 3. If duration ratio is 1, or close enough to 1 that + // the implementation will not further subdivide the move action, + // let last be true. Otherwise let last be false. + let last = 1.0 - duration_ratio < 0.001; + + // Step 4. If last is true, + // let delta x equal target delta x - current delta x and delta y equal target delta y - current delta y. + // Otherwise + // let delta x equal an approximation to duration ratio × target delta x - current delta x, + // and delta y equal an approximation to duration ratio × target delta y - current delta y. + let (delta_x, delta_y) = if last { + (target_delta_x - curr_delta_x, target_delta_y - curr_delta_y) + } else { + ( + duration_ratio * target_delta_x - curr_delta_x, + duration_ratio * target_delta_y - curr_delta_y, + ) + }; + + // Step 5. If delta x != 0 or delta y != 0, run the following steps: + // Actually "last" should not be checked here based on spec. + // However, we need to send the webdriver id at the final perform. + if delta_x != 0.0 || delta_y != 0.0 || last { + // Step 5.1. Perform implementation-specific action dispatch steps + let msg_id = if last { + self.increment_num_pending_actions(); + self.current_action_id.get() + } else { + None + }; + let cmd_msg = WebDriverCommandMsg::WheelScrollAction( + self.verified_webview_id(), + x, + y, + delta_x, + delta_y, + msg_id, + ); + let _ = self.send_message_to_embedder(cmd_msg); + + // Step 5.2. Let current delta x property equal delta x + current delta x + // and current delta y property equal delta y + current delta y. + curr_delta_x += delta_x; + curr_delta_y += delta_y; + } + + // Step 6. If last is true, return. + if last { + return; + } + + // Step 7 + // TODO: The two steps should be done in parallel + // 7.1. Asynchronously wait for an implementation defined amount of time to pass. + thread::sleep(Duration::from_millis(WHEELSCROLL_INTERVAL)); + // 7.2. Perform a scroll with arguments duration, x, y, target delta x, + // target delta y, current delta x, current delta y. + // Notice that this simply repeat what we have done until last is true. } - - // Step 6 - if last { - return; - } - - // Step 7 - // TODO: The two steps should be done in parallel - // 7.1. Asynchronously wait for an implementation defined amount of time to pass. - thread::sleep(Duration::from_millis(WHEELSCROLL_INTERVAL)); - // 7.2. Perform a scroll with arguments duration, x, y, target delta x, - // target delta y, current delta x, current delta y. - self.perform_scroll( - duration, - x, - y, - target_delta_x, - target_delta_y, - curr_delta_x, - curr_delta_y, - tick_start, - ); } /// Verify that the given coordinates are within the boundary of the viewport. @@ -802,21 +797,64 @@ impl Handler { } } + /// + fn get_origin_relative_coordinates( + &self, + origin: &PointerOrigin, + x_offset: f64, + y_offset: f64, + source_id: &str, + ) -> Result<(f64, f64), ErrorStatus> { + match origin { + PointerOrigin::Viewport => Ok((x_offset, y_offset)), + PointerOrigin::Pointer => { + // Step 1. Let start x be equal to the x property of source. + // Step 2. Let start y be equal to the y property of source. + let (start_x, start_y) = match self + .session() + .unwrap() + .input_state_table() + .get(source_id) + .unwrap() + { + InputSourceState::Pointer(pointer_input_state) => { + (pointer_input_state.x, pointer_input_state.y) + }, + _ => unreachable!(), + }; + // Step 3. Let x equal start x + x offset and y equal start y + y offset. + Ok((start_x + x_offset, start_y + y_offset)) + }, + PointerOrigin::Element(web_element) => { + // Steps 1 - 3 + let (x_element, y_element) = self.get_element_in_view_center_point(web_element)?; + // Step 4. Let x equal x element + x offset, and y equal y element + y offset. + Ok((x_element as f64 + x_offset, y_element as f64 + y_offset)) + }, + } + } + /// fn get_element_in_view_center_point( &self, web_element: &WebElement, ) -> Result<(i64, i64), ErrorStatus> { let (sender, receiver) = ipc::channel().unwrap(); + // Step 1. Let element be the result of trying to run actions options' + // get element origin steps with origin and browsing context. self.browsing_context_script_command( WebDriverScriptCommand::GetElementInViewCenterPoint(web_element.to_string(), sender), VerifyBrowsingContextIsOpen::No, ) .unwrap(); + + // Step 2. If element is null, return error with error code no such element. let response = match wait_for_ipc_response(receiver) { Ok(response) => response, Err(WebDriverError { error, .. }) => return Err(error), }; + + // Step 3. Let x element and y element be the result of calculating the in-view center point of element. match response? { Some(point) => Ok(point), None => Err(ErrorStatus::UnknownError), @@ -826,7 +864,7 @@ impl Handler { /// pub(crate) fn extract_an_action_sequence(&self, params: ActionsParameters) -> ActionsByTick { // Step 1. Let actions be the result of getting a property named "actions" from parameters. - // Step 2 (ignored because params is already validated earlier). If actions is not a list, + // Step 2. (ignored because params is already validated earlier). If actions is not a list, // return an error with status InvalidArgument. let actions = params.actions; @@ -852,7 +890,7 @@ impl Handler { actions_by_tick.push(Vec::new()); } - // Step 4.2.3. + // Step 4.2.3. Append action to the List at index i in actions by tick. for (tick_index, action_item) in source_actions.into_iter().enumerate() { actions_by_tick[tick_index].push((id.clone(), action_item)); } diff --git a/components/webdriver_server/lib.rs b/components/webdriver_server/lib.rs index 199fc92cfab..4b5f10a52ba 100644 --- a/components/webdriver_server/lib.rs +++ b/components/webdriver_server/lib.rs @@ -2134,17 +2134,12 @@ impl Handler { ); self.browsing_context_script_command(cmd, VerifyBrowsingContextIsOpen::No)?; - // TODO: distinguish the not found and not focusable cases // File input and non-typeable form control should have // been handled in `webdriver_handler.rs`. if !wait_for_ipc_response(receiver)?.map_err(|error| WebDriverError::new(error, ""))? { return Ok(WebDriverResponse::Void); } - // TODO: there's a race condition caused by the focus command and the - // send keys command being two separate messages, - // so the constellation may have changed state between them. - // Step 10. Let input id be a the result of generating a UUID. let id = Uuid::new_v4().to_string(); diff --git a/ports/servoshell/desktop/app.rs b/ports/servoshell/desktop/app.rs index 103de4ca118..3a22daa15a3 100644 --- a/ports/servoshell/desktop/app.rs +++ b/ports/servoshell/desktop/app.rs @@ -502,7 +502,6 @@ impl App { } }, WebDriverCommandMsg::KeyboardAction(webview_id, key_event, msg_id) => { - // TODO: We should do processing like in `headed_window:handle_keyboard_input`. if let Some(webview) = running_state.webview_by_id(webview_id) { webview.notify_input_event( InputEvent::Keyboard(KeyboardEvent::new(key_event))