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

[FEATURE] toBeVisible to see height: 0 as "not visible" #450

Closed
KevinGhadyani-Okta opened this issue Mar 31, 2022 · 2 comments
Closed

[FEATURE] toBeVisible to see height: 0 as "not visible" #450

KevinGhadyani-Okta opened this issue Mar 31, 2022 · 2 comments

Comments

@KevinGhadyani-Okta
Copy link

KevinGhadyani-Okta commented Mar 31, 2022

Describe the feature you'd like:

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

In this case, I would expect .toBeVisible() to return false. The component is both not in the a11y tree nor is it visible to a user'.

There is a closed ticket about this same topic: #177

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 accessibility tree.

This is true, except I'm correctly using aria-hidden="true", so it's not there.

I can't use anything but height: 0 because of animations. You can't do a sliding animation without width or height changes. And if I want to hide something from view, height: 0 does it.

If I want to do sliding without height changes, then my code needs to be more complex when visually, the item isn't there.

Suggested implementation:

We can't look for only height: 0 because that doesn't hide it from the accessibility tree; although, if aria-hidden is true, then , we should be safe checking if the height is 0 for toBeVisible.

Describe alternatives you've considered:

Testing the wrong things

Instead of using .not.toBeVisible(), I can look for the styles on the element styles, but this is a hack that shouldn't be used when "visibility" is the semantic definer. I don't care if the height is 0 as a user, I care if I can see it or not.

Significantly increasing the component's complexity to workaround @testing-library/jest-dom

  1. Listening to the transitionend event (which doesn't work correctly in older Safari versions).
  2. When triggered, set a local state value to configure the hidden attribute on an element.
  3. Add another useEffect for when the value changes to open and immediately set the local state in that case.
  4. Add a hidden prop to the HTML element and only set it when we're in the local closed state.

This code significantly increases complexity:

// ...
const [
  localVisibility,
  setLocalVisibility,
] = useState(
  // This value comes from a context provider and should be what I'm using for the `hidden` attribute, but I cannot because animations.
  visibility
);

const contentRef = useRef();

useEffect(
  () => {
    if (
      visibility
      === Visibilities.opened
    ) {
      setLocalVisibility(
        visibility
      );
    }
  },
  [
    visibility,
  ]
);

useEffect(
  () => {
    const onTransitionEnd = () => {
      // This should be the "closed" state.
      setLocalVisibility(
        visibility
      );
    };

    const contentElement
      = contentRef
        .current;

    contentElement
      .addEventListener(
        'transitionend',
        onTransitionEnd
      );

    return () => {
      contentElement
        .removeEventListener(
          'transitionend',
          onTransitionEnd
        );
    };
  },
  [
    visibility,
  ]
);
// ...

Teachability, Documentation, Adoption, Migration Strategy:

Add a segment to the docs explaining what qualifies for toBeVisible as not aria-hidden="true" & height: 0.

@KevinGhadyani-Okta KevinGhadyani-Okta changed the title **[FEATURE]** toBeVisible to see height: 0 as "not visible" [FEATURE] toBeVisible to see height: 0 as "not visible" Mar 31, 2022
@aquelehugo
Copy link

aquelehugo commented Apr 2, 2022

There are lots of cases where elements with height: 0 are visible, as the OP described in the issue you linked.

I made a pen to make it more visual:
https://codepen.io/aquelehugo/pen/RwxjGaz

The documentation already talks about what it supports:
https://github.com/testing-library/jest-dom#tobevisible

Explaining why specific cases where height: 0 does hide a element are not covered seem a bit verbose for that quick docs. It would be more like a whole document dedicated to that.

Handling all the cases is not complex: it is impossible. If you wanted something like that in JSDOM, they would have to render fonts just to check which size would your element have with text inside. The issue about that is more than 10 years old there: jsdom/jsdom#135

So Testing Library is right to not support height: 0 handling here. Supporting it sometimes is worse than not supporting it at all.

Now about your transition: you could use opacity: 0.
If you don´t want opacity to raise gradually, you could create a CSS animation that has opacity: 1 at the 1% keyframe step.
Also, don´t use height for transitions/animations if you care about performance.

Examples:
With transition (CSS only): https://codepen.io/aquelehugo/pen/ZEvaBQV
With animation (CSS + JS): https://codepen.io/aquelehugo/pen/LYeOxxz

@KevinGhadyani-Okta
Copy link
Author

KevinGhadyani-Okta commented May 3, 2022

I've decided against height: 0 being invisible. Even though it's technically invisible to the user, it still allows you to [TAB] through items inside.

I rewrote my code to add a hidden attribute when the height-changing animation completed. This was pretty complicated to do, but it worked. The hidden attribute is the only real way to fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants