-
-
Notifications
You must be signed in to change notification settings - Fork 91
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
[test] Extract common popup tests #1358
base: master
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
c4d413b
to
ae787c7
Compare
dc0f139
to
9ff3306
Compare
role: 'presentation', | ||
hidden: !mounted, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this moved?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it's consistent with other roles - they are placed on popups, not positioners. Is it an issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some screen readers, the positioner wrapper needs to have role="presentation"
to not interfere with the child popup role
from what I've gathered. It makes it 'inert'/ignored by them. The popup is what should have the ARIA role, but never role=presentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 All right, I'll remove the role check from the common tests and revert this change.
To help ensure all our popup-based components work in a similar way, I added a popup test suite that's shared across all these components. I have included just the basic tests so far. It can be extended to test modality going forward.
Additional changes:
Created async versions of
rerender
andsetProps
functions returned fromrender
in tests. They now behave similarly torender
as they flush the microtask queue before returning.