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.

Sign up for GitHub

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

Jump to bottom

getComputedStyle does not return the right visibility sometimes #3182

Closed
romain-trotard opened this issue Apr 29, 2021 · 4 comments · Fixed by #3195
Closed

getComputedStyle does not return the right visibility sometimes #3182

romain-trotard opened this issue Apr 29, 2021 · 4 comments · Fixed by #3195

Comments

@romain-trotard
Copy link
Contributor

Hello guys

Basic info:

  • Node.js version: 14.16.0
  • jsdom version: 16.4.0

Minimal reproduction case

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

const dom = new JSDOM(`
  <html lang="en">
    <head>
      <title>Document</title>
      <style>
        #hiddenDiv {
            visibility: hidden;
        }
        
        /* This rule is important because add rule on the div */
        * {
            margin-bottom: 10px;
        }
      </style>
    </head>
    <body>
      You should not see anything below me
      <div id="hiddenDiv">You do not see me</div>
    </body>
  </html>`);

const { document } = dom.window;

const hiddenDiv = document.getElementById('hiddenDiv');

// Visibility is visible instead of hidden
console.log(dom.window.getComputedStyle(hiddenDiv)._values.visibility);

I also made a repository but not sure if it will be useful (because using react + styled-components + testing-library): /~https://github.com/romain-trotard/jsdom-visibility-styled-components-problem

How does similar code behave in browsers?

I just made a screenshot in a browser:

image

Reason of the problem

I have debugged jsdom and have found where the problem is.
Actually the problem is here:

jsdom/lib/jsdom/living/helpers/style-rules.js

Line 63 in 04f6c13

value = rule.style.getPropertyValue(property);

The reason is that next rules override the value of previous rule.
I have made a fix here /~https://github.com/romain-trotard/jsdom/tree/getCascadedPropertyValue-change

image

getPropertyValue returns an empty string "" if the property is not found in the rule (/~https://github.com/NV/CSSOM/blob/97bee0949153ecb3295eccf98da2be79b7269e2f/lib/CSSStyleDeclaration.js#L31)

If it's ok with you I can make a PR :)

Thanks
@el-ethan
Copy link

el-ethan commented May 27, 2021 •
edited
Loading

I am having an issue that I think is related to this same thing. I have a component that sets visibility to hidden in some conditions. The component is working fine in the browser, but after updating jest (which brought along an update to jsdom), my unit tests for visibility have started to fail. It appears that when a component has its styles modified by a styles defined in a parent component, visibility is overridden, even when the parent component doesn't define any visibility styles. I set up a minimal example here.

@domenic
Copy link
Member

domenic commented May 27, 2021

https://mobile.twitter.com/slicknet/status/782274190451671040

@romain-trotard
Copy link
Contributor Author

I will make a PR soon, for this issue if it's ok for you @domenic

@romain-trotard romain-trotard mentioned this issue May 27, 2021
Merged
@romain-trotard
Copy link
Contributor Author

@el-ethan I have try my fix with your reproduction repo and it seems to work for your problem too ;)

@TimothyGu TimothyGu linked a pull request Jun 16, 2021 that will close this issue
Do not override propertyValue in getCascadedPropertyValue if "" #3195
Merged
domenic pushed a commit that referenced this issue Aug 1, 2021
@romain-trotard
Fix some bad cascade computation in getComputedStyle()
0dedfc0 
Closes #3182.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Do not override propertyValue in getCascadedPropertyValue if "" romain-trotard/jsdom
3 participants
@domenic @el-ethan @romain-trotard