Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Switch to Shadow DOM command #1320

Closed
wants to merge 1 commit into from

Conversation

AutomatedTester
Copy link
Contributor

This allows the ability to move between a Shadow DOM context and the
document context.

If there is no Shadow DOM to switch to an error
No Such Shadow DOM is returned.

This allows the ability to move between a Shadow DOM context and the
document context.

If there is no Shadow DOM to switch to an error
No Such Shadow DOM is returned.
return <a>error</a> with <a>error code</a> <a>no such shadow dom</a>.

<li><p>Set the <a>current browsing context</a> to <var>element</var>’s
<a><code>shadowRoot</code></a>.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the shadowRoot type? What if it is closed/open?

@@ -1325,6 +1326,12 @@ <h3>List of Endpoints</h3>
<td><a>Switch To Parent Frame</a></td>
</tr>

<tr>
<td>POST
<td>/session/{<var>session id</var>}/shadow
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs to be on the element

</tr>
<tr>
<td>POST</td>
<td>/session/{<var>session id</var>}/shadow</td>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be on a web element namespace.

<li><p>Run the substeps of the first matching condition:

<dl class=switch>
<dt><var>id</var> is <a>null</a>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The id will always be defined if it’s a web element.

@justinfagnani
Copy link

justinfagnani commented Oct 26, 2018

The switching API for Shadow DOM is really suboptimal and adds a lot of unnecessary overhead to testing Web Components. A ShadowRoot is not an iframe, nor even like an iframe, and it shouldn't have a similar API.

Please see my objections in #5869: SeleniumHQ/selenium#5869 (comment)

@jlipps
Copy link
Contributor

jlipps commented Oct 26, 2018

So far I think @justinfagnani has a good point; I'm curious what your response would be, @andreastt and @AutomatedTester.

@jgraham
Copy link
Member

jgraham commented Oct 26, 2018

This was briefly discussed in the working group meeting today. I think there was some agreement that this might not be the best API approach, but some thought is needed about how interactability checks should work with shadow trees.

@christian-bromann
Copy link
Member

I am curious how easy it is for browser vendors to find an element within a shadow DOM. In the CDP I don't find any method like querySelectorAll that includes finding an element within a shadow DOM. Not sure about Gecko. I see two scenarios:

  1. We include searching in shadow DOMs when using the findElement command and leave it up to the browser driver to also check within every shadow DOM of every element on the page that has one.
    • Pro: almost no work for WebDriver bindings to add this functionality
    • Con: heavy work for driver to look into every shadow DOM to find the element
  2. We do it as proposed here and let the WebDriver bindings ensure that they implement it correctly
    • Pro: more efficient and easier to implement for drivers
    • Con: WebDriver bindings need to implement an API that takes a selector for the element containing the shadow DOM and the selector to find the element within the shadow DOM.

The question is also whether or not we need to switch context in order to execute an action on an shadow DOM element. This can also add a lot of work on the client libraries having to flag elementIds as shadow elements or non shadow elements and having to perform a context switch before using that element id.

TLDR: depending on how easy it is for browser vendors to find a shadow dom and execute an action on it we should either abstract it away or have the WebDriver bindings have to implement an abstraction on top of it.

@jgraham
Copy link
Member

jgraham commented Oct 29, 2018

Looking inside shadow doms should be an explicit action because it's breaking encapsulation. It should be quite acceptable to treat a <my-button> element with associated shadow dom as a single entity which can be interacted with just like a UA-defined element. But it should also be possible to pierce the veil and interact with the implementation of the component when that's a necessary thing to do. I don't think that implemenation complexity is the overriding concern here, but presenting an API that is both easy to use and has the desired featureset. The problem with the model where you scope all commands to a session-global shadow DOM is that it fails the test of being easy to use. The counter-proposal (roughly) is to have two possible web-elements corresponding to a DOM node with a shadow tree; one represeting the host document view of the element, and one representing the shadow root. So you would have an endpoint like /session/{session id}/element/shadow which would return a Shadow Root Web Element. Passing the element id of that shadow root web element to /session/{session id}/element/{element id}/element would allow searching for elements within the shadow tree (which would then return normal web element references that could be passed to other commands).

@christian-bromann
Copy link
Member

christian-bromann commented Oct 29, 2018

How about enhancing existing commands, e.g.

  • /session/{session id}/element: accepting a third parameter shadow(default: false) that if set to true returns an Shadow Root Web Element Reference, if that element doesn't have a shadow dom but shadowDom is set to true we would return null like we would do when not finding an element at all.
  • /session/{session id}/element/{element id}/element: that accepts as element id a web element reference or shadow root web element reference

This could allow the driver to differentiate between both contexts and element types without putting more complexity to the binding implementations. If an action is then called on a shadow root web element reference the context can be switched by the driver automatically. This would avoid confusion when user forget to switch back from a shadow dom context and wonder why they can not click on any other element anymore.

@andreastt
Copy link
Member

andreastt commented Oct 29, 2018 via email

@jgraham
Copy link
Member

jgraham commented Oct 29, 2018

If an action is then called on a shadow root web element reference the context can be switched by the driver automatically.

Note that in the proposal there is no session-global state associated with shadow DOMs. The change is that for elements that represent a shadow root, there are two associated Web Elements, one that represents the shadow host and cannot see inside the shadow dom, and one that represents the shadow root and can. Once you have a web element for an element inside a shadow DOM, it's assumed that you can use it just like one for an element inside the host document.

That said, I could get behind adding parameters to the existing endpoints rather than adding new endpoints. One deciding factor there could be how the fallback works in existing implementations. In the case where there's a separate endpoint to get the shadow root then old implementations will fail explicitly. In the case that there's a parameter, it will be ignored by old implementations and they will silently have incorrect behaviour.

@christian-bromann
Copy link
Member

thanks for clarifying @jgraham @andreastt

One deciding factor there could be how the fallback works in existing implementations

Are there already existing implementations for that endpoint? I was under the assumption that there aren't any. A brief research resulted that many implementations are allowing actions in the shadow dom by using the execute command.

@jgraham
Copy link
Member

jgraham commented Oct 29, 2018

Are there already existing implementations for that endpoint?

I meant the Find Element endpoint(s), which are implemented. If the way to get the shadow root is to call /session/{session id}/element with data {"using": "css selector", "value": "my-element", "shadow": true} then an implementation with no support for getting shadow roots would silently return the my-element shadow host rather than the associated shadow root, which would be incorrect behaviour. If instead we make the endpoint /session/{session id}/element/shadow (or something) with the data {"using": "css selector", "value": "my-element"} then the method would return unsupported command in non-supporting implementations. But the usability of the first does seem superior so I could be convinced it's the right thing to do.

@christian-bromann
Copy link
Member

then an implementation with no support for getting shadow roots would silently return the my-element shadow host rather than the associated shadow root

Yes, which is why the default is set to false which results in all implementation won't change their behavior at all. Having this default also make sense because in 95% of all use cases you are not interested in looking into the shadow dom (this number can change if web components become more popular).

@jgraham
Copy link
Member

jgraham commented Oct 29, 2018

The default doesn't matter. The question is what happens in an existing implementation in the case that a user specifically requests looking at the shadow dom. With a new parameter, that parameter is ignored and we return the wrong result. With a new endpoint, that endpoint returns a 404 and an unsupported command exception.

@AutomatedTester
Copy link
Contributor Author

The one reason why this can't be on findElement directly is we need to take into account that if people are expecting a shadowRoot and don't get one then we need to handle that case. We also don't have any mechanisms on the web platform to allow us to penetrate shadow DOM from a find. This has been removed (see w3c/csswg-drafts#640). I don't want WebDriver to go do it's own thing here.

The other reason for doing this as an explicit switch is that all interaction commands need to know the context of where it is executing. For example:

<html>
  <my-list>
    <my-button>
       <fancy-list-that-is-data-driven>
    </my-button>
  </my-list>

If we have a mechanism that allows us to traverse down to fancy-list-that-is-data-driven when we do try do a click we will get either a stale element reference or element click intercepted since currently we do this from document where with each shadow DOM we need to set the context to shadowRoot. This means we need to have a similar context to where findElement is starting.

In Web API terms if we do document.elementsFromPoint(...) we get the top most element with a shadowRoot and then we need to move to that and redo the query. Doing it without explicit transitions means we could start adding recursive code to find what we mean which would be less than ideal.

@jgraham
Copy link
Member

jgraham commented Oct 29, 2018

We also don't have any mechanisms on the web platform to allow us to penetrate shadow DOM from a find.

