[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 `<a>` 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 <yezhizhenjiakang@gmail.com>

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(

components/script/webdriver_handlers.rs

@@ -104,16 +104,19 @@ pub(crate) fn find_node_by_unique_id_in_document(
     }
 }
 
+/// <https://w3c.github.io/webdriver/#dfn-link-text-selector>
 fn matching_links(
     links: &NodeList,
     link_text: String,
     partial: bool,
+    can_gc: CanGc,
 ) -> impl Iterator<Item = String> + '_ {
     links
         .iter()
         .filter(move |node| {
             let content = node
-                .GetTextContent()
+                .downcast::<HTMLElement>()
+                .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::<Node>()
-                .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<Vec<String>, ErrorStatus> {
+    // <https://w3c.github.io/webdriver/#dfn-find>
+    // 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<Option<String>, ErrorStatus> {
+    // <https://w3c.github.io/webdriver/#dfn-find>
+    // 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<DomRoot<Document>, ErrorStatus> {
+    let document = documents
+        .find_document(pipeline)
+        .ok_or(ErrorStatus::NoSuchWindow)?;
+
+    // <https://w3c.github.io/webdriver/#find-element>
+    // <https://w3c.github.io/webdriver/#find-elements>
+    // 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<Result<Option<String>, ErrorStatus>>,
 ) {
-    reply
+    match retrieve_document_and_check_root_existence(documents, pipeline) {
+        Ok(document) => 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::<Node>().unique_id(pipeline))),
             )
-        .unwrap();
+            .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<Result<Option<String>, ErrorStatus>>,
+    can_gc: CanGc,
 ) {
-    reply
-        .send(
-            documents
-                .find_document(pipeline)
-                .ok_or(ErrorStatus::UnknownError)
-                .and_then(|document| {
-                    first_matching_link(document.upcast::<Node>(), selector.clone(), partial)
-                }),
-        )
-        .unwrap();
+    match retrieve_document_and_check_root_existence(documents, pipeline) {
+        Ok(document) => reply
+            .send(first_matching_link(
+                document.upcast::<Node>(),
+                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<Result<Option<String>, ErrorStatus>>,
     can_gc: CanGc,
 ) {
-    reply
-        .send(
-            documents
-                .find_document(pipeline)
-                .ok_or(ErrorStatus::UnknownError)
-                .map(|document| {
-                    document
+    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.map(|x| x.upcast::<Node>().unique_id(pipeline))),
-        )
-        .unwrap();
+                .map(|node| node.upcast::<Node>().unique_id(pipeline))))
+            .unwrap(),
+        Err(error) => reply.send(Err(error)).unwrap(),
+    }
 }
 
 pub(crate) fn handle_find_elements_css(
@@ -653,16 +680,12 @@ pub(crate) fn handle_find_elements_css(
     selector: String,
     reply: IpcSender<Result<Vec<String>, ErrorStatus>>,
 ) {
-    reply
+    match retrieve_document_and_check_root_existence(documents, pipeline) {
+        Ok(document) => 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()
@@ -670,7 +693,9 @@ pub(crate) fn handle_find_elements_css(
                             .collect()
                     }),
             )
-        .unwrap();
+            .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<Result<Vec<String>, ErrorStatus>>,
+    can_gc: CanGc,
 ) {
-    reply
-        .send(
-            documents
-                .find_document(pipeline)
-                .ok_or(ErrorStatus::UnknownError)
-                .and_then(|document| {
-                    all_matching_links(document.upcast::<Node>(), selector.clone(), partial)
-                }),
-        )
-        .unwrap();
+    match retrieve_document_and_check_root_existence(documents, pipeline) {
+        Ok(document) => reply
+            .send(all_matching_links(
+                document.upcast::<Node>(),
+                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<Result<Vec<String>, 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
+    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::<Node>().unique_id(pipeline))
-                        .collect::<Vec<String>>()
-                }),
-        )
-        .unwrap();
+                .collect::<Vec<String>>()))
+            .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<Result<Option<String>, 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<Result<Vec<String>, 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();
 }

components/webdriver_server/lib.rs

@@ -929,10 +929,15 @@ impl Handler {
         )))
     }
 
+    /// <https://w3c.github.io/webdriver/#find-element>
     fn handle_find_element(
         &self,
         parameters: &LocatorParameters,
     ) -> WebDriverResult<WebDriverResponse> {
+        // 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,13 +966,16 @@ 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(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
+    /// <https://w3c.github.io/webdriver/#find-elements>
     fn handle_find_elements(
         &self,
         parameters: &LocatorParameters,
     ) -> WebDriverResult<WebDriverResponse> {
+        // 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> = value
-                    .into_iter()
-                    .map(|x| serde_json::to_value(WebElement(x)).unwrap())
-                    .collect();
+                let resp_value: Vec<WebElement> = value.into_iter().map(WebElement).collect();
                 Ok(WebDriverResponse::Generic(ValueResponse(
                     serde_json::to_value(resp_value)?,
                 )))

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
 

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[<a href=#>link<br>text</a>-link\\ntext\]]
-    expected: FAIL
-
-  [test_find_elements_link_text[<a href=# style='text-transform: uppercase'>link text</a>-LINK TEXT\]]
-    expected: FAIL
-
-  [test_find_elements_partial_link_text[<a href=#>partial link<br>text</a>-k\\nt\]]
-    expected: FAIL
-
-  [test_find_elements_partial_link_text[<a href=# style='text-transform: uppercase'>partial link text</a>-LINK\]]
-    expected: FAIL
-
   [test_xhtml_namespace[xpath-//*[name()='a'\]\]]
     expected: FAIL