WebDriver Input clear/value commands fails when target is inside shadow dom

• Component: WebDriver
• Status: RESOLVED FIXED

[Bnaya]

Created attachment 393856 [details]
Failing selenium webdriver code

When sending input commands to input element inside shadow dom,
The commands fails with
"WebDriverError: An unknown error occurred: A JavaScript exception occured: Argument 1 ('element') to Window.getComputedStyle must be an instance of Element"


A repro snippet is attached.
you will need to install https://www.npmjs.com/package/selenium-webdriver package.

The snippet tested to work works in chrome, firefox

Debugging into the node code, the commands `/{element-id}/clear`, `/{element-id}/value` returning that error from the safari side


these errors happens with Safari 12 & 13

[linkgoron]

Created attachment 409609 [details]
Patch

[linkgoron]

Just a note that the isShadowDescendent was kind of a guess... I got the element is not interactable error, went over the code to see where it was thrown, and the only path that made sense to me that would cause the shadow dom issue was the isDescendentOf path. 

Regarding the ElementIsDisplayed change, that's also how the implementation is in Selenium (https://github.com/SeleniumHQ/selenium/blob/trunk/javascript/atoms/domcore.js#L167)

[BJ Burg]

Comment on attachment 409609 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409609&action=review

This code change looks good, but it lacks a new test. I'll try running the WPT suite with this change to see if it progresses any failures.

> Source/WebCore/ChangeLog:8
> +        Added support for nested shadow DOM in isDescendantOrShadowDescendantOf,

Reword: "Add support for nested shadow DOM elements in Node::isDescendantOrShadowDescendantOf()."

> Source/WebCore/ChangeLog:9
> +        and exposed it for use in the WebDriver

Reword: delete this line.

> Source/WebCore/ChangeLog:11
> +        Did not add a new test.

If there's no new test for this specifically, mention which test(s) exercise the behavior already if you know.

> Source/WebCore/dom/Node.h:378
> +    bool isDescendantOrShadowDescendantOf(const Node* other) const { return other && isDescendantOrShadowDescendantOf(*other); } 

This is unusual. Typically we change all call sites at once rather than adding more wrappers.

> Source/WebKit/UIProcess/Automation/atoms/ElementDisplayed.js:30
> +        return !!node && node.nodeType === Node.ELEMENT_NODE

Nit: needs a trailing newline.

> Source/WebKit/UIProcess/Automation/atoms/ElementDisplayed.js:41
> +        for (let node = targetNode; node && node !== targetNode.getRootNode(); node = node.parentNode)

This is essentially changing the parent node search from composed to not-composed. I think that's okay because this is used to determine containment/obscured elements, not to do actual hit testing like elementsFromPoint.

[linkgoron]

Thanks for the review!

Regarding the isDescendantOrShadowDescendantOf new refence overload, I just wanted to change it so that it will be consistent with isDescendantOf which also has similar reference and pointer overloads. Do you want me to change other calls, or just remove the overload that I added?

Regarding tests - I'll look around and see how to add tests.

In addition to what I added, I saw that there's a missing check for Shadow DOM active elements with the focus check in the send keys command (a check which exists in ChromeDriver). In the WebKit implementation it only checks the outer active element, while in ChromeDriver it also checks inside of the shadow DOM. Is it out of scope or should I also add it?

I'll fix everything once you give me the clear that your testing passed (or, I'll fix what's needed if you encounter any issues).

[BJ Burg]

(In reply to linkgoron from comment #4)
> Thanks for the review!
> 
> Regarding the isDescendantOrShadowDescendantOf new refence overload, I just
> wanted to change it so that it will be consistent with isDescendantOf which
> also has similar reference and pointer overloads. Do you want me to change
> other calls, or just remove the overload that I added?

Oh, I totally missed that. I think your change is appropriate, then.

> 
> Regarding tests - I'll look around and see how to add tests.

This didn't cause any WPT regressions which is great!

As for adding tests, it's easiest to make a new test in the WPT format. The top-level directory for WebDriver is here:

https://github.com/web-platform-tests/wpt/tree/master/webdriver

I suggest you check out the wpt repository, create the test, then try to run against Chrome/Firefox and Safari to ensure that it passes and fails as expected.

> In addition to what I added, I saw that there's a missing check for Shadow
> DOM active elements with the focus check in the send keys command (a check
> which exists in ChromeDriver). In the WebKit implementation it only checks
> the outer active element, while in ChromeDriver it also checks inside of the
> shadow DOM. Is it out of scope or should I also add it?

I'd prefer this is a separate patch. That way it's easier to upstream separate tests to WPT, or to amend the specification to include these essential-but-missing checks.

Thanks, and feel free to ping me on WebKit Slack (see webkit.org for details).

[linkgoron]

Created attachment 410426 [details]
Patch

[BJ Burg]

Comment on attachment 410426 [details]
Patch

r=me, thanks for your help!

[EWS]

Committed r267978: <https://trac.webkit.org/changeset/267978>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 410426 [details].

[Radar WebKit Bug Importer]

<rdar://problem/69960660>