That doesn't matter; the proposed API doesn't depend on the ability to do anything other than select a shadow host from css (which is necessarily possible since it's just a normal element) and get a reference to its associated shadow root (which is possible via content APIs for the case of an open shadow root and must be possible from browser-internal APIs for a closed shadow root since that's required for the page to actually function).

If we have a mechanism that allows us to traverse down to fancy-list-that-is-data-driven when we do try do a click we will get either a stale element reference or element click intercepted since currently we do this from document where with each shadow DOM we need to set the context to shadowRoot

Interaction APIs will presumably start from the element's nearest shadow root for the purposes of click. So in your example I would have to write code like:

var myList = webdriver.select("my-list", shadowRoot=true);
var myButton = myList.select("my-button", shadowRoot=true);
var myFancyList = myButton.select("my-fancy-list-which-is-data-driven");
myfancyList.click()

(in practice you would chain the calls rather than making lots of intermediates at least in languages that support that style, so I don't think the programmer burden of this is too high). Note that this is basically exactly the same as you could do today using script APIs to access the contents of open shadow hosts:

var myListHost = webdriver.select("my-list");
var myList = webdriver.execute("return arguments[0].shadowRoot", myListHost);
var myButtonHost = myList.select("my-button");
var myButton = webdriver.execute("return arguments[0].shadowRoot", myButtonHost);
var myFancyList = myButton.select("my-fancy-list-which-is-data-driven");
myfancyList.click()

@andreastt
Copy link
Member

andreastt commented Oct 29, 2018 via email

@christian-bromann
Copy link
Member

we need to handle that case

This could be handled similar to if no element was found.

I don't want WebDriver to go do it's own thing here

I am not sure how to interpret this. I see that we are aligned to some web APIs (e.g. querySelector -> findElement or querySelectorAll -> findElements) but in many cases we already do our own "thing". Aligning to existing standards is definitely good but if that means it would add unnecessary complexity I would avoid it.

This means we need to have a similar context to where findElement is starting.

I think this can be easily solved when we say that every shadow root web element reference is connected to another web element reference or shadow root web element reference. With that the driver can easily resolve all contexts as shown in @jgraham example.

@gsnedders
Copy link
Member

Looking inside shadow doms should be an explicit action because it's breaking encapsulation.

I'm super strongly +💯 on this, to the extent I'll probably object to doing anything else.

/session/{session id}/element/shadow seems a bit weird to me: what exactly would it do? Find an element and return its shadow root?

To me it makes more sense to have /session/{session id}/element/{element id}/shadow which returns a Shadow Root Web Element (which is just a special type of Web Element, and Find Element from Element etc. work normally on it; or null) for the given element id, splitting the finding and returning the shadow root into separate actions.

I also think the harder part is defining element interactibility (given a <input type=text> should still be considered interactable if it's within an shadow tree).

@justinfagnani
Copy link

I want to point out that in the standards groups the assumption has been that testing frameworks will provide a means to pierce ShadowRoots in searches, which is why it was ok to remove the shadow-piercing selectors like /deep/. I can try to dig up minutes if you like. I know that it was discussed at TPAC 2017 in the Web Components breakout.

For open ShadowRoots, the encapsulation isn't intended to be strong enough to disallow code that knows that it's doing from reaching in. It's possible, and definitely not discouraged for testing, to write a querySelectorDeep(selectors: string[]) function that recurses through ShadowRoots applying each selector in turn.

@jgraham
Copy link
Member

jgraham commented Oct 29, 2018

/session/{session id}/element/{element id}/shadow looks more like the DOM APIs, but typically has more overhead since if you find an element then you have to make a second call to get the shadow root. It has the advantage that it works when you return a (closed) shadow host from a script and want to get into the shadow dom.

For reference CDP seems to do something more like "give me the children of this node, including any shadow children", which does make sense for a debugger, but doesn't seem to map well to WebDriver since WebDriver only has selection by (various kinds of) selector not direct DOM traversal.

@gsnedders
Copy link
Member

gsnedders commented Oct 29, 2018

I guess my suggestion is entirely redundant, anyway, given wd.execute_script("return arguments[0].shadowRoot", element) will do the same. Oh well. I don't know well enough what makes sense here.

Given @justinfagnani said the Web Platform WG expected testing frameworks to provide a means to pierce shadow roots in searches, it would seemingly imply we either need to alter the semantics of CSS/XPath selectors or invent a new API to do something else?

@justinfagnani
Copy link

alter the semantics of CSS/XPath selectors

One option is to just implement /deep/ and ::shadow. I think Ryosuke Niwa from Apple has a userland implementation of that somewhere, but it's not too difficult. Another is to take a list of selections, with an implied /deep/ between each.

@jgraham
Copy link
Member

jgraham commented Nov 1, 2018

To be clear, if elementsFromPoint doesn't pierce into shadow DOMs, we have a problem with the definition of interactable irrespective of the model we use for switching into the shadow tree. So that's definitely a problem that needs to be addressed writing the spec here, but afaict it doesn't obviously place additional constraints on how this should work.

@justinfagnani
Copy link

The consensus from CSSWG is that we should not be allowed to pierce shadow DOMs that are closed. This is why piercing queryselectors were dropped.

That's closed ShadowRoots. The vast majority are open.

@justinfagnani
Copy link

Having test frameworks use useland shadow-piercing selector libraries was discussed at TPAC 2017, and several other times in in-person conversations:

Ryosuke: We have lots of test APIs in WebKit.
... Turns out a lot of apps end up using those test APIs.
... That's not good, not thread safe.
... If we expose it in static profile, someone's going to use it in production somewhere.
... Maybe we should standardize polyfill or something (for test frameworks).

https://www.w3.org/2017/11/10-webplat-minutes.html

@AutomatedTester
Copy link
Contributor Author

The consensus from CSSWG is that we should not be allowed to pierce shadow DOMs that are closed. This is why piercing queryselectors were dropped.

That's closed ShadowRoots. The vast majority are open.

The point is the piercing would break encapsulation.

@coreyfarrell
Copy link

What is the desired outcome of getText() run on an element which only contains shadowRoot? Currently chromedriver returns the text contents from within the shadowRoot (including recursively), geckodriver returns an empty string. Minimum example is posted at mozilla/geckodriver#1417.

@andreastt
Copy link
Member

The behaviour in relation to Get Element Text is somewhat undefined. However the browser implements shadow DOM, the expected outcome should be whatever running bot.dom.getVisibleText as defined in https://github.com/SeleniumHQ/selenium/blob/e09e28f016c9f53196cf68d6f71991c5af4a35d4/javascript/atoms/dom.js#L981 gives.

Get Element Text is horribly underdefined because we couldn’t reach agreement on what constituted visible text.

@coreyfarrell
Copy link

Even so I if I have:

const fragment = '<div>any html fragment</div>';
document.getElementById('test1').innerHTML = fragment;
document.getElementById('test2').attachShadow({mode:'open'}).innerHTML = fragment;

I think getText should return the same value for test1 and test2. Even if different elements of W3C disagree on what constitutes visible text, each browser should agree with itself. Putting fragment in a shadowRoot does not change the element’s text “as rendered”.

@gsnedders
Copy link
Member

Neither textContent nor innerText get anything from shadow trees, do they? Should we expect Get Element Text to differ?

@coreyfarrell
Copy link

They do not. I think textContent is not worth comparing since it returns the contents of non-visible childNodes such as <style>.

In reality I'm unable to get rendered text from within a shadow tree from any browser, only chromedriver Get Element Text does what I need. innerText returns a blank string on both Chrome and Firefox. As is a test helper would be needed to reimplement the logic of innerText by iterating ele.shadowRoot.childNodes and ignoring non-visual and hidden elements. Unfortunately innerText does not return blank when requested directly from non-visual childNodes so the following prints div{display:none}test1test2 to the console twice.

ele.attachShadow({mode: 'open'}).innerHTML = '<style>div{display:none}</style><div>test1</div>test2'
console.log(Array.from(ele.shadowRoot.childNodes).map(e => e.innerText || e.wholeText).join(''));
console.log(ele.shadowRoot.textContent);

I'm not sure what the correct answer is to how Get Element Text should respond for elements containing shadow trees. I can understand the argument for not wanting the existing command to pierce shadow trees, but I also feel that the ability to do so (somehow) is important for testing. I tried using Take Element Screenshot with a test that compared the screenshot to expected (previous) image, unfortunately the fonts on Travis-CI are slightly different than my workstation so the screenshots cannot match. I can't think of any other options.

@jgraham
Copy link
Member

jgraham commented Nov 6, 2018

I think the behaviour of get element text should be a separate issue. This issue should be about how to access elements in the shadow tree, and the other issue about how to access text under the shadow tree. Depending on the constraints we find, the solutions may be unrelated (i.e. if it's possible to update Get Element Text to at least optionally return the shadow tree's text, that won't depend at all on the ability to access elements in the shadow tree).

@jgraham
Copy link
Member

jgraham commented Nov 6, 2018

Filed #1350 for the Get Element Text issue.

@AutomatedTester
Copy link
Contributor Author

I have raised WICG/webcomponents#771

@lukebjerring
Copy link

@AutomatedTester - what's the status on this?
Ran into this issue myself testing wpt.fyi, which I worked around using execute script of arguments[0].shadowRoot.querySelector('foo') to fetch foo from a web-component element.

@AutomatedTester
Copy link
Contributor Author

AutomatedTester commented Jan 10, 2019 via email

@jrobinson01
Copy link

jrobinson01 commented Feb 8, 2019

I'm not sure this is the best place for this, but here goes anyway. I'm using webdriverio and have added this simple custom command:

browser.addCommand('getShadowRoot', function(el) {
  return browser.execute(el => el.shadowRoot ? el.shadowRoot : el, el.value);
});

Usage is sort of clunky:

get myButton() {
    browser.getShadowRoot(browser.element('my-container')).element('my-button');
}

but I think it could be much nicer, if in webdriverio it were a getter function on the element object returned by browser.element('selector'). This would allow the following usage (as part of a page object in webdriverio for example):

// get the custom element
get myElement() {
    return browser.element('my-element');
}
// query inside it's shadowRoot
get myButton() {
    return this.myElement.shadowRoot.element('my-button');
}

The last example basically lines up with how things work inside the browser:

document.querySelector('my-element').shadowRoot.querySelector('my-button');

edit: reread the thread. looks like @gsnedders suggested something very similar already. I still really like the idea of explicitly getting the shadowRoot (which my command does NOT do, it returns the element if it can't find a shadowRoot).

@osmolyar

This comment has been minimized.

@junguchina
Copy link

Is there any update here?Who can push this pr?

@osmolyar

This comment has been minimized.

@letsgotomars
Copy link

letsgotomars commented May 8, 2020

Any updates for us on the status of this PR @AutomatedTester, @shs96c, or @jgraham? It's been awhile since this PR started and it would be nice to know what the path forward is.

I feel that this is a very important issue that needs to be addressed in order for developers to truly adopt and embrace Shadow DOM seriously. Shadow DOM enabled web components are one most exciting and powerful features of the modern web, but sadly if developers cannot properly or easily write tests for them without going through kludgy or complex workarounds then their adoption will be crippled since the cons will outweigh the pros in many cases. More on this here.

@AutomatedTester
Copy link
Contributor Author

Any updates for us on the status of this PR @AutomatedTester, @shs96c, or @jgraham? It's been awhile since this PR started and it would be nice to know what the path forward is.

I feel that this is a very important issue that needs to be addressed in order for developers to truly adopt and embrace Shadow DOM seriously. Shadow DOM enabled web components are one most exciting and powerful features of the modern web, but sadly if developers cannot properly or easily write tests for them without going through kludgy or complex workarounds then their adoption will be crippled since the cons will outweigh the pros in many cases. More on this here.

There was an offer from Salesforce to tackle this.

This is really not something the WebDriver spec should be tackling as it should have been handled by the DOM spec and all the work around web components. We are trying to rectify the issue now.

@AutomatedTester
Copy link
Contributor Author

Closing this for now. This feature will be added but hopefully Salesforce will do it soon

@letsgotomars
Copy link

letsgotomars commented Jun 10, 2020

Thanks so much for the update @AutomatedTester!

This is really not something the WebDriver spec should be tackling as it should have been handled by the DOM spec and all the work around web components. We are trying to rectify the issue now.

It's great to hear this is being actively worked on! How can I track the progress of this DOM/web components work and the aforementioned Salesforce team work? Are there relevant issues/PRs that we can reference?

@leobalter
Copy link

Plugging some updates here.

We do have a merged PR for Supporting Shadow DOM, and respective WPT support. Thanks @jimevans!

It seems that we still need browser implementations to catch up with the changes. @letsgotomars I believe the next step is looking at how we can help browser vendors now the tests have landed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.