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

Conversation

@geoffrich
Copy link
Contributor

@geoffrich geoffrich commented Oct 13, 2020

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 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.

@gnapse
gnapse approved these changes Oct 14, 2020
Copy link
Member

@gnapse gnapse left a comment

@geoffrich nice idea!

It looks good, and looks safe to me.

@nickmccurdy
Copy link
Member

@nickmccurdy nickmccurdy commented Oct 21, 2020

Ready to merge?

@gnapse
Copy link
Member

@gnapse 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)

@nickmccurdy
Copy link
Member

@nickmccurdy nickmccurdy 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

@geoffrich geoffrich commented Oct 21, 2020

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.

@nickmccurdy
Copy link
Member

@nickmccurdy nickmccurdy commented Oct 21, 2020

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
3 checks passed
3 checks passed
codecov/patch 100.00% of diff hit (target 100.00%)
Details
codecov/project 100.00% (+0.00%) compared to 3fb1835
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gnapse
Copy link
Member

@gnapse gnapse commented Oct 21, 2020

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

@allcontributors
Copy link
Contributor

@allcontributors allcontributors bot commented Oct 21, 2020

@gnapse

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

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

@gnapse gnapse commented Oct 23, 2020

🎉 This PR is included in version 5.11.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

@gnapse gnapse added the released label Oct 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.