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

Could "it does not have its css property size(width/height) set to 0" be included in 'toBeVisible'? #177

Closed
anyexinglu opened this issue Dec 29, 2019 · 5 comments
Labels
enhancement New feature or request help wanted Extra attention is needed needs discussion We need to discuss this to come up with a good solution

Comments

@anyexinglu
Copy link

anyexinglu commented Dec 29, 2019

Describe the feature you'd like:

Sometimes, a div disappear with "transition: height 0.3s" (transition starts with height: 100px and ends with height: 0px)

Suggested implementation:

function isStyleHasNoSize(element) {
  const {getComputedStyle} = element.ownerDocument.defaultView

  const {width, height, display, visibility, opacity} = getComputedStyle(element)
  return (
    width !== '0px' && // and '0em', etc... ?
    width !== 0 &&
    height !== '0px' &&
    height !== 0 &&
  )
}

Describe alternatives you've considered:

I guess it's very difficult, even impossible, to include the rule into 'toBeVisible':
(1) A element with width: 0; height: 0; overflow: hidden may be visible:
<a href="#installation" style="width: 0; height: 0; overflow: hidden">Installation</a>

(2) A div with height: 0; width: 0; overflow: hidden; padding: 20px / height: 0; width: 0; overflow: hidden; border: 20px solid may also be visible:
<div style=" height: 0; width: 0; overflow: hidden; padding: 20px; ">Installation</div>
<div style=" height: 0; width: 0; overflow: hidden; border: 20px solid; ">Installation</div>

(3) A div with height: 0; width: 0; display: flex may also be visible:
<div style="height: 0; width: 0; display: flex">Installation</div>

Here toHaveStyle helps a little. (Although, we like toBeVisilbe since it's inherit, but toHaveStyle isn't )

@gnapse
Copy link
Member

gnapse commented Jan 31, 2020

Why the double negation when you propose this? Isn't this the same as asking that having width or height set to zero should make toBeVisible return false?

Anyway, given you recognize some difficulties yourself with making it work, given all the special cases you've considered that still do not work. I can even imagine that there could be different behaviors in this regard between browsers.

I'd rather stick with what we've got, but I'm willing to leave the discussion open here.

@gnapse gnapse added enhancement New feature or request help wanted Extra attention is needed needs discussion We need to discuss this to come up with a good solution labels Jan 31, 2020
@Archieru
Copy link

Archieru commented Feb 7, 2020

I'd also add overflow !== 'hidden' && to the list. WDYT?

@eps1lon
Copy link
Member

eps1lon commented Feb 7, 2020

TL;DR; toBeVisible should not be changed because a11y; toBeVisuallyHidden cannot be implemented correctly ins JSDOM and will add false positives to your tests. I'd recommend matching styles so that you see directly in your test what's happening.

The problem is that there are different kinds of visibility with regards to the accessibility tree. Zero width/height will still be included in the a11y tree.

From my experience these kind of elements are considered "visually hidden" not "invisible" which is used for e.g. skip links.

Another problem is that your approach is still incomplete. You would likely want to use something like offsetHeight which isn't implemented in JSDOM and therefore useless.

@anyexinglu
Copy link
Author

@eps1lon you are right, I will close it

@KevinGhadyani-Okta
Copy link

I have a component that is both height: 0 and aria-hidden="true".

In this case, I would expect .toBeVisible() to return false. It's not in the a11y tree nor is it visible from a user's persepective.

The issue is animations. You can't animate a sliding animation without width or height changes.

My code needs to be more complex to accommodate for the transitionend event (which doesn't work correctly in older Safari versions) to then set the hidden attribute on the element after the height transition animation is complete for it to work with @testing-library/jest-dom.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed needs discussion We need to discuss this to come up with a good solution
Projects
None yet
Development

No branches or pull requests

5 participants