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: add custom element support to toBeDisabled #368

Merged
merged 4 commits into from
Feb 3, 2022

Conversation

astorije
Copy link
Contributor

@astorije astorije commented May 21, 2021

What:

Allow toBeDisabled to match against custom elements as supported by the WHATWG spec.

Why:

Rationale and example failure + reproduction repository available at #367

How:

This is a draft PR I opened directly from the GitHub UI. In other words, it could not get less tested than that. I'm just opening this to see if you agree with the premise. If you do, I'll add tests, docs, etc. properly. Done.

Checklist:

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

expect(queryByTestId('disabled-custom-element')).not.toBeDisabled()
}).toThrowError('element is disabled')

expect(queryByTestId('enabled-custom-element')).not.toBeDisabled()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This currently fails with:

Screen Shot 2021-06-04 at 1 16 52 PM

I can't figure this out: clearly the custom element is not disabled. More to the point, if I remove the disabled custom element (L122) long with the corresponding assertions (L126-129), the test now passes. It's as if some state was not being reset or something. @gnapse, do you have any idea?

(The same thing happens on the other test L281-L284)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In other words, splitting the 2 new tests into 4 tests (see diff below) passes. I could do that, but I would prefer to figure out first if this is a bug with the test or with the code: if I split them and it passes, nothing ensures this wouldn't happen in real-life tests that have a mix of disabled and non-disabled custom elements.

Diff
@ src/__tests__/to-be-disabled.js:120 @ test('.toBeDisabled fieldset>legend', () => {
   expect(queryByTestId('outer-fieldset-element')).toBeDisabled()
 })

-test('.toBeDisabled custom element', () => {
+test('.toBeDisabled disabled custom element', () => {
   const {queryByTestId} = render(`
-    <custom-element data-testid="disabled-custom-element" disabled="" />
-    <custom-element data-testid="enabled-custom-element" />
+    <custom-element data-testid="custom-element" disabled="" />
   `)

-  expect(queryByTestId('disabled-custom-element')).toBeDisabled()
+  expect(queryByTestId('custom-element')).toBeDisabled()
   expect(() => {
-    expect(queryByTestId('disabled-custom-element')).not.toBeDisabled()
+    expect(queryByTestId('custom-element')).not.toBeDisabled()
   }).toThrowError('element is disabled')
+})
+
+test('.toBeDisabled enabled custom element', () => {
+  const {queryByTestId} = render(`
+    <custom-element data-testid="custom-element" />
+  `)

-  expect(queryByTestId('enabled-custom-element')).not.toBeDisabled()
+  expect(queryByTestId('custom-element')).not.toBeDisabled()
   expect(() => {
-    expect(queryByTestId('enabled-custom-element')).toBeDisabled()
+    expect(queryByTestId('custom-element')).toBeDisabled()
   }).toThrowError('element is not disabled')
 })

@ src/__tests__/to-be-disabled.js:275 @ test('.toBeEnabled fieldset>legend', () => {
   }).toThrowError()
 })

