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

merge multiple isHidden utility functions #1519

Closed
jeeyyy opened this issue Apr 25, 2019 · 3 comments
Closed

merge multiple isHidden utility functions #1519

jeeyyy opened this issue Apr 25, 2019 · 3 comments
Labels
feat New feature or enhancement tech debt Technical debt related tasks

Comments

@jeeyyy
Copy link
Contributor

jeeyyy commented Apr 25, 2019

We have multiple fn's that do more or less the same thing:

  • dom.isVisible
  • util.isHidden
  • dom.isHiddenWithCss

Nice to have:

  • It would be nice to merge these with optional arguments perhaps

Good to solve:

Must do:

  • decorate @deprecated if we choose to not use any.
@jeeyyy jeeyyy added the feat New feature or enhancement label Apr 25, 2019
@straker straker added the tech debt Technical debt related tasks label Nov 25, 2020
@straker
Copy link
Contributor

straker commented Dec 22, 2020

Here's my findings in comparing the 3 functions

utils/is-hidden dom/is-visible dom/is-hidden-with-css
takes single element ✓ Yes ✓ Yes ✓ Yes
takes screenReader option × No ✓ Yes × No
looks at nodeType ✓ Yes ✓ Yes ✓ Yes
looks at nodeName × No ✓ Yes ✓ Yes
looks at css display ✓ Yes ✓ Yes ✓ Yes
looks at css visibility ✓ Yes ✓ Yes ✓ Yes
looks at aria-hidden ✓ Yes ✓ Yes × No
looks at css clip × No ✓ Yes × No
looks at css opacity × No ✓ Yes × No
looks at css scroll height × No ✓ Yes × No
looks at offscreen × No ✓ Yes × No
recurses with parent el ✓ Yes ✓ Yes ✓ Yes

And how they are used in the code:

  • utils/is-hidden
    • when gathering nodes in core/rule.js gather step
    • when gathering frames in core/context.js
  • dom/is-hidden-with-css
    • checking if element is hidden in dom/focus-disabled.js
    • checking if form control is hidden in text/form-control-value.js
  • dom/is-visible
    • everywhere else

utils/is-hidden is used only in the two files to gather nodes before passing them to a rule/check, and dom/is-hidden-with-css is used in two specific files, though I'm not sure why it is used over dom/is-visible. dom/is-visible is used the majority of the time (59 matches when doing a search) and is the function other code bases use to check an elements visibility.

Therefore, I propose we deprecate utils/is-hidden and dom/is-hidden-with-css and use dom/is-visible as the single function. Since we need the function in core, we can move the function to utils and export it in dom like we've done for other functions (making it backwards compatible). The function covers everything the other two functions does, and more, so we should be able to make it work for all use cases.

@straker
Copy link
Contributor

straker commented Jan 13, 2021

@WilcoFiers suggest mimicking ACT definitions of is-visible and is-hidden-state which help distinguish when a DOM node is visually visible (offscreen, visible, etc., excluding aria-hidden) and when a node is hidden from the accessibility tree (looks at aria-hidden, visibility, and display)

@WilcoFiers
Copy link
Contributor

I don't see us ever making this change. I don't think the effort out weights the effort involved in making this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or enhancement tech debt Technical debt related tasks
Projects
None yet
Development

No branches or pull requests

3 participants