JSDOM-react adapter should implement isDisplayed

[NitayRabi]

Currently isDisplayed implementation under JSDOM adapter returns true.

Source
Method should test element visibility similar to -

Protractor

(e.g Test existence, size over 0 X 0 and display !== 'none')

[GabiGrin]

I agree it shouldn't return true
the problem with jsdom is that you don't have proper measuring so you can't know

thus, I prefer to either:

1. throw an error
2. always return false

Thoughts? any other options we can do in JSDom?
@borisd9 @hepiyellow

[hepiyellow]

1. Certainly throwing an Unsupported Error is better than returning false. 2. I this ,again, that jsdom is a special case. And we should have both a React adapter and a React/JSDom adapter. 2.a after such separation, i think jsdom adapter is the only adapter that we should allow to throw Unsupported Error.

…

On 2019. Mar 10., at 16:07, Gabriel Grinberg ***@***.***> wrote: I agree it shouldn't return true the problem with jsdom is that you don't have proper measuring so you can't know thus, I prefer to either: throw an error always return false Thoughts? any other options we can do in JSDom? @borisd9 @hepiyellow — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[GabiGrin]

@hepiyellow what does it mean a "react" adapter alone? what environment does it run on?

[dor-itzhaki]

what about implementing this https://github.com/testing-library/jest-dom#tobevisible ?

[dor-itzhaki]

or just something like:

function isVisible (ele) {
    var style = window.getComputedStyle(ele);
    return  style.width !== "0" &&
    style.height !== "0" &&
    style.opacity !== "0" &&
    style.display!=='none' &&
    style.visibility!== 'hidden';
}

[GabiGrin]

@dor-itzhaki height and width might be 0 on valid cases too, but I am OK with implementing the same logic the did on testing-library

[liorcode]

Any progress on that? It took me a while to realize how come my tests always fail :( just when I used the debugger, I found this weird behaviour.
I think throwing an exception is much better than this unexpected behaviour, or going with the testing-library logic

[GabiGrin]

@liorcode no, a PR should be fairly easy, the challenge will be coordinating this release with all users of WSR and ensuring nothing breaks unexpectedly

[liorcode]

hmm I see. did you consider using some flag+warning for the new behavior and then removing it in the next major version of WSR? this is what WSR usually do for breaking changes.
e.g.

isDisplayed(realCheck = false) {
  if (!realCheck) { 
    console.warn('Warning: using mocked implementation of 'isDisplayed' which always returns true. This will change in the next version. Pass "true" to really check')
  }
}

[GabiGrin]

@liorcode the warning is great meanwhile, can be added now.
As for the fix, I prefer following semver and release a new major of the react jsdom adapter and have WSR upgrade at will

@shlomitc thoughts?

[shlomitc]

I went through our code and didn't see anywhere we're using the isDisplayed() method in our code, so probably this will not break us.
@liorcode - where did this happen to you?
Overall I'm ok with both warning and error, as long as it is clear and displayed well in the logs.

[liorcode]

It happened for me when I used my own drivers, not WSR. I did some tests to make sure some element was not displayed, and I was very surprised when it failed. took me a while to realize that isDisplayed returned true for no reason :)

[AlmogAdziashvili]

@GabiGrin
According to jest-dom doc:

An element is visible if all the following conditions are met:

- it does not have its css property display set to none
- it does not have its css property visibility set to either hidden or collapse
- it does not have its css property opacity set to 0
- its parent element is also visible (and so on up to the top of the DOM tree)

what do you think?

[GabiGrin]

@AlmogAdziashvili Sounds good