[web] Ensure Sidebar siblings do not remain inert forever #572
Merged
[web] Ensure Sidebar siblings do not remain inert forever #572
Conversation
While unmounting the Sidebar is unlikely to happen, it's better to make the component a good citizen by "restoring" its siblings by removing the `inert` and `aria-hidden` attributes. That way, we're minimizing the risk of users being trapped in an issue similar to the one described at patternfly/patternfly-react#9096 Please note that at the time of writing it's not a real restore because the component isn't tracking the status of these attributes BEFORE adding them. However, removing the supposedly added attributes seems enough for now.
Pull Request Test Coverage Report for Build 4945512932
💛 - Coveralls |
imobachgs
approved these changes
May 11, 2023
This was referenced May 17, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
As mentioned in patternfly/patternfly-react#9096, #564 revealed that unmounting a PF4/Modal component results in its siblings remaining hidden from the accessibility API.
Since #563 followed the same pattern of hiding the Sidebar siblings, it may suffer the same problem if it is unmounted (although it is not supposed to happen). Therefore, it would be better to minimize the risk of keeping siblings inert if the Sidebar is unmounted in the future.
Solution
To remove the
inertandaria-hiddenattributes from its siblings BEFORE the Sidebar is unmounted. Note the use ofuseLayoutEffectfor doing so while the component still having access to the parent and its children.Testing
Added another unit test
Tested manually
Notes
Consider this PR as an extension or improvement of [web] Set sidebar siblings as inert and aria-hidden when it's open #563. I.e., no entry in the changelog needed.
I was tempted to drop the
aria-hiddenattribute and use onlyinert, which should do both, make the element not interactive and remove it from the accessibility treeBut I have left there meanwhile to be in sync with PF4/Modal. We can drop it in the future, once
inertis more adopted.To know more, read https://developer.chrome.com/articles/inert/
We started doing this in [web] Set sidebar siblings as inert and aria-hidden when it's open #563 for mimicking PF4/Modal and to be ready for [web] Blur and grayscale
aria-hiddenandinertdiv nodes #564. Now I have realized that all of this is well described in the Dialog (Modal) pattern available at ARIA Authoring Practices Guide (APG) . So, looks like we're in the right path 😉 (and doing almost the same that the modal version of the nativedialogelement, which would be nice to start using at some point in the future).Worth mentioning/reminding that for now the component is just adding/removing these attributes, but not tracking if they were already there (which is unlikely) for restoring them instead of removing. Related to Create a hook to work with siblings elements #565.
Screenshots
N/A