Closed Bug 1824664 Opened 11 months ago Closed 3 months ago

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

Categories

(Remote Protocol :: Marionette, defect, P1)

Firefox 109
defect
Points:
5

Tracking

(firefox-esr102 unaffected, firefox-esr115 fix-optional, firefox111 wontfix, firefox112 wontfix, firefox113 wontfix, firefox114 wontfix, firefox115 wontfix, firefox116 wontfix, firefox117 wontfix, firefox120 wontfix, firefox121 wontfix, firefox122 fixed)

RESOLVED FIXED
122 Branch
Tracking Status
firefox-esr102 --- unaffected
firefox-esr115 --- fix-optional
firefox111 --- wontfix
firefox112 --- wontfix
firefox113 --- wontfix
firefox114 --- wontfix
firefox115 --- wontfix
firefox116 --- wontfix
firefox117 --- wontfix
firefox120 --- wontfix
firefox121 --- wontfix
firefox122 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

(Regression, )

Details

(Keywords: regression, Whiteboard: [webdriver:m9], [wptsync upstream][webdriver:relnote])

Attachments

(4 files, 1 obsolete file)

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"

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.

Regressed by: 1771942

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.

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.

Flags: needinfo?(david.burns)

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.

Flags: needinfo?(david.burns)
Summary: Since Firefox 109 the command "WebDriver:GetElementText" returns an empty value for an element within a ShadowRoot on https://taskmob.demo.vaadin.com → Selenium atoms update caused "WebDriver:GetElementText" to return an empty value for an element within a ShadowRoot on https://taskmob.demo.vaadin.com

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.

Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Points: --- → 3
Priority: -- → P1
Whiteboard: [webdriver:m6]

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

Whiteboard: [webdriver:m6] → [webdriver:m7]

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

For more information, please visit auto_nag documentation.

Flags: needinfo?(hskupin)

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

Whiteboard: [webdriver:m7] → [webdriver:m8]

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

Assignee: hskupin → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(hskupin)
Priority: P1 → P2
Whiteboard: [webdriver:m8] → [webdriver:backlog]

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.

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.

Assignee: nobody → hskupin
Status: NEW → ASSIGNED

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.

Summary: Selenium atoms update caused "WebDriver:GetElementText" to return an empty value for an element within a ShadowRoot on https://taskmob.demo.vaadin.com → "WebDriver:GetElementText" returns an empty value for an element within a ShadowRoot

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.

Summary: "WebDriver:GetElementText" returns an empty value for an element within a ShadowRoot → Selenium atoms update caused "WebDriver:GetElementText" to return an empty value for an element within a ShadowRoot

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.

Attached file Testcase (obsolete) —

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.

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.

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.

Summary: Selenium atoms update caused "WebDriver:GetElementText" to return an empty value for an element within a ShadowRoot → Selenium atoms update caused "WebDriver:GetElementText" to return an empty value for elements that are part of a ShadowRoot`s slot
Attached file Testcase
Attachment #9363578 - Attachment is obsolete: true
Attachment #9363680 - Attachment description: Bug 1824664 - [marionette] Update usage of Selenium atoms for window global. → Bug 1824664 - [marionette] Evaluate Selenium atom code in content and not in privileged code.

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.

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.

Points: 3 → 5

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.

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.

Whiteboard: [webdriver:backlog] → [webdriver:backlog][DO NOT LAND BEFORE 122 VERSION BUMP]
Attachment #9363681 - Attachment description: Bug 1824664 - [wdspec] Add "Get Element Text" test for ShadowRoot slot. → Bug 1824664 - [wdspec] Add "Get Element Text" tests for ShadowRoot with slot.
Severity: -- → S3
Priority: P2 → P1
Whiteboard: [webdriver:backlog][DO NOT LAND BEFORE 122 VERSION BUMP] → [webdriver:m9][DO NOT LAND BEFORE 122 VERSION BUMP]
Whiteboard: [webdriver:m9][DO NOT LAND BEFORE 122 VERSION BUMP] → [webdriver:m9]
Pushed by hskupin@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → 122 Branch
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/43282 for changes under testing/web-platform/tests
Whiteboard: [webdriver:m9] → [webdriver:m9], [wptsync upstream]
Upstream PR merged by moz-wptsync-bot
No longer blocks: 1865359
Regressions: 1865359
Whiteboard: [webdriver:m9], [wptsync upstream] → [webdriver:m9], [wptsync upstream][webdriver:relnote]
You need to log in before you can comment on or make changes to this bug.