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

[test] Extract common popup tests #1358

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

michaldudak
Copy link
Member

@michaldudak michaldudak commented Jan 27, 2025

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 and setProps functions returned from render in tests. They now behave similarly to render as they flush the microtask queue before returning.

Copy link

netlify bot commented Jan 27, 2025

Deploy Preview for base-ui ready!

Name Link
🔨 Latest commit 4adf2c6
🔍 Latest deploy log https://app.netlify.com/sites/base-ui/deploys/67a603493834b70008d4f74f
😎 Deploy Preview https://deploy-preview-1358--base-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@michaldudak michaldudak marked this pull request as ready for review January 29, 2025 13:18
@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Feb 4, 2025
role: 'presentation',
hidden: !mounted,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this moved?

Copy link
Member Author

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?

Copy link
Contributor

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

Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants