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

Refactor multiple visibility functions #3617

Closed
straker opened this issue Aug 24, 2022 · 1 comment · Fixed by #3351
Closed

Refactor multiple visibility functions #3617

straker opened this issue Aug 24, 2022 · 1 comment · Fixed by #3351
Labels
feat New feature or enhancement

Comments

@straker
Copy link
Contributor

straker commented Aug 24, 2022

Currently we have 3 visibility functions: isHidden, isHiddenWithCss, and isVisible. It's not particularly clear which one should be used in what scenarios. Also, the isHidden function is in utils which means we can't access virtual nodes which makes the work for knowing when elements are hidden in a closed details difficult.

We've decided to refactor the functions to make things more clear and greatly simplify the code (as there is a lot of duplicated logic between them). First, the names will be updated to better reflect what the logic is trying to say about when a node is visible (or hidden):

  • isHidden - will be deprecated. A new function called isVisibleToScreenreader will be created in commons (so we now have access to virtual nodes).
  • isVisible - will be deprecated and renamed to isVisibleOnScreen. It will not take a screenreader option. isVisibleToScreenreader should be used instead of passing a screenreader option.
  • isHiddenWithCss - will be deprecated and renamed to isHiddenForEveryone.

As part of the refactor, we will split out all the individual visibility checks (#3351) into their own functions that can be used by the 3 new visibility functions.

If possible, we would like to be able to have the 3 visibility functions call each other when needed (so isHiddenForEveryone would call isVisibleToScreenreader and isVisibleOnScreen). To do this we'll need to pass a flag to not have the functions recurse up the parent tree in order to just get the answer for the passed in node. We'll also have to figure out what to do with the cache for this type of call.

@straker straker added the feat New feature or enhancement label Aug 24, 2022
@WilcoFiers
Copy link
Contributor

I think isVisibleToScreenreader and isVisibleOnScreen should call isHiddenForEveryone. Put the shared logic into isHiddenForEveryone, like display:none, hidden-by-default element (like style), hidden area elements, closed details, etc. isVisibleToScreenreader would then only need to add aria-hidden (I think), and isVisibleOnScreen would do things like clipping and off-screen position testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or enhancement
Projects
None yet
2 participants