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

Remove aria-hidden=true with noIsolation behavior #57

Merged
merged 1 commit into from
Feb 13, 2022

Conversation

omerdemirkan
Copy link
Contributor

Current behavior adds aria-hidden attribute throughout the DOM with noIsolation === true by calling hideOthers(refs, document.body, undefined).

This makes introducing the library into a codebase with existing UI testing extremely challenging, as many testing frameworks ignore subtrees with aria-hidden="true", meaning as soon as react-focus-on is used, many UI tests may go red. I made this in hopes of removing that barrier to entry.

@theKashey
Copy link
Owner

From one point of view hiding everything except currently "usable" area should fix those tests, so this moment is not really makes sense.
But there are two other points:

  • I know how hard it to be "enforced" to follow some best practices as the old code might be just not "ready"
  • This code was supposed to be like this. I should double check (git) history to understand why it ended this way, but noIsolation means NO ISOLATION. No other reading is possible.

Copy link
Owner

@theKashey theKashey left a comment

Choose a reason for hiding this comment

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

Change has been approved. Please give me a moment to verify my decisions of the past.

@theKashey
Copy link
Owner

  • the intention of noIsolation ? undefined : focusHiddenMarker is to set focusHiddenMarker for "other nodes"
  • focusHiddenMarker controls pointer-events: none
  • 😅 obviously removing mouse interaction should be syncronized with removing aria control
  • in other words - proposed change in the only valid way to do that

@theKashey theKashey merged commit f2804bf into theKashey:master Feb 13, 2022
@omerdemirkan
Copy link
Contributor Author

Thanks for the context!

Would love to get my hands on an updated version; is there a particular schedule the package follows for releases?

@theKashey
Copy link
Owner

Trying to handle 4 more problems (starting from #58)

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

Successfully merging this pull request may close these issues.

2 participants