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

JSDOM doesn't retrieve the "*style.backgroundColor = 'pink'" property set on a table row #1965

Open
FagnerMartinsBrack opened this issue Aug 31, 2017 · 8 comments
Labels

Comments

@FagnerMartinsBrack
Copy link

Basic info:

  • Node.js version: 8.1.2
  • jsdom version: 11.2.0

Minimal reproduction case

const { JSDOM } = require("jsdom");

const document = new JSDOM(`
    <table>
      <thead>
        <tr><th>Time</th></tr>
        <tr><th>Customer Name</th></tr>
      </thead>
      <tbody>
        <tr><td>...</td><td>Ricki</td></tr>
        <tr><td>...</td><td>Ricki</td></tr>
        <tr><td>...</td><td>Anna</td></tr>
      </tbody>
    </table>
  `).window.document;

[].slice.call(document.querySelectorAll('tr')).forEach((tr) => { return tr.style.backgroundColor = 'pink'; });

// Expected to have an array of "pink" but got an array of empty values
// Result: [ '', '', '', '', '' ]
console.log([].slice.call(document.querySelectorAll('tr')).map((tr) => { return tr.style.backgroundColor; }));

How does similar code behave in browsers?

The same code works in the browser: https://jsfiddle.net/r0u9p0uv/

Am I missing something?

@TimothyGu
Copy link
Member

We don't support getting the style property as JS properties on style for now. You would have to use tr.style.getPropertyValue(). I believe @Zirro is working on a rewrite of the entire CSSOM support in jsdom, though, which should bring support for this feature.

@FagnerMartinsBrack
Copy link
Author

@TimothyGu Very useful information. Should we keep this open until support is added?

@FagnerMartinsBrack
Copy link
Author

@TimothyGu Unfortunately, this still doesn't work on NodeJS (works in the browser):

[].slice.call(document.querySelectorAll('tr')).forEach((tr) => { return tr.style.backgroundColor = 'pink'; });

// Prints ["","","","",""]
console.log(JSON.stringify([].slice.call(document.querySelectorAll('tr')).map((tr) => { return tr.style.getPropertyValue('background-color'); })));

@TimothyGu
Copy link
Member

TimothyGu commented Aug 31, 2017

@FagnerMartinsBrack Apologies, I believe I was incorrect in my earlier evaluation of this issue. This seems to be another manifestation of https://github.com/chad3814/CSSStyleDeclaration/issues/48, in that only a subset of supported CSS colors are regarded as such by the cssstyle module we use:

https://github.com/chad3814/CSSStyleDeclaration/blob/5c51d1657f8b693d46744e05239c24ae03be57e5/lib/parsers.js#L100-L116

Until @Zirro finishes his work I'm not sure there's anything that can be done about this, as @chad3814 does not seem to be active.

@FagnerMartinsBrack
Copy link
Author

@TimothyGu So from what I understand 'pink' needs to be added to that list, is that it?

This one works in node:

[].slice.call(document.querySelectorAll('tr')).forEach((tr) => { return tr.style.backgroundColor = '#000'; });

    // Expected to have an array of "pink" but got an array of empty values
    // Result: [ '', '', '', '', '' ]
    console.log([].slice.call(document.querySelectorAll('tr')).map((tr) => { return tr.style.backgroundColor; }));

And in the browser: https://jsfiddle.net/r0u9p0uv/1/

@TimothyGu
Copy link
Member

@FagnerMartinsBrack Yes I believe so. "red" works as well, for example.

@FagnerMartinsBrack
Copy link
Author

Might be worth using the extended color list to be spec compliant: https://www.w3.org/TR/css3-color/

See on "4.3. Extended color keywords".

@FagnerMartinsBrack FagnerMartinsBrack changed the title JSDOM doesn't retrieve the "backgroundColor" property from a table row JSDOM doesn't retrieve the "*style.backgroundColor = 'pink'" property set on a table row Aug 31, 2017
@TimothyGu
Copy link
Member

Note, we always try to implement the latest Editor's Draft, so https://drafts.csswg.org/css-color/#named-colors.

@domenic domenic added the css label Oct 2, 2017
wardpeet pushed a commit to gatsbyjs/gatsby that referenced this issue Mar 27, 2019
## Description

Certain CSS styles have been failing to appear in Jest tests due to an outdated JSDom(and some it's dependencies, one being updated with bugfix in Feb 2019). This was causing false positives for `gatsby-image` tests for a while.

Nothing major, just some CSS props weren't appearing in tests when they technically should have been.

## Details

[My PR](#12468) is failing in the CI but was passing tests locally.

I had been running tests on the `gatsby-image` package on it's own, which uses `react-testing-library: ^5.0.0`, and it failed against the snapshot test. Updated snapshots failed when my PR ran on CircleCI. The main repo root yarn.lock file has `react-testing-library: ^5.2.1` locked down(retrieves 5.2.1 instead of 5.9.0, although is a v6 release now). 

Correcting this didn't help for passing the test suite. [`cssstyle: 1.2.0`](jsdom/cssstyle#74) fixes the [problems with certain styles](jsdom/jsdom#1965 (comment)) being [ignored](testing-library/react-testing-library#214. `transitionDelay`, and certain color values for `backgroundColor`, "red" works, but not "lightgray"). That however [needs `jsdom: ^12.0.0`](jsdom/jsdom@ed11465#diff-b9cfc7f2cdf78a7f4b91a753d10865a2), but [Jest refuses to update as support for Node <8 is dropped](jestjs/jest#8016 (comment))..

I've added a workaround until Jest supports a newer major version of JSDom. Using [`jest-environment-jsdom-fourteen`](https://github.com/ianschmitz/jest-environment-jsdom-fourteen) is the [advised approach](jestjs/jest#7122 (comment)) for the meantime, it'll only impact users running Jest test suite on Node 6, that should be fine for Gatsby? (Node 6 CI test seems to pass)

Only `gatsby-image` tests needed updates. [Node 6 will be EoL in May](https://github.com/nodejs/Release#release-schedule), no idea when Jest will bump JSDom from then, but we could wait a few months if you rather avoid the workaround?

## Related Issues

#12468 (Spotted it while working on this)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants