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

fix: treat shadow DOM elements as in the document #298

Merged
merged 1 commit into from
Oct 21, 2020
Merged

fix: treat shadow DOM elements as in the document #298

merged 1 commit into from
Oct 21, 2020

Conversation

geoffrich
Copy link
Contributor

What:

expect(element).toBeInTheDocument() would return false when element was in a shadow root. It now returns true.

Why:
I want toBeInTheDocument to treat elements in a shadow root as in the document. I am experimenting with using Testing Library on apps that use custom elements.

How:
toBeInTheDocument originally checked that element.ownerDocument.contains(element) was true. However, this returns false if the element being checked is within a shadow root.

I changed the function to use getRootNode instead. getRootNode will return the element's root (usually either the document or the element's shadow root). Passing {composed: true} will return the document instead of any shadow roots, so we can compare this to element.ownerDocument to see if the element is in the document.

All existing tests pass with these changes, but let me know if you can think of any edge cases I missed.

Checklist:

  • Documentation N/A
  • Tests
  • Updated Type Definitions N/A
  • Ready to be merged

I am not sure what the browser support requirements are for this library, but getRootNode does not work in IE11 and non-Chromium Edge. If that's a problem I can work on an alternate approach.

@codecov
Copy link

codecov bot commented Oct 13, 2020

Codecov Report

Merging #298 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #298   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           23        23           
  Lines          315       315           
  Branches        72        72           
=========================================
  Hits           315       315           
Impacted Files Coverage Δ
src/to-be-in-the-document.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3fb1835...e0c7ad5. Read the comment docs.

Copy link
Member

@gnapse gnapse left a comment

Choose a reason for hiding this comment

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

@geoffrich nice idea!

It looks good, and looks safe to me.

@nickserv
Copy link
Member

Ready to merge?

@gnapse
Copy link
Member

gnapse commented Oct 21, 2020

yup, I forgot about it.

One thing: I'm not 100% this is a patch release (a fix). Isn't this potentially breaking tests to others that may have had stuff under shadow DOM that were not found? (apologies if this makes little sense, I'm not 100% familiar with shadow dom)

@nickserv
Copy link
Member

nickserv commented Oct 21, 2020

That depends on whether we consider not discovering shadow DOM elements as a bug from the previous version.

@geoffrich
Copy link
Contributor Author

It's possible that someone has a test depending on this behavior, but I think it's unlikely. Testing Library queries do not currently return any elements in the shadow DOM -- they use document.querySelectorAll under the hood, which only returns elements in the light DOM. So you would have to query the shadow root directly (not using any testing library queries) to get the element you're looking for, and then expect it not to be in the document.

Here's an example with the custom element I defined in the tests:

const customElement = document.querySelector('custom-element');
const shadowDiv = customElement.shadowRoot.querySelector('div');
expect(shadowDiv).not.toBeInTheDocument(); // passed before my commit, and will now fail

So it's potentially a breaking change for that kind of test, but that test seems contrived -- what's the point of expecting the shadowDiv to not be in the document? It was a brittle test, because it would have also passed if querySelector returned nothing. If they were doing something like this, it's more likely that they would expect shadowDiv to be in the document (as I did) and be confused when it didn't work.

That was my rationale for treating this as a bug and not a new feature. However, I don't care too much one way or another, so I can reword the commit if you like.

@nickserv
Copy link
Member

It is a fairly specific example and I agree it's contrived, so it sounds like a bug to me, and therefore not a breaking change. I'm interested in what other maintainers think about it though.

@gnapse gnapse merged commit 7ee54ab into testing-library:master Oct 21, 2020
@gnapse
Copy link
Member

gnapse commented Oct 21, 2020

@all-contributors please add @geoffrich for code, test, idea, bug

@allcontributors
Copy link
Contributor

@gnapse

I've put up a pull request to add @geoffrich! 🎉

@geoffrich geoffrich deleted the pr/shadow-in-document branch October 21, 2020 15:19
@gnapse
Copy link
Member

gnapse commented Oct 23, 2020

🎉 This PR is included in version 5.11.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

3 participants