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

Add comment role #1135

Merged
merged 19 commits into from
Feb 11, 2020
Merged

Add comment role #1135

merged 19 commits into from
Feb 11, 2020

Conversation

aleventhal
Copy link
Contributor

@aleventhal aleventhal commented Dec 11, 2019

Refers to issue #749


Preview | Diff

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
Copy link
Contributor

@carmacleod carmacleod left a comment

Choose a reason for hiding this comment

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

+1

Copy link
Contributor

@joanmarie joanmarie left a comment

Choose a reason for hiding this comment

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

LGTM

@carmacleod
Copy link
Contributor

carmacleod commented Jan 16, 2020

Discussed during call:

  • Need to add aria-setsize and aria-posinset as supported attributes.
  • Name should not be prohibited.

@jnurthen jnurthen requested a review from mcking65 January 23, 2020 18:49
Copy link
Contributor

@mcking65 mcking65 left a comment

Choose a reason for hiding this comment

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

I tried to use the suggestion feature to propose changes. It got a bit tricky though because I made some suggestions that only work if accepted as a set.

My suggestions do not address all the issues. In particular, I am unclear about relationships among hierarchical comments when not all comment elements are present in the DOM.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Show resolved Hide resolved
Copy link
Contributor

@mcking65 mcking65 left a comment

Choose a reason for hiding this comment

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

This is much cleaner and easier to understand now. I have a few editorial suggestions And one substantive suggestion that we not require role complementary for a container.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@mcking65
Copy link
Contributor

I wonder if screen readers would really want to probe every use of aria-details referring to an element that is not a specific annotation type to find out whether it is a general description or a container of specific annotation types. If they have no problem with that, then there would be no use for a property like aria-containsannotations. So, whether it would add value is really a question for screen reader devs.

@carmacleod
Copy link
Contributor

carmacleod commented Jan 28, 2020

I wonder if screen readers would really want to probe every use of aria-details referring to an element that is not a specific annotation type to find out whether it is a general description or a container of specific annotation types.

Good question. If the container had a label, then the user might benefit more from hearing the container's label than from hearing about detailed annotation types within the container?

For example, if a complementary region was the target of an aria-details, and the author added a nice visible label saying 24 comments on "Dogs and their Owners" then would the user prefer to hear that label before navigating there, or something like "contains comments" or "contains 24 comments"?

<article aria-details="comment-section">
  <h1>Dogs and their Owners</h1>
  <p>This is all about dogs. Or is it all about their owners?</p>
</article>
<aside id="comment-section" aria-labelledby="comment-section-label">
  <h2 id="comment-section-label">24 comments on "Dogs and their Owners"<h2>
  ... 24 sibling comment elements go here ...
</aside>

@aleventhal
Copy link
Contributor Author

Matt, I took your suggestions except for complementary. I think there is value in requiring a specific role for a grouping element for comments. This will make it easier for the user agent to know when to bother computing a "hascomments" object attribute hint.

@aleventhal
Copy link
Contributor Author

@mcking65 @jnurthen I've made the requested change to use group or region. I've made it a SHOULD. LMK what you think.

@jnurthen
Copy link
Member

jnurthen commented Feb 3, 2020

@mcking65 can you take a look. I need to merge this one before I merge aria-details.

Copy link
Contributor

@mcking65 mcking65 left a comment

Choose a reason for hiding this comment

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

I think the wording should be more clear. I made a suggestion.

index.html Outdated Show resolved Hide resolved
aleventhal and others added 8 commits February 7, 2020 13:50
Refers to issue #749
Co-Authored-By: Carolyn MacLeod <Carolyn_MacLeod@ca.ibm.com>
Co-Authored-By: Carolyn MacLeod <Carolyn_MacLeod@ca.ibm.com>
Co-Authored-By: Carolyn MacLeod <Carolyn_MacLeod@ca.ibm.com>
Co-Authored-By: Carolyn MacLeod <Carolyn_MacLeod@ca.ibm.com>
@jnurthen jnurthen merged commit eed0ef0 into master Feb 11, 2020
carmacleod pushed a commit that referenced this pull request May 7, 2020
Refers to issue #749
Co-Authored-By: Carolyn MacLeod <Carolyn_MacLeod@ca.ibm.com>
Co-Authored-By: Matt King <a11yThinker@Gmail.com>
@jnurthen jnurthen deleted the comment_role branch July 23, 2020 22:27
@pkra pkra added this to the ARIA 1.3 milestone Jan 10, 2022
@pkra pkra mentioned this pull request Jan 10, 2022
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.

6 participants