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.

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.

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.

I stumbled over this today, which is actually kinda easy to fix. Will have a patch shortly.

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.

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.

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.

Do you have time to fixup your patch?

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.

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]

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.

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

Backed out changeset 5ba7d67fddaa (bug 1381519) for linting failures at builds/worker/checkouts/gecko/testing/marionette/element.js on a CLOSED TREE Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=5ba7d67fddaac0f9caafb0886ce8b96e55bba6b4&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=169502087&repo=autoland&lineNumber=236 Backout: https://hg.mozilla.org/integration/autoland/rev/620491c60697cfc8eaa1e24a0125bd726acd72fa

I clearly fixed those linter failures earlier on my local box. But maybe I missed to push those to mozreview? Let me check.

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

Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10216 for changes under testing/web-platform/tests

Upstream PR merged