Closed Bug 1381519 Opened 7 years ago Closed 6 years ago

Find element(s) for link text and partial link text has to work on content as rendered

Categories

(Remote Protocol :: Marionette, defect, P1)

Product:
Remote Protocol
Component:
Marionette
Type:
defect

Tracking

(firefox61 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

(Blocks 1 open bug,
URL
)

Details

(Whiteboard: [spec][17/12][wptsync upstream])

Attachments

(2 files)

minimized testcase
439 bytes, text/plain
Details
Bug 1381519 - Find element for (partial) link text has to use rendered content.
59 bytes, text/x-review-board-request
ato
: review+
Details
Attached file minimized testcase — 
Originally filed as: https://github.com/mozilla/geckodriver/issues/802

The WebDriver specification, section 13.5, says:

> The Get Element Text command intends to return an element’s text “as rendered”.

Right now a call to `find_element(By.LINK_TEXT, "FOO")` fails.

As Andreas wrote in the last comment on this issue:

> We may likely be using an outdated version of the Selenium atom (it is hard to
> update because we don’t know exactly how to build it in the format Marionette 
> expects), so it would be interesting if would be useful if you would verify that 
> this use case works in practice with the Selenium atom from upstream.
If we can’t (easily) get the Selenium atom working, we might
consider just using innerText.  Admittedly that will not be 100%
conforming to the specification, but I think it catches the same…
and more edge cases than the atom does.
Priority: -- → P2
The right place to start investigating is by looking at the task
list in Selenium (./go -T) to see if there’s a compilation target
we can use.
We also got https://github.com/mozilla/geckodriver/issues/978 filed which I was able to reproduce with the following Marionette test:

class TestMinimizedTestCase(MarionetteTestCase):

    def inline(self, doc):
        return "data:text/html;charset=utf-8,{}".format(urllib.quote(doc))

    def test(self):
        self.marionette.navigate(self.inline("<a>Self&nbsp;-&nbsp;Charles Grant</a>"))
        self.marionette.find_element(By.LINK_TEXT, "Self - Charles Grant")

So it's not only happening for CSS transformations. But we should really work on the rendered text.
Summary: Find element does not work when link text case changed by CSS text-transform → Find element for link text has to work on content as rendered
The Selenium atoms have `link_text` and `partial_link_text`, which maybe could fix that issue. Lets wait for a general bump of the existing Selenium atoms first which gets done on bug 1375660.
Depends on: 1375660
I stumbled over this today, which is actually kinda easy to fix. Will have a patch shortly.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Summary: Find element for link text has to work on content as rendered → Find element(s) for link text and partial link text has to work on content as rendered
Attachment #8934498 - Flags: review?(ato)
Whiteboard: [spec][17/12]
Comment on attachment 8934498 [details]
Bug 1381519 - Find element for (partial) link text has to use rendered content.

https://reviewboard.mozilla.org/r/205410/#review211038

Could you please clarify if this is what the specification says we
should do?

::: testing/marionette/element.js:446
(Diff revision 1)
>   *
>   * @return {Iterable.<HTMLAnchorElement>}
>   *     Sequence of link elements which text is <var>s</var>.
>   */
>  element.findByLinkText = function(startNode, linkText) {
> -  return filterLinks(startNode, link => link.text.trim() === linkText);
> +  return filterLinks(startNode, link => link.innerText === linkText);

As I understand it, this should run the bot.dom.getVisibleText atom
from Selenium and then trim the text?

::: testing/marionette/element.js:554
(Diff revision 1)
>      case element.Strategy.LinkText:
>        for (let link of startNode.getElementsByTagName("a")) {
> -        if (link.text.trim() === selector) {
> +        if (link.innerText === selector) {
>            return link;
>          }
>        }
>        return undefined;
>  
>      case element.Strategy.PartialLinkText:
>        for (let link of startNode.getElementsByTagName("a")) {
> -        if (link.text.includes(selector)) {
> +        if (link.innerText.includes(selector)) {
>            return link;
>          }
>        }
>        return undefined;

Why do these not use element.findByLinkText and
element.findByPartialLinkText?

::: testing/web-platform/tests/webdriver/tests/element_retrieval/find_element_from_element.py:57
(Diff revision 1)
> +    ("<a href=#>link text</a>", "link text"),
> +    ("<a href=#>link<br>text</a>", "link\ntext"),
> +    ("<a href=#>link&amp;text</a>", "link&text"),
> +    ("<a href=#> link text </a>", "link text"),
> +    ("<a href=#>LINK TEXT</a>", "LINK TEXT"),
> +    ("<a href=# style='text-transform: uppercase'>link text</a>", "LINK TEXT"),

I feel we’re missing a test with whitespace here.
Attachment #8934498 - Flags: review?(ato) → review-
Comment on attachment 8934498 [details]
Bug 1381519 - Find element for (partial) link text has to use rendered content.

https://reviewboard.mozilla.org/r/205410/#review211038

From the spec:

> For each element in elements:
>    Let rendered text be the value that would be returned via a call to Get Element Text for element.
>    Let trimmed text be the result of removing all whitespace from the start and end of the string rendered text.
>    If trimmed text equals selector, append element to result. 

The trimming is not necessary because `innerText` doesn't contain it. But now I see that we should get it via `getElementText`. If I have to make that change now the patch will certainly become larger. Otherwise we can file follow-up bugs. Let me know what you want.

> As I understand it, this should run the bot.dom.getVisibleText atom
> from Selenium and then trim the text?

Why should we use Selenium atoms? We want to get rid of those! Sorry, but I don't understand this comment.

> Why do these not use element.findByLinkText and
> element.findByPartialLinkText?

If both should use the same underlying code, it would have to be the other way around. But that's how it is right now. I can certainly combine those now, or similar to `Get Element Text` move this to a follow-up, maybe mentored bug.

> I feel we’re missing a test with whitespace here.

Can you please explain? What kind of test? We clearly have a whitespace test in here.
Flags: needinfo?(ato)
Comment on attachment 8934498 [details]
Bug 1381519 - Find element for (partial) link text has to use rendered content.

https://reviewboard.mozilla.org/r/205410/#review211038

The reason I’m weary of accepting the patch is because it
doesn’t implement it the way the specification says it should be
implemented, which means we need to go back and change this anyway.
What is impeding you from making it specification compatible?

> Why should we use Selenium atoms? We want to get rid of those! Sorry, but I don't understand this comment.

Did you read the specification before making this change?  The
specification says the bot.dom.getVisibleText atom should be used.

> If both should use the same underlying code, it would have to be the other way around. But that's how it is right now. I can certainly combine those now, or similar to `Get Element Text` move this to a follow-up, maybe mentored bug.

OK.

> Can you please explain? What kind of test? We clearly have a whitespace test in here.

You have a test with a line break and one with naturally collapsed
whitespace (<a href=#> link text </a>).  The whitespace in the
latter is removed during parsing.
Flags: needinfo?(ato)
Do you have time to fixup your patch?
Flags: needinfo?(hskupin)
Priority: P2 → P1
Yes, I will continue but it would happen after my mozprocess work, or in parallel while working on test results. It's number 2 on my queue.
Flags: needinfo?(hskupin)
Priority: P1 → P2
Comment on attachment 8934498 [details]
Bug 1381519 - Find element for (partial) link text has to use rendered content.

https://reviewboard.mozilla.org/r/205410/#review211038

> Did you read the specification before making this change?  The
> specification says the bot.dom.getVisibleText atom should be used.

Ok, I checked the spec now, and indeed it is listed there. Interesting that we have to keep this atom.

> You have a test with a line break and one with naturally collapsed
> whitespace (<a href=#> link text </a>).  The whitespace in the
> latter is removed during parsing.

I see. So I assume &nbsp; will do its job here then. Will get this updated.
Comment on attachment 8934498 [details]
Bug 1381519 - Find element for (partial) link text has to use rendered content.

https://reviewboard.mozilla.org/r/205410/#review235416


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: testing/marionette/element.js:445
(Diff revision 3)
>   * @return {Iterable.<HTMLAnchorElement>}
>   *     Sequence of link elements which text is <var>s</var>.
>   */
>  element.findByLinkText = function(startNode, linkText) {
> -  return filterLinks(startNode, link => link.text.trim() === linkText);
> +  return filterLinks(startNode,
> +                     link => atom.getElementText(link).trim() === linkText);

Error: Expected indentation of 6 spaces but found 21. [eslint: indent-legacy]

::: testing/marionette/element.js:463
(Diff revision 3)
>   *     Iterator of link elements which text containins
>   *     <var>linkText</var>.
>   */
>  element.findByPartialLinkText = function(startNode, linkText) {
> -  return filterLinks(startNode, link => link.text.includes(linkText));
> +  return filterLinks(startNode,
> +                     link => atom.getElementText(link).includes(linkText));

Error: Expected indentation of 6 spaces but found 21. [eslint: indent-legacy]
Attachment #8934498 - Flags: review?(ato)
Comment on attachment 8934498 [details]
Bug 1381519 - Find element for (partial) link text has to use rendered content.

https://reviewboard.mozilla.org/r/205410/#review235554

Very good tests.

::: testing/marionette/element.js:445
(Diff revision 3)
>   * @return {Iterable.<HTMLAnchorElement>}
>   *     Sequence of link elements which text is <var>s</var>.
>   */
>  element.findByLinkText = function(startNode, linkText) {
> -  return filterLinks(startNode, link => link.text.trim() === linkText);
> +  return filterLinks(startNode,
> +                     link => atom.getElementText(link).trim() === linkText);

You shouldn’t have to trim the return value form atom.getElementText
here?

::: testing/marionette/element.js:556
(Diff revision 3)
>      case element.Strategy.XPath:
>        return element.findByXPath(document, startNode, selector);
>  
>      case element.Strategy.LinkText:
>        for (let link of startNode.getElementsByTagName("a")) {
> -        if (link.text.trim() === selector) {
> +        if (atom.getElementText(link).trim() === selector) {

Same question as above with regards to trimming.
Attachment #8934498 - Flags: review?(ato) → review+
Comment on attachment 8934498 [details]
Bug 1381519 - Find element for (partial) link text has to use rendered content.

https://reviewboard.mozilla.org/r/205410/#review235554

> You shouldn’t have to trim the return value form atom.getElementText
> here?

The spec says we have to trim. As such I added the call. See step 3.2.: https://w3c.github.io/webdriver/webdriver-spec.html#link-text
Comment on attachment 8934498 [details]
Bug 1381519 - Find element for (partial) link text has to use rendered content.

https://reviewboard.mozilla.org/r/205410/#review235554

> The spec says we have to trim. As such I added the call. See step 3.2.: https://w3c.github.io/webdriver/webdriver-spec.html#link-text

OK, feel free to resolve these issues if you are confident.
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5ba7d67fddaa
Find element for (partial) link text has to use rendered content. r=ato
Priority: P2 → P1
I clearly fixed those linter failures earlier on my local box. But maybe I missed to push those to mozreview? Let me check.
Flags: needinfo?(hskupin)
Interesting is the following failure:

> TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/testing/web-platform/meta/MANIFEST.json:0 | xhr/xmlhttprequest-closing-worker.html in manifest but removed from source. (wpt-manifest)

This came in because of a manifest sync I had to trigger to get the new tests added. But in the meantime the specific test got removed again, maybe a backout. Given that I cannot rebase against this yet, I manually removed the entries for this test from the Manifest.json.
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3e204686f1b1
Find element for (partial) link text has to use rendered content. r=ato
https://hg.mozilla.org/mozilla-central/rev/3e204686f1b1
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Whiteboard: [spec][17/12] → [spec][17/12][wptsync upstream error]
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10216 for changes under testing/web-platform/tests
Whiteboard: [spec][17/12][wptsync upstream error] → [spec][17/12][wptsync upstream]
Whiteboard: [spec][17/12][wptsync upstream] → [spec][17/12][wptsync upstream error]
Whiteboard: [spec][17/12][wptsync upstream error] → [spec][17/12][wptsync upstream]
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: