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

Feature request: Shadow DOM support #413

Open
apalaniuk opened this issue Dec 11, 2019 · 37 comments
Open

Feature request: Shadow DOM support #413

apalaniuk opened this issue Dec 11, 2019 · 37 comments
Labels
needs discussion We need to discuss this to come up with a good solution released on @beta released

Comments

@apalaniuk
Copy link
Contributor

apalaniuk commented Dec 11, 2019

Describe the feature you'd like:

Web components are an increasingly popular way of creating reusable elements. Due to the existence of shadow DOM, user-focused testing is more difficult and requires tooling focused around the issue, as in the most permissive case, element queries/observer events (eg. via MutationObservers) stop at shadow boundaries and must explicitly be projected into shadow trees. Without support from tooling, aspects of a web app written using web components are virtually untestable without using methods contrary to the goals of this project.

Suggested implementation:

  • Standard element queries can be modified to be projected into successive shadow trees
  • Mutation observers to support eg. waits for DOM tree changes can be attached to all shadow roots of descendants
  • Solution must consider related concepts (eg. slots) insofar as they relate to testing goals and principle of least surprise according to their documented behaviour/DOM standards
  • No expectation of closed shadow roots to be supported in any way, but ideally would be explicitly documented

Describe alternatives you've considered:

  • As first-class support is required, alternatives involve using/writing an alternative implementation which supports shadow trees explicitly (or potentially, writing a plugin system to allow others to support it)
  • As shadow tree concepts are an aspect of WHATWG/W3C DOM standards, it makes sense that they're supported first-class by tooling that supports DOM operations

I'd be happy to add this feature, and support for basic functionality (query, waits) was straightforward to implement, but I wanted to get insight on constraints/concerns in advance as I don't currently use this library.

@ashubham
Copy link

+1

It will be great to build support for WebComponent based frameworks like lit-element. If there is a adapter/plugin API, I am sure the community will jump on to build it.

@alexkrolick
Copy link
Collaborator

alexkrolick commented Dec 13, 2019

How much of this depends on JSDOM itself? This library doesn't implement the DOM part, just traversal.

@apalaniuk
Copy link
Contributor Author

Hi @alexkrolick, it's a good question. JSDOM currently has shadow DOM support, with a few restrictions. The largest issue for testing right now is that this is almost entirely used in the context of web components, and support for custom elements is in-flight (jsdom/jsdom#2548).

That said, the only changes required in here for shadow DOM support should be unrelated, since this would just need to support automatically traversing through shadow trees for queries/attaching MutationObservers/etc, and shadow roots can be attached to many standard elements.

Since the overall change seems small, it's probably better to just implement in a fork and get more direct feedback based on an implementation.

@kentcdodds
Copy link
Member

A PR would be helpful. But I'm getting the impression this will be anything but a small change 😬

@ashubham
Copy link

What about when we use this library with puppeteer ? How difficult will be to have webcomponent support in that mode ?

@apalaniuk
Copy link
Contributor Author

apalaniuk commented Dec 13, 2019

@kentcdodds There's definitely a lot of complexity to work out, so I'm just being (overly) optimistic 🙂 we have done similar things at my company in the past (though only supporting a subset of the required operations), but supporting it for general use cases will take some thought (complexity to get same behaviour in traversing the light DOM given there aren't any primitives for traversal/element selection when considering shadow DOM, performance, maintainability, providing unsurprising behaviour in all cases, etc.)

@Westbrook
Copy link
Contributor

I've been toying with an approach to making this possible: adobe/spectrum-web-components#420

For me, there are two issues in particular that I'm working on:

  1. ES Module based support so that Karma + es-dev-server can run @testing-library/dom which has some downstream CJS dependencies, for which I'm rebuilding the project locally: https://github.com/adobe/spectrum-web-components/pull/420/files#diff-de467343944f9302cd195062f4d866b2 it also knocks off some Node vs browser differences
  2. Seeing into shadow DOM appropriately (normally an anti-pattern, but acceptable, IMO, in a testing environment), for which I'm using: https://github.com/adobe/spectrum-web-components/pull/420/files#diff-33d340131f49793bce88e37a2122f57c

At the intersection between the two issues is shimming @testing-library/dom to use the non-standard querySelectorAllWithShadowDOM so that the "available" DOM is queried without altering the API of the components.

Is this looking completely crazy? Are either or both of these changes something that might look interesting in a PR here?

@Westbrook
Copy link
Contributor

So, in a way, the above is also a bit of a revival of #161 as well... 😬

@kentcdodds
Copy link
Member

Hi folks,

I just am not convinced that the complexity will be worth it. You can feel free to open a PR if you like, but if it's more than a little complex I don't think I'll want it here and instead maybe you could add a custom query and even make a library of those for the people using Shadow DOM.

@kentcdodds kentcdodds added the needs discussion We need to discuss this to come up with a good solution label Jan 21, 2020
Westbrook added a commit to Westbrook/dom-testing-library that referenced this issue Jan 27, 2020
refs testing-library#413 
refs testing-library#161 

Not all environments (particularly the browser, where testing via tools like Karma occurs) have access to the `process` variable. There is already checking to ensure that its member properties have fallbacks, and this ensures that if it is simply unavailable the same fallback occurs.
@Westbrook
Copy link
Contributor

Definitely the steps for full web component support could be more complex, but ensuring that DOM targetted code can actually be tested in a real DOM environment would be a great baseline for this library to support. Then, were someone to want to extend/alter it to better support the growing capabilities of the full DOM model itself they could so with little difficulty. To that end, I think the initial steps that I'd love to see are as follows:

  • Don't rely on process.env: see fix(pretty-dom): Ensure safe process.env access #431
  • Don't rely on CJS dependencies: This one is a little more tricky (however seems to be contained to @sheerun/mutationobserver-shim at this time) and it would be better to get some insight on what is deemed acceptable before I move forward.

The best solution is to PR the library to export ESM on its own (if you were willing to support an issue/PR of that there, the voice of such a well-used and respected library would likely go a long way, so I'd be happy to open the conversation for you to jump in there), but as we've seen not everyone is excited about supporting modern APIs in their libraries. In the interim of that occuring, I'd like to pre-process @sheerun/mutationobserver-shim via an addition to the Rollup config similar to (output locations TBD, etc, etc):

    {
        input: require.resolve(
            '@sheerun/mutationobserver-shim/MutationObserver.js'
        ),
        output: {
            file: './lib/mutationobserver-shim/index.js',
            format: 'es',
        },
        plugins: [
            resolve({
                browser: true,
                preferBuiltins: false,
            }),
            commonjs(),
        ],
    },

Then it's a simple alias to slot this new output into @test-library/dom without having to affect the current contents of the JS (meaning it's plug&play when the upstream output changes!). This looks like:

            alias({
                entries: {
                    '@sheerun/mutationobserver-shim':
                        'lib/mutationobserver-shim/index.js',
                },
            }),

With these two changes, it is likely that #161 could be reopened/closed as fixed in one fell swoop and then we could have a stand-alone conversation about supporting the web component specifications. The decision as to whether that could be achieved via custom queries or should belong in @test-library/dom, in an alternate @test-library/full-dom sort of package, or be supported via an external extension of the library is an important one and should be given the focused consideration that it deserves.

Thanks in advance for your gracious insight.

kentcdodds pushed a commit that referenced this issue Jan 28, 2020
refs #413 
refs #161 

Not all environments (particularly the browser, where testing via tools like Karma occurs) have access to the `process` variable. There is already checking to ensure that its member properties have fallbacks, and this ensures that if it is simply unavailable the same fallback occurs.
@kentcdodds
Copy link
Member

Is there any chance that you could maintain a separate package that extends the capabilities here (via custom queries or similar)? If so, I would be willing to make a few smaller modifications (like the process PR I just merged of yours [thank you]) to make that easier. What do you think?

@apalaniuk
Copy link
Contributor Author

apalaniuk commented Jan 29, 2020

I haven't been able to revisit this at all, but I definitely appreciate the work others have done. I was thinking over the holidays that the relative complexity of the implementation as it trends toward "reasonable behaviour" in ways the specs don't define + possible performance implications + the relatively few users that need this in the near term means that it would likely be best as a separate package/plugin.

@Westbrook
Copy link
Contributor

Thanks for your thoughts here. I wouldn't be opposed to that, to support that path requiring as few modifications as possible could you share a little more on the following point?

As per #348 (comment) it seems like newer versions of JSDom will have caught up with browser specs as far as MutationObservers (Can I Use). That being so, do you foresee a future where you removed the @sheerun/mutationobserver-shim dependency from this library? I suppose that would technically be a breaking change, but it would prevent from needing to alter its output either at the repo level, where the project seems complete (beyond simply not accepting issues), or at the project level, via Rollup configs as outlined above.

With that change a separate package would only need to do:

replace({
    querySelectorAll: 'querySelectorAllWithShadowDOM',
}),

and put the library behind the shim for querySelectorAllWithShadowDOM which would be a dream to maintain on my end. Thanks for helping me talk through this possibilities here!

@kentcdodds
Copy link
Member

Hi @Westbrook,

I definitely have plans to remove that shim in the next breaking change (which is hopefully coming soon).

Sounds like once we get that done then you'll be in a pretty good place to make a simple package that supports shadow DOM which I think is the best path forward 👍

I'll be sure to ping you all in here when we've published the update. You're more than welcome to participate in discussion leading up to that major version bump. Hopefully that'll start in the next few days.

kentcdodds added a commit that referenced this issue Mar 4, 2020
Closes #413

BREAKING CHANGE: MutationObserver is supported by all major browsers and recent versions of JSDOM. If you need, you can create your own shim (using @sheerun/mutationobserver-shim) and attach it to the window.
kentcdodds pushed a commit that referenced this issue Mar 4, 2020
Closes #413

BREAKING CHANGE: MutationObserver is supported by all major browsers and recent versions of JSDOM. If you need, you can create your own shim (using @sheerun/mutationobserver-shim) and attach it to the window.
@kentcdodds
Copy link
Member

🎉 This issue has been resolved in version 7.0.0-beta.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

kentcdodds pushed a commit that referenced this issue Mar 4, 2020
Closes #413
Closes #357

BREAKING CHANGE: MutationObserver is supported by all major browsers and recent versions of JSDOM. If you need, you can create your own shim (using @sheerun/mutationobserver-shim) and attach it to the window.
kentcdodds pushed a commit that referenced this issue Mar 4, 2020
Closes #413
Closes #357

BREAKING CHANGE: MutationObserver is supported by all major browsers and recent versions of JSDOM. If you need, you can create your own shim (using @sheerun/mutationobserver-shim) and attach it to the window.
kentcdodds pushed a commit that referenced this issue Mar 12, 2020
Closes #413
Closes #357

BREAKING CHANGE: MutationObserver is supported by all major browsers and recent versions of JSDOM. If you need, you can create your own shim (using @sheerun/mutationobserver-shim) and attach it to the window.
@kentcdodds
Copy link
Member

🎉 This issue has been resolved in version 7.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@ashleyryan
Copy link

ashleyryan commented Jan 21, 2022

With React 18 adding support for web components behind the experimental flag, I'm wondering if testing-library has given any more thought to officially supporting the shadow dom.

@Westbrook has done some great work providing a third party library, but it's not official, poorly documented, and the NPM page for it links to the Spectrum Web Components github, not the lib directly.

More and more web component based design systems are being released using Lit or Stencil. Both frameworks provide react wrappers, but with React 18 they no longer will have to.

@kentcdodds
Copy link
Member

I think you're right Ashley... We probably should build in support for Shadow DOM. As I'm no longer an active maintainer though it's not my decision. But I think it's probably the right one at this point.

@wbern
Copy link

wbern commented Mar 31, 2022

@kentcdodds who's the maintainer now?

@kentcdodds
Copy link
Member

There's a great team of people who are maintaining the Testing Library repositories. You can check who's most recently made changes by looking at the contributing graph: https://github.com/testing-library/dom-testing-library/graphs/contributors?from=2021-08-06&to=2022-03-31&type=c

@KonnorRogers
Copy link
Contributor

KonnorRogers commented Jul 6, 2022

Working on adding tests for it, but I created a shadow DOM inspection library with a "shadow" for DOM Testing library. Still very new and need to run the paces on it.

https://github.com/ParamagicDev/shadow-dom-testing-library

@treshugart
Copy link

treshugart commented Jul 21, 2022

Is this issue talking about being able to screen.getByText('something in a shadow root') (or on an ancestor node) and have it just work for descendant shadow roots?

That would probably be a serious lift and I don't think it makes sense, either, because then you'd be testing implementation details of a component down the tree. Currently, queries will run and find light dom which should suffice.

However, when directly testing a web component that has a shadow root, there issues:

  • Awaiting a shadowRoot attachment.
  • Binding queries to the shadowRoot.
  • prettyDOM and prettyFormat.

Awaiting a shadowRoot attachment

For every component that you want to test, the upgrading and shadow root attachment may happen asynchronously on the next event loop. For this reason, one must do something like:

async function waitForShadowRoot(host) {
  return await waitFor(() => {
    if (!host.shadowRoot) {
      throw new Error(`Host does not have a shadow root.\n\n${prettyDOM(host)}`)
    }
  })
}

Binding queries to the shadowRoot

You can use within(shadowRoot) to bind queries to it and it works. However, if using TypeScript, the type expected by within is HTMLElement so you must do within((shadowRoot as unknown) as HTMLElement). I'm unsure of the ramifications of making this more permissible but it seems doing DocumentFragment | Element may be ok, at least on the surface.

prettyDOM and prettyFormat (from Jest)

Things work fine if there are no assertion errors. However, if there are, when the error messages are being formatted with prettyDOM and prettyFormat, these functions have issues understanding a node of type DocumentFragment (ShadowRoot inherits from DocumentFragment) because they use outerHTML and prettyFormat doesn't understand it (but does work with a NodeList). I got this working pretty quickly by:

  • Removing the check for outerHTML.
  • Using dom.outerHTML || dom.innerHTML when checking length.
  • Using dom.children instead of dom when passing to prettyFormat.

These could be made more robust by checking if dom is a DocumentFragment first, and then using the alternatives only if it is.


I'm happy to attempt a PR for this, but I'd like to see what others think first before spending time on something that may potentially not make its way in.

Thanks! Thoughts?

@alexkrolick alexkrolick reopened this Jul 22, 2022
@alexkrolick
Copy link
Collaborator

alexkrolick commented Jul 22, 2022

Is this issue talking about being able to screen.getByText('something in a shadow root') (or on an ancestor node) and have it just work for descendant shadow roots?

That's what Cypress (and Playwright) seem to do when you have the "pierce Shadow DOM" mode on, but I think it breaks the intended encapsulation a bit too much. I think a better solution is more like what you're describing, where entering the shadow root is explicit.

EDIT: I could see the default being to enter the Shadow DOM by default though, since users shouldn't have to know the difference.

<h1>Login</h1>
<login-form>
  <input aria-label="Email" />
</login-form>
const emailInput = await findByRole("input", { name: "Email" }) // this is a light dom child element
const submitButton = await within(await findByRole('form'), { shadow: true }) // entering the shadow dom
  .findByRole('button', {name: "Submit"}) // something in the shadow dom

You can use within(shadowRoot) to bind queries to it and it works.

Question - is that due to how JSDOM implements Shadow DOM or does it work in browsers too?

@treshugart
Copy link

treshugart commented Jul 22, 2022

I could see the default being to enter the Shadow DOM by default though, since users shouldn't have to know the difference.

Of course, my suggestion does break the ability to have "far reaching" integration tests, but these concepts ("piercing" and my proposed idea) aren't mutually exclusive. In fact, the latter complements the former and could also be considered a prerequisite.

You could go as far to say that if your test depends on being able to locate something in a shadow root of a descendant, that it's likely what you're trying to locate should either be passed via light DOM or you should be testing it in another way, such as the existence of an attribute on the descendant host. The tests for the descendant may best be left to the responsibility of that component.

A good example of this are built-in elements such as <input />. There's no way of knowing how it is implemented. Instead, we test by way of attributes, properties and events. It just so happens that with custom elements, we can see into the shadow root if we want, but just because we can does not mean we should.

Question - is that due to how JSDOM implements Shadow DOM or does it work in browsers too?

I'm unsure if you're asking about the within API or about shadow DOM behavior? The within function is a @testing-library/dom API that binds the getBy*, findBy* and queryBy* functions to a particular node. JSDOM implements shadow DOM the same way a browser would and therefore behaves the same way.

@alexkrolick
Copy link
Collaborator

JSDOM implements shadow DOM the same way a browser would and therefore behaves the same way.

That's what I wanted to confirm. There can be some implementation differences.

A good example of this are built-in elements such as . There's no way of knowing how it is implemented. Instead, we test by way of attributes, properties and events.

That is true but it represents a single node. A more complex component like a video player may have multiple interactive inner elements that implement various aria roles.

@treshugart
Copy link

treshugart commented Jul 25, 2022

A more complex component like a video player may have multiple interactive inner elements that implement various aria roles.

I see what you're saying but it doesn't change much about how we'd integration test it. Said video player should have tests of its own for its implementation details.

Similarly, composite trees such as lists and tables may have their individual elements composed of smaller pieces, but we can't test those because they're hidden to the outside world. We can, however access, a tr in a table, which would be similar to what we call light dom in custom composite elements.

Being able to see into the trees of descendant elements is convenient but it doesn't mean that it's necessarily a good thing. I often find in my own experiences that it makes testing more brittle to rely on such behavior.

@ashleyryan
Copy link

ashleyryan commented Jul 25, 2022

As a react consumer of a web-component based component library, some recurring issues where I've run into not being able to access things in the shadow DOM have often-times involved an X/close button - where a modal, or alert, or other component render the X button button in the shadow dom, instead of being passed into the components via the light dom. But I need to be able to click it to test an interaction.

If the intent of testing-library is to be able to test an app like a user would, then the shadow dom is irrelevant - there is no distinction to users.

However, because of the recursive nature of traversing the shadow dom, and because the shadow dom is not intended to be a public API, I think it's reasonable to not having it enabled by default.

@treshugart
Copy link

As a react consumer of a web-component based component library, some recurring issues where I've run into not being able to access things in the shadow DOM have often-times involved an X/close button

That's a super valid point and there are many similar use cases. The only thing I could think of right now - that isn't as easy, for sure - would be to use fireEvent.click(within(getByText('text in modal light DOM').shadowRoot).findByText('close')) or similar. At the very least, it's explicit and we can rely on shadowRoot being publicly available on the element.

@alexkrolick
Copy link
Collaborator

I've been using Web Components and I'd like to figure out how we can move forward with this.
I'll start by listing some blockers with current workarounds:

  1. PrettyDOM doesn't understand shadowRoot (it extends Node and DocumentFragment so it should work). This prevents error messages from being usable.
prettyDOM(element.shadowRoot)
//     TypeError: Expected an element or document but got ShadowRoot
  1. within(el.shadowRoot).getByText() workaround isn't recursive. This new package shadow-dom-testing-library seems to be queries that recursively search in shadow roots, if found. This could be a good solution to figure out how to add to core.

@KonnorRogers
Copy link
Contributor

KonnorRogers commented Oct 28, 2022

@alexkrolick just don't look too closely at the source. Some maniac is out here patching the ShadowRoot prototype to trick PrettyDOM...

https://github.com/KonnorRogers/shadow-dom-testing-library/blob/5029048b332d46acf5f3875a54a76d2a509cb2e0/src/index.ts#L33

my main concern is that many of these queries may start behaving differently than expected when they start recursing through shadowRoots especially in regards to "ids" and other unique attributes. This is why I added a "Shadow" prefix so as not to break the behavior of existing queries.

@alexkrolick
Copy link
Collaborator

@KonnorRogers thanks for the info. I created #1188 to track the prettyDOM changes separately. Ideally we could remove outerHTML patch.

Regarding your library, have you considering packaging it as a set of custom queries rather than an extended screen import? This may make the package more compatible with other custom extensions.

https://testing-library.com/docs/react-testing-library/setup#add-custom-queries

@KonnorRogers
Copy link
Contributor

KonnorRogers commented Nov 1, 2022

@alexkrolick I don't believe this is the best place to discuss this. There's a lot of people attached to this. If you want to open an issue on the repo itself you're more than welcome to. All that said, I do agree It may be worth providing an entrypoint only for specific queries, and then the overriden screen as a seperate entrypoint.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion We need to discuss this to come up with a good solution released on @beta released
Projects
None yet
Development

No branches or pull requests