Skip to content

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Sep 28, 2019

Looking for better names of this function.

What:

  • Export helper method that is used in byRoles queries to exclude inaccessible elements

Why:

  • To implement a custom test matcher like expect(element).toBeInaccessible()
    This is useful if you want to test relationships between elements which are established via aria-controls. We want to test that the value of aria-controls is valid by checking that expect(document.getElementById(control.getAttribute('aria-controls'))).toBeInTheDocument() but we want to make sure that at that point the element is still excluded from the a11y tree.

    Another use case might be if you queried the element with something other than byRole and you want to make sure the element is (in)accessible.

How:

  • "just" export it

Checklist:

@eps1lon eps1lon added the enhancement New feature or request label Sep 28, 2019
Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure of the best name either. Maybe simply isHidden?

@eps1lon
Copy link
Member Author

eps1lon commented Oct 7, 2019

I'm not sure of the best name either. Maybe simply isHidden

What do you think about isInaccessible? (Although I'd be fine with the current name since it is currently coupled to the spec and I'm not sure we'll ever want to expand what it does).

isHidden has a visual implication which does not apply to aria-hidden or "visually hidden" (i.e. content with zero-width, zero-opacity etc).

@alexkrolick
Copy link
Collaborator

🤔 Could include the word ARIA in the name to indicate its relationship to that spec: isAriaHidden, isAriaVisible, ...?

@eps1lon
Copy link
Member Author

eps1lon commented Oct 7, 2019

🤔 Could include the word ARIA in the name to indicate its relationship to that spec: isAriaHidden, isAriaVisible, ...?

isAriaHidden may be confused with aria-hidden and isAriaVisible might be even more confusing since aria-visible doesn't exist.

It's really just an implementation of "Excluding Elements from the Accessibility Tree". I thought about the name for a bit but I would just punt the name towards spec folks.

kentcdodds
kentcdodds previously approved these changes Oct 7, 2019
Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with this. I don't really mind the name all that much 🤷‍♂️ I don't expect many people will use this API.

@kentcdodds
Copy link
Member

lol, actually, I think I like isInaccessible better.

@eps1lon
Copy link
Member Author

eps1lon commented Oct 11, 2019

lol, actually, I think I like isInaccessible better.

Ok let's go with that.

@eps1lon eps1lon force-pushed the feat/public-accessible-exclude branch from 75890ed to ea42a4d Compare October 11, 2019 06:43
@eps1lon eps1lon changed the title feat: Export shouldExcludeFromA11yTree feat: Export isInaccessible Oct 11, 2019
Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super. thanks!

@kentcdodds kentcdodds merged commit 5783c12 into testing-library:master Oct 11, 2019
@kentcdodds
Copy link
Member

🎉 This PR is included in version 6.8.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@eps1lon eps1lon deleted the feat/public-accessible-exclude branch October 12, 2019 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants