From f8f7c6ebd19fdc5750cf1c992ae81d0c8c8c08c7 Mon Sep 17 00:00:00 2001 From: Euclid Ye Date: Mon, 16 Jun 2025 13:23:26 +0800 Subject: [PATCH] [WebDriver] Properly report error for `find_element` & `find_elements`; Get correct visible text when matching links (#37452) 1. Properly report new types of errors for `find_element` and `find_elements`. Previously never reported. 1.1. `InvalidSelector` 1.2. `NoSuchElement` 1.3. `InvalidArgument` 2. Get the visible text for `` correctly in `script::webdriver_handler` so that matching would work. Testing: `./mach test-wpt -r --log-raw "D:\servo test log\all.txt" webdriver/tests/classic/find_element/find.py webdriver/tests/classic/find_elements/find.py --product servodriver` --------- Signed-off-by: Euclid Ye --- components/script/script_thread.rs | 4 + components/script/webdriver_handlers.rs | 205 ++++++++++-------- components/webdriver_server/lib.rs | 30 ++- .../tests/classic/find_element/find.py.ini | 9 - .../tests/classic/find_elements/find.py.ini | 12 - 5 files changed, 138 insertions(+), 122 deletions(-) diff --git a/components/script/script_thread.rs b/components/script/script_thread.rs index 0b1f1680a2f..6418506ef96 100644 --- a/components/script/script_thread.rs +++ b/components/script/script_thread.rs @@ -2278,6 +2278,7 @@ impl ScriptThread { selector, partial, reply, + can_gc, ) }, WebDriverScriptCommand::FindElementTagName(selector, reply) => { @@ -2304,6 +2305,7 @@ impl ScriptThread { selector, partial, reply, + can_gc, ) }, WebDriverScriptCommand::FindElementsTagName(selector, reply) => { @@ -2336,6 +2338,7 @@ impl ScriptThread { selector, partial, reply, + can_gc, ), WebDriverScriptCommand::FindElementElementTagName(selector, element_id, reply) => { webdriver_handlers::handle_find_element_element_tag_name( @@ -2368,6 +2371,7 @@ impl ScriptThread { selector, partial, reply, + can_gc, ), WebDriverScriptCommand::FindElementElementsTagName(selector, element_id, reply) => { webdriver_handlers::handle_find_element_elements_tag_name( diff --git a/components/script/webdriver_handlers.rs b/components/script/webdriver_handlers.rs index 67fc12abbfb..a81c9396da7 100644 --- a/components/script/webdriver_handlers.rs +++ b/components/script/webdriver_handlers.rs @@ -104,16 +104,19 @@ pub(crate) fn find_node_by_unique_id_in_document( } } +/// fn matching_links( links: &NodeList, link_text: String, partial: bool, + can_gc: CanGc, ) -> impl Iterator + '_ { links .iter() .filter(move |node| { let content = node - .GetTextContent() + .downcast::() + .map(|element| element.InnerText(can_gc)) .map_or("".to_owned(), String::from) .trim() .to_owned(); @@ -123,32 +126,41 @@ fn matching_links( content == link_text } }) - .map(|node| { - node.upcast::() - .unique_id(node.owner_doc().window().pipeline_id()) - }) + .map(|node| node.unique_id(node.owner_doc().window().pipeline_id())) } fn all_matching_links( root_node: &Node, link_text: String, partial: bool, + can_gc: CanGc, ) -> Result, ErrorStatus> { + // + // Step 7.2. If a DOMException, SyntaxError, XPathException, or other error occurs + // during the execution of the element location strategy, return error invalid selector. root_node .query_selector_all(DOMString::from("a")) - .map_err(|_| ErrorStatus::UnknownError) - .map(|nodes| matching_links(&nodes, link_text, partial).collect()) + .map_err(|_| ErrorStatus::InvalidSelector) + .map(|nodes| matching_links(&nodes, link_text, partial, can_gc).collect()) } fn first_matching_link( root_node: &Node, link_text: String, partial: bool, + can_gc: CanGc, ) -> Result, ErrorStatus> { + // + // Step 7.2. If a DOMException, SyntaxError, XPathException, or other error occurs + // during the execution of the element location strategy, return error invalid selector. root_node .query_selector_all(DOMString::from("a")) - .map_err(|_| ErrorStatus::UnknownError) - .map(|nodes| matching_links(&nodes, link_text, partial).take(1).next()) + .map_err(|_| ErrorStatus::InvalidSelector) + .map(|nodes| { + matching_links(&nodes, link_text, partial, can_gc) + .take(1) + .next() + }) } #[allow(unsafe_code)] @@ -584,25 +596,42 @@ pub(crate) fn handle_get_element_in_view_center_point( .unwrap(); } +fn retrieve_document_and_check_root_existence( + documents: &DocumentCollection, + pipeline: PipelineId, +) -> Result, ErrorStatus> { + let document = documents + .find_document(pipeline) + .ok_or(ErrorStatus::NoSuchWindow)?; + + // + // + // Step 7 - 8. If current browsing context's document element is null, + // return error with error code no such element. + if document.GetDocumentElement().is_none() { + Err(ErrorStatus::NoSuchElement) + } else { + Ok(document) + } +} + pub(crate) fn handle_find_element_css( documents: &DocumentCollection, pipeline: PipelineId, selector: String, reply: IpcSender, ErrorStatus>>, ) { - reply - .send( - documents - .find_document(pipeline) - .ok_or(ErrorStatus::UnknownError) - .and_then(|document| { - document - .QuerySelector(DOMString::from(selector)) - .map_err(|_| ErrorStatus::InvalidSelector) - }) - .map(|node| node.map(|x| x.upcast::().unique_id(pipeline))), - ) - .unwrap(); + match retrieve_document_and_check_root_existence(documents, pipeline) { + Ok(document) => reply + .send( + document + .QuerySelector(DOMString::from(selector)) + .map_err(|_| ErrorStatus::InvalidSelector) + .map(|node| node.map(|x| x.upcast::().unique_id(pipeline))), + ) + .unwrap(), + Err(error) => reply.send(Err(error)).unwrap(), + } } pub(crate) fn handle_find_element_link_text( @@ -611,17 +640,19 @@ pub(crate) fn handle_find_element_link_text( selector: String, partial: bool, reply: IpcSender, ErrorStatus>>, + can_gc: CanGc, ) { - reply - .send( - documents - .find_document(pipeline) - .ok_or(ErrorStatus::UnknownError) - .and_then(|document| { - first_matching_link(document.upcast::(), selector.clone(), partial) - }), - ) - .unwrap(); + match retrieve_document_and_check_root_existence(documents, pipeline) { + Ok(document) => reply + .send(first_matching_link( + document.upcast::(), + selector.clone(), + partial, + can_gc, + )) + .unwrap(), + Err(error) => reply.send(Err(error)).unwrap(), + } } pub(crate) fn handle_find_element_tag_name( @@ -631,20 +662,16 @@ pub(crate) fn handle_find_element_tag_name( reply: IpcSender, ErrorStatus>>, can_gc: CanGc, ) { - reply - .send( - documents - .find_document(pipeline) - .ok_or(ErrorStatus::UnknownError) - .map(|document| { - document - .GetElementsByTagName(DOMString::from(selector), can_gc) - .elements_iter() - .next() - }) - .map(|node| node.map(|x| x.upcast::().unique_id(pipeline))), - ) - .unwrap(); + match retrieve_document_and_check_root_existence(documents, pipeline) { + Ok(document) => reply + .send(Ok(document + .GetElementsByTagName(DOMString::from(selector), can_gc) + .elements_iter() + .next() + .map(|node| node.upcast::().unique_id(pipeline)))) + .unwrap(), + Err(error) => reply.send(Err(error)).unwrap(), + } } pub(crate) fn handle_find_elements_css( @@ -653,24 +680,22 @@ pub(crate) fn handle_find_elements_css( selector: String, reply: IpcSender, ErrorStatus>>, ) { - reply - .send( - documents - .find_document(pipeline) - .ok_or(ErrorStatus::UnknownError) - .and_then(|document| { - document - .QuerySelectorAll(DOMString::from(selector)) - .map_err(|_| ErrorStatus::InvalidSelector) - }) - .map(|nodes| { - nodes - .iter() - .map(|x| x.upcast::().unique_id(pipeline)) - .collect() - }), - ) - .unwrap(); + match retrieve_document_and_check_root_existence(documents, pipeline) { + Ok(document) => reply + .send( + document + .QuerySelectorAll(DOMString::from(selector)) + .map_err(|_| ErrorStatus::InvalidSelector) + .map(|nodes| { + nodes + .iter() + .map(|x| x.upcast::().unique_id(pipeline)) + .collect() + }), + ) + .unwrap(), + Err(error) => reply.send(Err(error)).unwrap(), + } } pub(crate) fn handle_find_elements_link_text( @@ -679,17 +704,19 @@ pub(crate) fn handle_find_elements_link_text( selector: String, partial: bool, reply: IpcSender, ErrorStatus>>, + can_gc: CanGc, ) { - reply - .send( - documents - .find_document(pipeline) - .ok_or(ErrorStatus::UnknownError) - .and_then(|document| { - all_matching_links(document.upcast::(), selector.clone(), partial) - }), - ) - .unwrap(); + match retrieve_document_and_check_root_existence(documents, pipeline) { + Ok(document) => reply + .send(all_matching_links( + document.upcast::(), + selector.clone(), + partial, + can_gc, + )) + .unwrap(), + Err(error) => reply.send(Err(error)).unwrap(), + } } pub(crate) fn handle_find_elements_tag_name( @@ -699,20 +726,16 @@ pub(crate) fn handle_find_elements_tag_name( reply: IpcSender, ErrorStatus>>, can_gc: CanGc, ) { - reply - .send( - documents - .find_document(pipeline) - .ok_or(ErrorStatus::UnknownError) - .map(|document| document.GetElementsByTagName(DOMString::from(selector), can_gc)) - .map(|nodes| { - nodes - .elements_iter() - .map(|x| x.upcast::().unique_id(pipeline)) - .collect::>() - }), - ) - .unwrap(); + match retrieve_document_and_check_root_existence(documents, pipeline) { + Ok(document) => reply + .send(Ok(document + .GetElementsByTagName(DOMString::from(selector), can_gc) + .elements_iter() + .map(|x| x.upcast::().unique_id(pipeline)) + .collect::>())) + .unwrap(), + Err(error) => reply.send(Err(error)).unwrap(), + } } pub(crate) fn handle_find_element_element_css( @@ -740,11 +763,12 @@ pub(crate) fn handle_find_element_element_link_text( selector: String, partial: bool, reply: IpcSender, ErrorStatus>>, + can_gc: CanGc, ) { reply .send( find_node_by_unique_id(documents, pipeline, element_id) - .and_then(|node| first_matching_link(&node, selector.clone(), partial)), + .and_then(|node| first_matching_link(&node, selector.clone(), partial, can_gc)), ) .unwrap(); } @@ -803,11 +827,12 @@ pub(crate) fn handle_find_element_elements_link_text( selector: String, partial: bool, reply: IpcSender, ErrorStatus>>, + can_gc: CanGc, ) { reply .send( find_node_by_unique_id(documents, pipeline, element_id) - .and_then(|node| all_matching_links(&node, selector.clone(), partial)), + .and_then(|node| all_matching_links(&node, selector.clone(), partial, can_gc)), ) .unwrap(); } diff --git a/components/webdriver_server/lib.rs b/components/webdriver_server/lib.rs index b364b88423b..513823fbd0d 100644 --- a/components/webdriver_server/lib.rs +++ b/components/webdriver_server/lib.rs @@ -929,10 +929,15 @@ impl Handler { ))) } + /// fn handle_find_element( &self, parameters: &LocatorParameters, ) -> WebDriverResult { + // Step 4. If selector is undefined, return error with error code invalid argument. + if parameters.value.is_empty() { + return Err(WebDriverError::new(ErrorStatus::InvalidArgument, "")); + } let (sender, receiver) = ipc::channel().unwrap(); match parameters.using { @@ -961,12 +966,15 @@ impl Handler { }, } + // Step 10. If result is empty, return error with error code no such element. + // Otherwise, return the first element of result. match wait_for_script_response(receiver)? { - Ok(value) => { - let value_resp = serde_json::to_value( - value.map(|x| serde_json::to_value(WebElement(x)).unwrap()), - )?; - Ok(WebDriverResponse::Generic(ValueResponse(value_resp))) + Ok(value) => match value { + Some(value) => { + let value_resp = serde_json::to_value(WebElement(value)).unwrap(); + Ok(WebDriverResponse::Generic(ValueResponse(value_resp))) + }, + None => Err(WebDriverError::new(ErrorStatus::NoSuchElement, "")), }, Err(error) => Err(WebDriverError::new(error, "")), } @@ -1121,13 +1129,16 @@ impl Handler { } } - // https://w3c.github.io/webdriver/#find-elements + /// fn handle_find_elements( &self, parameters: &LocatorParameters, ) -> WebDriverResult { + // Step 4. If selector is undefined, return error with error code invalid argument. + if parameters.value.is_empty() { + return Err(WebDriverError::new(ErrorStatus::InvalidArgument, "")); + } let (sender, receiver) = ipc::channel().unwrap(); - match parameters.using { LocatorStrategy::CSSSelector => { let cmd = WebDriverScriptCommand::FindElementsCSS(parameters.value.clone(), sender); @@ -1156,10 +1167,7 @@ impl Handler { match wait_for_script_response(receiver)? { Ok(value) => { - let resp_value: Vec = value - .into_iter() - .map(|x| serde_json::to_value(WebElement(x)).unwrap()) - .collect(); + let resp_value: Vec = value.into_iter().map(WebElement).collect(); Ok(WebDriverResponse::Generic(ValueResponse( serde_json::to_value(resp_value)?, ))) diff --git a/tests/wpt/meta/webdriver/tests/classic/find_element/find.py.ini b/tests/wpt/meta/webdriver/tests/classic/find_element/find.py.ini index 60f973b8a44..0fd6fbb2bf1 100644 --- a/tests/wpt/meta/webdriver/tests/classic/find_element/find.py.ini +++ b/tests/wpt/meta/webdriver/tests/classic/find_element/find.py.ini @@ -2,15 +2,6 @@ [test_no_browsing_context] expected: FAIL - [test_no_such_element_with_unknown_selector[not-existent\]] - expected: FAIL - - [test_no_such_element_with_unknown_selector[existent-other-frame\]] - expected: FAIL - - [test_no_such_element_with_unknown_selector[existent-inside-shadow-root\]] - expected: FAIL - [test_find_element[xpath-//a\]] expected: FAIL diff --git a/tests/wpt/meta/webdriver/tests/classic/find_elements/find.py.ini b/tests/wpt/meta/webdriver/tests/classic/find_elements/find.py.ini index 06a8b89b5b6..072f59fd4f7 100644 --- a/tests/wpt/meta/webdriver/tests/classic/find_elements/find.py.ini +++ b/tests/wpt/meta/webdriver/tests/classic/find_elements/find.py.ini @@ -5,18 +5,6 @@ [test_find_elements[xpath-//a\]] expected: FAIL - [test_find_elements_link_text[link
text-link\\ntext\]] - expected: FAIL - - [test_find_elements_link_text[link text-LINK TEXT\]] - expected: FAIL - - [test_find_elements_partial_link_text[partial link
text-k\\nt\]] - expected: FAIL - - [test_find_elements_partial_link_text[partial link text-LINK\]] - expected: FAIL - [test_xhtml_namespace[xpath-//*[name()='a'\]\]] expected: FAIL