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

additional accessibility guidance for popover #8791

Closed

Conversation

scottaohara
Copy link
Collaborator

@scottaohara scottaohara commented Jan 27, 2023

closes #8714

  • adds two notes for the popover attribute and its target attributes to provide author guidance to ensure best accessibility practices for their popovers.
  • modifies the popover examples to ensure the popover follows the triggering element in the DOM reading order. This is NOT a requirement, but it will often be the way it SHOULD be done. though, i am not sure we want to make that a proper ‘should’ - hence it’s being added as a note.

/popover.html ( diff )

scottaohara and others added 4 commits January 27, 2023 15:28
closes whatwg#8714

- adds two notes for the popover attribute and its target attributes to provide author guidance to ensure best accessibility practices for their popovers.
- modifies the popover examples to ensure the popover follows the triggering element in the DOM reading order.  This is NOT a requirement, but it will often be the way it SHOULD be done.  though, i am not sure we want to make that a proper ‘should’ - hence it’s being added as a note.
@scottaohara
Copy link
Collaborator Author

sorry for the build bug here. i'm not exactly sure what it's asking me to fix (as per my series of extra commits that demonstrate my lack of understanding)

source Outdated Show resolved Hide resolved
thank you @smhigley for the suggestions.  changed a 'should; to 'will need to' so as to not use a 'should' within a note.
@scottaohara

This comment was marked as outdated.

@@ -81269,6 +81269,17 @@ dictionary <dfn dictionary>DragEventInit</dfn> : <span>MouseEventInit</span> {
won't be rendered until it becomes shown, at which point it will be rendered on top of other page
content.</p>

<div class="note">
<p>The <code data-x="attr-popover">popover</code> attribute is a global attribute that allows
Copy link
Member

Choose a reason for hiding this comment

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

Single-space indentation and wrapping at a 100 columns please.

@@ -81931,28 +81942,34 @@ dictionary <dfn dictionary>DragEventInit</dfn> : <span>MouseEventInit</span> {
attribute is specified, then that attribute's value must be the ID of element with the <code
data-x="attr-popover">popover</code> attribute.</p>

<p class="note">To help ensure a popover will be programmatically exposed in a logical reading
order to users of assistive technology, for most authors will need to place the popover
Copy link
Member

Choose a reason for hiding this comment

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

"for most" doesn't read correctly to me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah yup. that was a rewrite of the sentence that i clearly didn't finish. will fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about this?

It is recommended to place the popover element immediately after the triggering element in the DOM in order to ensure the popover will be programatically exposed in a logical reading order to users of assistive technology.

<span data-x="custom element">custom elements</span>. However, it will often be the case that
authors will need to ensure proper accessibility semantics are also provided to these otherwise
generic elements. For instance, by also specifying the appropriate ARIA roles and properties
which represent the type of popover they have created.</p>
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be more helpful for readers if this were an example.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

there were already examples in the next section that i had relied upon, but glad to can add another one here.

Copy link
Member

Choose a reason for hiding this comment

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

So in particular what I'm looking for is an example where it's made accessible through augmentation of the markup. I don't think we cover that at the moment?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should there also be a sentence or two talking about how you can also use non-generic, semantic elements like <figure> or even <dialog>? Since I presume we would prefer that over <div> with ARIA attributes.

@@ -81931,28 +81942,34 @@ dictionary <dfn dictionary>DragEventInit</dfn> : <span>MouseEventInit</span> {
attribute is specified, then that attribute's value must be the ID of element with the <code
data-x="attr-popover">popover</code> attribute.</p>

<p class="note">To help ensure a popover will be programmatically exposed in a logical reading
order to users of assistive technology, for most authors will need to place the popover
element immediately after the triggering element in the DOM.</p>
Copy link
Member

Choose a reason for hiding this comment

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

Why is this the case actually? Wouldn't a popover not be rendered and therefore not be read?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

maybe i need to clarify this, as your question isn't inline with what this is meant to be about.

yes, if a popover is not rendered it's not going to be read. but when one is rendered it'll generally be best to have it in the DOM directly after the trigger that invoked it so that someone using a screen reader and navigating using the virtual cursor (not tab key) will be able to easily find the popover (if it's placed at the bottom of the DOM, for instance, it'll be quite difficult to find, and placing it there sort of defeats one of the benefits to this feature).

&lt;/button></code></pre>
&lt;/button></code>

&lt;ul popover=auto id="foo">
Copy link
Member

Choose a reason for hiding this comment

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

id=links then?

@annevk
Copy link
Member

annevk commented Feb 14, 2023

@mfreed7 @josepharhar @aleventhal presumably you're planning on reviewing this as well?

@annevk annevk added the accessibility Affects accessibility label Feb 14, 2023
@scottaohara
Copy link
Collaborator Author

thanks for the initial review @annevk. i'll work on updating the PR today or tomorrow.

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.

Generally LGTM

<span data-x="custom element">custom elements</span>. However, it will often be the case that
authors will need to ensure proper accessibility semantics are also provided to these otherwise
generic elements. For instance, by also specifying the appropriate ARIA roles and properties
which represent the type of popover they have created.</p>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should there also be a sentence or two talking about how you can also use non-generic, semantic elements like <figure> or even <dialog>? Since I presume we would prefer that over <div> with ARIA attributes.

@annevk annevk added the topic: popover The popover attribute and friends label Feb 16, 2023
@scottaohara
Copy link
Collaborator Author

with the attributes changing, this'll need a bit more work as well. still on my radar, just trying to get time to work on it

scottaohara added a commit to scottaohara/html that referenced this pull request Mar 14, 2023
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
Copy link
Collaborator Author

scottaohara commented Mar 14, 2023

closing this PR and replacing with #9024.

I'll leave the branch alone for now, just in case something was missed. but likely fine to delete the branch

annevk pushed a commit that referenced this pull request 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.

Additional accessibility guidance for popover
5 participants