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

toHaveStyle doesn't take account of CSS custom properties #280

Open
thomaslombart opened this issue Jul 16, 2020 · 11 comments · Fixed by #285
Open

toHaveStyle doesn't take account of CSS custom properties #280

thomaslombart opened this issue Jul 16, 2020 · 11 comments · Fixed by #285
Labels
bug Something isn't working help wanted Extra attention is needed released

Comments

@thomaslombart
Copy link

thomaslombart commented Jul 16, 2020

  • @testing-library/jest-dom version: 5.1.1
  • node version: 12.16.2
  • npm (or yarn) version: 1.22.4

Relevant code or config:

test.only('failing test', () => {
  const style = document.createElement('style')
  style.innerHTML = `
    div {
      --color: blue;
    }
  `

  const {container} = render(`
    <div>
      Hello world
    </div>
  `)

  document.body.appendChild(style)
  document.body.appendChild(container)

  expect(container).toHaveStyle(`--color: blue`)
})

Problem description:

If you were to run the test above, this would fail because toHaveStyle doesn't take account of CSS custom properties.

Suggested solution:

I think the PR #276 caused this behavior. Indeed, the isSubset function has been changed and doesn't use getPropertyValue anymore (see this line).

It seems a custom property on CSSStyleDeclaration can't be accessed simply using the property key (for example computedStyle['--color']), you explicitely have to use getPropertyValue.

I forked the repo and changed back the isSubset function to use getPropertyValue and things work well. That may be a solution (I assume there's a reason why it's been changed so things may not be so simple).

@gnapse
Copy link
Member

gnapse commented Jul 16, 2020

Indeed, #276 was there for a reason, and unfortunately, we had not been covering in a test the support for these custom properties, which would've flagged this when it was introduced. The reason for #276, you can see it by checking #272, which is pretty serious and un-intuitive.

Anyway, we need to figure out how to support both use-cases. I may be able to dig into this soon, but I'll also mention here @just-boris who flagged #272 and authored the fix, because maybe they have an immediate understanding of what could be needed here.

@just-boris
Copy link
Contributor

I see two possible options here:

  1. Bring back the getPropertyValue call, while keeping the property getter:
Object.entries(styles).every(
      ([prop, value]) =>
        computedStyle[prop] === value ||
        computedStyle.getPropertyValue(prop.toLowerCase()) === value,
    )
  1. Use only getPropertyValue and write a custom camelCase to dash-case converter, based on the following code: https://github.com/jsdom/cssstyle/blob/master/lib/parsers.js#L711-L722 (sadly, it is not exported)

@gnapse
Copy link
Member

gnapse commented Jul 16, 2020

Option (1) seems reasonable enough without the extra overhead of the case converter.

@gnapse
Copy link
Member

gnapse commented Jul 16, 2020

BTW @thomlom in the mean time, while this is resolved, you need not have a fork. You can fix the version in your package.json to the one prior to the release of that recent fix.

@thomaslombart
Copy link
Author

thomaslombart commented Jul 16, 2020

Option 1 seems good to me too! Tests with a CSS custom property would need to be added of course.

@gnapse, sure! I didn't mean "forked" but "cloned" 😄

@gnapse
Copy link
Member

gnapse commented Aug 11, 2020

🎉 This issue has been resolved in version 5.11.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@AshConnolly
Copy link

AshConnolly commented Dec 22, 2021

@gnapse can we please get this issue reopened as it doesn't appear to have been resolved.
see here - #322 (comment)
PR kindly opened by @hughes-ch here - jsdom/jsdom#3299

@gnapse
Copy link
Member

gnapse commented Jan 4, 2022

If the fix lies on jsdom, then the fix on our side is to merely update the jsdom dependency, right? Which, BTW, may require an update to the jsdom dependency in dom testing library.

Anyway, I'll open this for now to reflect this situation.

@swese44
Copy link

swese44 commented Aug 28, 2023

Any update here? Thanks!

@gnapse
Copy link
Member

gnapse commented Sep 14, 2023

Not yet. I wonder if updating the jsdom dependency could help. It is at v16.x here, and it could be taken to v20 like in dom-testing-library. I'll give it a try later, but no hard promises on the deadline. If someone wants to give it a try and ping me in the PR, I'll be happy to check it out.

@Fullchee
Copy link

Fullchee commented Jan 24, 2024

Not ideal but the workaround I found (which might be helpful in figuring out the issue) was this

function App() {
  return <div style={{ "--my-property": 1 }}>Hello</div>;
}

expect(app.style._values).toMatchObject({
  "--my-property": "1", // string instead of a number
});

Reproducible repo with create react app (with the latest version of jest-dom)

(make sure to use Node 16+)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed released
Projects
None yet
6 participants