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

accessibility popover guidance #9024

Merged
merged 12 commits into from
Mar 20, 2023
Merged

Conversation

scottaohara
Copy link
Collaborator

@scottaohara scottaohara commented Mar 14, 2023

this PR is replacing #8791 due to the changes to the related popovertarget attributes. Was much easier to start a new PR than resolve the diffs.

i've added more examples and incorporated feedback that was provided from the review of the original PR.

added the labels and reviewers from the original PR.

Thanks!


/popover.html ( diff )

this PR is replacing whatwg#8791 due to the changes to the related popovertarget attributes.

i've added more examples and incorporated feedback that was provided from the review of the original PR.
@scottaohara scottaohara added accessibility Affects accessibility topic: popover The popover attribute and friends labels Mar 14, 2023
remove 'recommended' from note
fix typo in closing tag order
missing semicolon
add missing id to sub-navigation popover example.

one last example showing how to use popover=manual to reveal a popover as a status message (e.g., a toast).
@scottaohara
Copy link
Collaborator Author

I had been asked in an offline conversation if i could get a status message / toast-like example into the spec for popover=manual. Hence the most recent example added. If anyone thinks this is adding too much, or if more details are necessary to accompany this example, please let me know.

source Show resolved Hide resolved
source Show resolved Hide resolved
Copy link
Collaborator

@mfreed7 mfreed7 left a comment

Choose a reason for hiding this comment

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

LGTM modulo @josepharhar's style comments.

source Show resolved Hide resolved
fix incorrect attribute, re catch from @mfreed7
resolves feedback from @josepharhar
@scottaohara
Copy link
Collaborator Author

thank you @josepharhar @mfreed7. changes have been pushed

after speaking with @aleventhal, a note about popovers/live regions has been added after the code examples.
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Thanks @scottaohara! I've got mainly nits, one more substantive comment.

source Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
this addresses the feedback from @annevk's review.
@scottaohara
Copy link
Collaborator Author

thank you @annevk. have incorporated your feedback

source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
@annevk
Copy link
Member

annevk commented Mar 17, 2023

Thanks @scottaohara. I plan on merging this Monday. If there's any remaining nits at that point I'll attempt to fix them.

@annevk annevk merged commit dbe5f6c into whatwg:main Mar 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility Affects accessibility topic: popover The popover attribute and friends
Development

Successfully merging this pull request may close these issues.

None yet

4 participants