1824664 - Selenium atoms update caused "WebDriver:GetElementText" to return an empty value for elements that are part of a ShadowRoot`s slot

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P1)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Originally filed as: https://github.com/mozilla/geckodriver/issues/2097

I can reproduce the problem with Marionette and release version of Firefox that this regression happened between the 108 and 109 release of Firefox. Sadly when using Nightly builds of Firefox even the 108 release fails. Maybe there is a Nightly-only pref that enables a specific feature on Nightly only which finally got released in Firefox 109.

We should check which feature caused this regression. Until then I'm going to file the bug in our Marionette component.

To reproduce the bug you can run the following Marionette test:

    def test(self):
        self.marionette.navigate("https://taskmob.demo.vaadin.com/")
        pdb.set_trace()
        view = self.marionette.find_element(By.TAG_NAME, "tasks-view")

        button = self.marionette.execute_script("""
            return arguments[0].shadowRoot.querySelector("#newTaskButton");
        """, script_args=[view])

        assert button.text == "New task"

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Here the result of the mozregression run:

12:14.42 INFO: Test command result: 0 (build is good)
12:14.42 INFO: Narrowed integration regression window from [96a6cdbc, 1ce1fc25] (4 builds) to [50bef32e, 1ce1fc25] (2 builds) (~1 steps left)
12:14.42 INFO: No more integration revisions, bisection finished.
12:14.42 INFO: Last good revision: 50bef32e268d050b14427daf73efeff5b80163c2
12:14.42 INFO: First bad revision: 1ce1fc25ab484d8df9732979a85d31a8fc62d822
12:14.42 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=50bef32e268d050b14427daf73efeff5b80163c2&tochange=1ce1fc25ab484d8df9732979a85d31a8fc62d822

So this most likely regressed by the Selenium atoms upgrade via bug 1771942. Hereby bot.dom.getVisibleText seems to have been regressed.

What's still unclear to me is why this doesn't show up for the 108 release given that the change landed in Firefox 107. It would be good to know which preference might have started to trigger it on release/beta.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

An interesting starting point could be that 108.0b1 is failing while 108.0b9 is passing. I'm going to bi-sect the releases to figure out which feature change got reverted during the beta cycle for the 108 release.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1771942

:david.burns, since you are the author of the regressor, bug 1771942, could you take a look?

For more information, please visit auto_nag documentation.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

While Firefox 108.0b6 is still failing the beta 108.0b7 is passing. Note that with the beta 7 we also switch from early to late beta.

Note that I'm going to investigate that one.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

FYI, backing out the patch from bug 1771942 fixes this issue. It would be good to know what exactly has been changed for the Selenium atom between these two changesets.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

So the atoms got updated from the source on Selenium in javascript/atoms/dom.js from the changeset facd199c31c9d81316a1db3bbb0c10603799bb8b to a6b161a159c3d581b130f03a2e6e35f577f38dec.

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:whimboo, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1771942

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Right now I'm not actively working on this bug.

Comment 10

[mleppich]

This affects testing of web components which also rely on shadow dom. Our team at adobe is trying to migrate from playwright to webdriverio in order to test with real firefox releases and this issue forces us to use workarounds to get the innerText of web components via executeScript commands.

See https://github.com/webdriverio/webdriverio/issues/11360 for more details on the issue we are facing.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I'll have a look at it again. If we cannot easily find the regressor in the Selenium repository we might want to revert the getElementText atom changes temporarily.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thanks to the testcase from mleppich, I'm more easily able to reproduce the problem - means without manually intervention. Here the Marionette test that can be used:

    def test(self):
        self.marionette.navigate("https://opensource.adobe.com/spectrum-web-components/storybook/iframe.html?id=button-primary-fill-sizes--m&args=&viewMode=story")
        button = self.marionette.find_element(By.CSS_SELECTOR, "#root-inner > sp-story-decorator:nth-child(2) > sp-button:nth-child(1)")

        assert button.text == "Click Me"

Can see that whatever revision we have for the Selenium atoms it doesn't make a difference with Firefox 102ESR. Both variants are working fine. On the other side it always fails with a recent Firefox also independent of the atoms.

That basically means that a feature has been enabled in Firefox 108 which breaks the atoms to retrieve the text. Also it won't help to revert the atoms to an older version because it's still failing.

Next step would be to check in which Firefox Nightly this problem appeared first.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Interestingly I was wrong in my last comment. When I checked that I only reverted the atom code for Get Element Text and that didn't help. But reverting everything in that file makes it work.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The regressing changeset that actually broke the Get Element Text atom is the following:

https://github.com/SeleniumHQ/selenium/commit/9e5016d660bdec00eb7179c7344284862307231c#diff-53bbf9a76b3e18a714284bd37ffdbcb670cbf537319cdaee94aacca31bcc497d

I've filed https://github.com/SeleniumHQ/selenium/issues/13132 to get this further investigated and fixed.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I got very useful feedback from the reported Selenium issue and as we figured out the problem is actually not in Selenium but in our code, and related in how we execute the atom. When I'm using the compiled atom code in a standalone HTML page, it works as well and the text can correctly be retrieved. But when it is run through Marionette as privileged code it fails.

I'm going to continue now with the non compiled and not minified code from https://jsfiddle.net/xmhr2cjg/, which hopefully can give us an idea why it doesn't work. At least I will be able to use a debugger now and step through the code.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Testcase

With that testcase we can verify that the text can be retrieved. Also using the embedded getElementText() method via Execute Script and no sandbox selected gives me valid results as well.

So maybe it's a sandbox issue overall? I tried with unwaiveXRays for the element, but that doesn't make a difference as well.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Screenshot of ShadowRoot instance check

Ok, so the problem here is actually the instance check for the ShadowRoot in appendVisibleTextLinesFromNodeInComposedDom_(). If the code is run with content privileges, as that usually happens when a web page executes the code, it works just fine but within a privileged scope it evaluates to falsy and executes the wrong subsequent methods:

Call stack for content scope:

 *** appendVisibleTextLinesFromElementInComposedDom_

 **** node: [object HTMLElement]

 *** appendVisibleTextLinesFromNodeInComposedDom_

 **** node: [object HTMLDivElement] type: 1 shown: true

 *** appendVisibleTextLinesFromElementInComposedDom_

 **** node: [object HTMLDivElement]

 *** appendVisibleTextLinesFromElementCommon_

 *** isNodeDistributedIntoShadowDom

 *** appendVisibleTextLinesFromNodeInComposedDom_

 **** node: [object HTMLSlotElement] type: 1 shown: true
 **** found slot
 **** parent: [object HTMLDivElement]
 **** parent: [object ShadowRoot]
 **** parent is shadowroot

 *** appendVisibleTextLinesFromNodeInComposedDom_

 **** node: [object Text] type: 3 shown: true

 *** appendVisibleTextLinesFromTextNode_

 *** appendVisibleTextLinesFromElementCommon_

 *** isNodeDistributedIntoShadowDom

Call stack for privileged scope:

 *** appendVisibleTextLinesFromElementInComposedDom_

 **** node: [object HTMLElement]

 *** appendVisibleTextLinesFromNodeInComposedDom_

 **** node: [object HTMLDivElement] type: 1 shown: true

 *** appendVisibleTextLinesFromElementInComposedDom_

 **** node: [object HTMLDivElement]

 *** appendVisibleTextLinesFromElementCommon_

 *** isNodeDistributedIntoShadowDom

 *** appendVisibleTextLinesFromNodeInComposedDom_

 **** node: [object HTMLSlotElement] type: 1 shown: true
 **** found slot
 **** parent: [object HTMLDivElement]
 **** parent: [object ShadowRoot]

 *** appendVisibleTextLinesFromElementInComposedDom_

 **** node: [object HTMLSlotElement]

 *** appendVisibleTextLinesFromElementCommon_

 *** appendVisibleTextLinesFromElementCommon_

 *** isNodeDistributedIntoShadowDom

It's not clear to me why the instance check fails and why using our internal method ShadowRoot.isInstance() works fine.

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Interesting. I can make it work when using window.ShadowRoot. Then our privileged code works as well. So maybe the default ShadowRoot global that we actually have available in privileged code is not usable for instance checks in content? I have to add that by default the code as run in this atom doesn't have a global window, and as such we fallback to that other global.

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Testcase

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1824664 - [marionette] Evaluate Selenium atom code in content and not in privileged code.

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1824664 - [wdspec] Add "Get Element Text" tests for ShadowRoot with slot.

Depends on D193650

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+1]]

After discussion we decided to move the evaluation of the Selenium atoms from the privileged code into the content scope. That means the current patch is using a Sandbox, which itself makes the window global available to the code. So far this all works fine except the case when the ShadowRoot has mode closed. Then we currently fail and the atom getVisibleText returns an empty string.

Surprisingly the returned text is not empty when I use the stripped down and not compiled version of the atom from the testcase as attached. So I wonder if this is some breakage as caused by the compilation or some other change as explicitly done for the testcase. I'll have to ask over on https://github.com/SeleniumHQ/selenium/issues/13132 from which version of the atom the code has been taken from.

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I'm currently discussing a fix in Selenium to get the visible text for a closed Shadow DOM. The actual problem is here:
https://github.com/SeleniumHQ/selenium/blob/3041af31ccfd7b2b459051e547246def83688870/javascript/atoms/dom.js#L590-L594

Most likely this check should only be performed for an open shadow root, and otherwise we might fallback to the default code path. It might be better as to not supporting that atom at all in a closed Shadow DOM.

If we agree to it I'll patch the Selenium atom, and also would have to update the spec for the new atom commit, and pull the new atoms. Because of that we agreed on to bump the points to 5.

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

As the try build shows the extra check for parent.mode === "open works as expected and allows all the tests for closed Shadow DOMs to pass:
https://treeherder.mozilla.org/jobs?repo=try&revision=ab35411d9e3bb37b3cedd2c712811a5bb83b361e

I'll create a PR for Selenium now and get tests added. I hope that this will be approved.

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Note that we are going to push this patch as is by Monday after the next mozilla-central to mozilla-beta merge. That will give us 4 weeks to get remaining issue with the Selenium atom fixed.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a8e68cba552a
[marionette] Evaluate Selenium atom code in content and not in privileged code. r=webdriver-reviewers,jgraham
https://hg.mozilla.org/integration/autoland/rev/8c758adbe3cb
[wdspec] Add "Get Element Text" tests for ShadowRoot with slot. r=webdriver-reviewers,jgraham

Comment 27

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/a8e68cba552a
https://hg.mozilla.org/mozilla-central/rev/8c758adbe3cb

Comment 28

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

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

Comment 29

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Lets not fix for ESR 115 because the number of affected users is actually pretty low.