feat: make toBeVisible take into account 'hidden' attribute

[IlCallo]

Hey folks,
I noticed toBeVisible matcher ignore hidden attribute.
It may be a good idea to check that too.

[dirkluijk]

Weird. Spectator just uses jQuery for that $(el).is(':visible'), which should take hidden attribute into account.

But I know that hidden is not always really hidden, it may depend on the browsers default styling. What browser are you using for your unit test?

[IlCallo]

As you can see here, the hidden attribute is not taken into account (or, at least, it's not mentioned).

I'm using Jest as testing framework.

[dirkluijk]

I tried to reproduce (we use Jest too), but I noticed that the matchers doesn't work at all on Jest.

Found out that jsdom does not support jQuery :visible and :hidden selectors: jsdom/jsdom#1048. Let's wait for them to have it fixed.

Maybe it would work if Spectator downgrades to jQuery 1. Not sure if that is a good idea.

[IlCallo]

It doesn't seem like they are going to fix this at all...
Do you think it would be possible to create an "out-of-jquery" solution?

[dirkluijk]

We could also just change it to $(el).is(':visible') && !$(el).is('[hidden]') and $(el).is(':hidden') || $(el).is('[hidden]').

@NetanelBasal what do you think?

[NetanelBasal]

Will it work for you?

[IlCallo]

I guess yes for my use case, but it won't solve problems with jQuery and jsdom 🤔

[NetanelBasal]

You can submit a PR that replace this with native JS implementation if you want.

[IlCallo]

I'm trying to change the implementation but I would like to keep

$(el).is(':visible') && !$(el).is('[hidden]') and $(el).is(':hidden') || $(el).is('[hidden]').

for Jasmine (to keep the code cleaner and more performant) and only switch back to jQuery 1 implementation for Jest, probably something like

"hidden" !== elem.type &&
  $(elem).css("display") !== "none" &&
  $(elem).css("visibility") !== "hidden" &&
  !$(elem).is('[hidden]')

Is it possible to register different version of the matcher for the two testing frameworks?
Or is it possible to know which testing framework is running the test at runtime inside the matcher code?

Another possibility is the one proposed here which changes the getClientsRect prototype implementation to make it work keeping current implementation even in jsdom (adding the 'hidden' attribute check).

Is there a place where I can safely put that code to make it work only when tests are run with Jest?

[NetanelBasal]

I would keep it simple and go with something that works.

[IlCallo]

It depends on your perception of "something that works", given that it's always a matter of tradeoffs:

• Prototype-changing implementation theoretically works for every framework, but potentially have major performance impact;
• jQuery1 implementation detects visibility only the direct child, eg. it won't understand that children of an hidden element are hidden themselves

I can also try to use the prototype code (which checks all parents of the given element for visibility hints) directly inside the matcher if you prefer, but the code will be uglier

[NetanelBasal]

What about plain old JS?

[IlCallo]

I have nothing against plain JS (I actually prefer it over jQuery), but touching clean and working code (the one for Jasmine) is never a good idea for someone (me) which doesn't know well the codebase, expecially when there are no tests for the changed feature...

Anyway I did it with plain JS (noticing only at the end that a setup-jest file actually existed), I'll PR it soon