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

Handle aria-hidden attribute for children elements #162

Open
andre-matulionis-ifood opened this issue Nov 9, 2019 · 2 comments
Open

Handle aria-hidden attribute for children elements #162

andre-matulionis-ifood opened this issue Nov 9, 2019 · 2 comments

Comments

@andre-matulionis-ifood
Copy link

Originally posted as a comment for #144

Feature

.toBeVisible() is a useful matcher because it evaluates not only the current DOM node visibility, but also its parents'.

What I would like is to handle the same situation, but considering the attribute aria-hidden as visibility check

Example application

I have a carousel with copied fake slides rendered for infinite scrolling and I use aria-hidden in them.

<!-- DOM -->
<div id="carousel">
  <div class="slide fake-slide" aria-hidden="true">
    <span>Text Slide #1</span>
  </div>
  <div class="slide">
    <span>Text Slide #1</span>
  </div>
  <div class="slide fake-slide" aria-hidden="true">
    <span>Text Slide #1</span>
  </div>
</div>
// test
const slidesDOM = getAllByText(container, 'Text Slide #1')
expect(slidesDOM[0]).not.toBeVisible();
expect(slidesDOM[1]).toBeVisible();
expect(slidesDOM[2]).not.toBeVisible();

The slidesDOM array are not the .slide elements, but their span children. The tests rely on their parents being visible/not visible, so I cannot use the .toHaveAttribute('aria-hidden', "true") check.

Even though they are rendered as DOM nodes, they are not visible for most users. They are only visible if they scroll very far in a single swipe, (after which the hidden attribute is removed).

So I can't use the hidden attribute in them. The parents should be visible in the case of a long swipe, but I can't test that they are hidden for screen readers users in an easy way

Suggested implementation

The issue #144 already has a discussion about how to handle implementation of aria matchers. Some suggestions:

  • Create a set of .toBeAria* matchers, this would include .toBeAriaVisible() (or .toBeAriaHidden()
  • Handling inside the current matcher .toBeVisible().

The current state of the discussion leans toward the first suggestion of toBeAria* matchers

Alternatives

My first approach was to get the parent through .closest() and check this attribute.

const targetElement = getByText(container, 'example text');
const parentElement = targetElement.closest('.parent');
expect(parentElement).toHaveAttribute('aria-hidden', 'true');

This is very limited, since I'm looking for a specific element in the DOM tree and checking its aria-hidden attribute.

So I changed it to traverse the parents nodes using .closest() to find an aria-hidden attribute.

const targetElement = getByText(container, 'example text');
const hiddenElement = targetElement.closest('[aria-hidden="true"]);
expect(hiddenElement).not.toBe(null);

This is my current approach, but not the best. My hope with this issue is that jest-dom provide a way to make this logic cleaner.

@dimagimburg
Copy link

dimagimburg commented Feb 1, 2021

Hi, any suggestion on what is the best practice here? Right now I'm either checking for the existence of another class that is attached on the element (.hidden) or just checking
.toHaveAttribute('aria-hidden', 'true');
.toHaveAttribute('aria-hidden', 'false');

The use case btw is an element with expand-collapse button, the only thing changed is the height/offset to make it visible. I cannot use visibility: none this affects the ux.

thanks

@gnapse
Copy link
Member

gnapse commented Feb 1, 2021

I'd be open to a toBeAriaHidden only because visibility spreads down. So if you check for an element that's inside a parent with aria-hidden="true" that element will also be considered hidden even though the attribute is not on itself.

I made this clarification because in #144 there are comments about this meaning that there should then be special matchers for other aria attrs such as toBeAriaDisabled, etc. If a custom matcher of this sort is merely a syntactic substitute for toHaveAttribute('disabled') then I do not see value in such a dedicated matcher. But the one about aria visibility is more complex than that.

I am not sure I have time these days to add it, but in the mean time we always welcome contributions. So if anyone's up to it, let us know.

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

3 participants