-test('.toBeEnabled custom element', () => {
+test('.toBeEnabled disabled custom element', () => {
   const {queryByTestId} = render(`
-    <custom-element data-testid="disabled-custom-element" disabled="" />
-    <custom-element data-testid="enabled-custom-element" />
+    <custom-element data-testid="custom-element" disabled="" />
   `)

-  expect(queryByTestId('disabled-custom-element')).not.toBeEnabled()
+  expect(queryByTestId('custom-element')).not.toBeEnabled()
   expect(() => {
-    expect(queryByTestId('disabled-custom-element')).toBeEnabled()
+    expect(queryByTestId('custom-element')).toBeEnabled()
   }).toThrowError('element is not enabled')
+})
+
+test('.toBeEnabled enabled custom element', () => {
+  const {queryByTestId} = render(`
+    <custom-element data-testid="custom-element" />
+  `)

-  expect(queryByTestId('enabled-custom-element')).toBeEnabled()
+  expect(queryByTestId('custom-element')).toBeEnabled()
   expect(() => {
-    expect(queryByTestId('enabled-custom-element')).not.toBeEnabled()
+    expect(queryByTestId('custom-element')).not.toBeEnabled()
   }).toThrowError('element is enabled')
 })

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW I am not wrapping the elements in a div/fragment because the render function does it on its own, but the error is the same if I do.

@astorije
Copy link
Contributor Author

astorije commented Jun 8, 2021

Hey @gnapse, do you think you could help me with this one? 🙏 Thank you so much in advance!

@astorije astorije marked this pull request as ready for review June 9, 2021 15:13
@astorije
Copy link
Contributor Author

astorije commented Aug 3, 2021

Hi @gnapse, any chance you could help getting this over the finish line? We'd love to get unstuck!

Thanks!

@gnapse
Copy link
Member

gnapse commented Aug 3, 2021

Sorry @astorije for the lack of responsiveness here on my side.

Can you remind me what's blocking this right now?

@astorije
Copy link
Contributor Author

astorije commented Aug 5, 2021

Hi @gnapse, no worries!
That was some time ago so I don't remember all the details, but I documented above what I was stuck on - tests - and was hoping you could help me figure it out. I'll take another look tomorrow or Friday but if you stop by and have intuitions, that'd be welcome!

@astorije
Copy link
Contributor Author

astorije commented Feb 1, 2022

Hi @gnapse, friendly ping? We would really love to be able to use toBeDisabled in our projects :)

@gnapse
Copy link
Member

gnapse commented Feb 1, 2022

@astorije sorry for having neglected this.

I took a deeper look now. Even checking this branch out locally and running the tests.

Unfortunately, I have not much of an idea of what's going on. My main bet is that this may have something to do with some shortcoming in the support for custom elements in jsdom. Maybe you should investigate that.

My knowledge of custom elements is also not too good. I cannot confidently point out to anything that could be wrong in the PR changes per-se.

@ashleyryan
Copy link

ashleyryan commented Feb 2, 2022

@astorije @gnapse this is a straight forward fix. Per the spec, custom elements cannot be self closing. So for the second (enabled) element, it was treating the first (disabled) element as it's parent, and the logic says an element is disabled if it's ancestor is. Change the test code to have a closing tag.

See "Rules on creating custom elements": https://developers.google.com/web/fundamentals/web-components/customelements.

@codecov
Copy link

codecov bot commented Feb 2, 2022

Codecov Report

Merging #368 (afdc230) into main (3094eb1) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #368   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           26        26           
  Lines          621       624    +3     
  Branches       230       231    +1     
=========================================
+ Hits           621       624    +3     
Impacted Files Coverage Δ
src/to-be-disabled.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 3094eb1...afdc230. Read the comment docs.

@astorije
Copy link
Contributor Author

astorije commented Feb 2, 2022

Aaaah thank you @ashleyryan, silly me! It looks like CI is passing now 😍

@gnapse gnapse merged commit 8162115 into testing-library:main Feb 3, 2022
@gnapse
Copy link
Member

gnapse commented Feb 3, 2022

@all-contributors please add @astorije and @ashleyryan for code, idea

@allcontributors
Copy link
Contributor

@gnapse

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

@gnapse
Copy link
Member

gnapse commented Feb 3, 2022

@all-contributors please add @ashleyryan for code, idea

@allcontributors
Copy link
Contributor

@gnapse

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

@gnapse
Copy link
Member

gnapse commented Feb 3, 2022

@astorije @ashleyryan thanks!

@github-actions
Copy link

github-actions bot commented Feb 3, 2022

🎉 This PR is included in version 5.16.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@astorije astorije deleted the patch-1 branch February 3, 2022 01:24
@astorije
Copy link
Contributor Author

astorije commented Feb 3, 2022

Thank you @gnapse and @ashleyryan!!

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