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

term/definition should use aria-details instead of aria-labelledby #1147

Merged
merged 14 commits into from Dec 16, 2020

Conversation

aleventhal
Copy link
Contributor

@aleventhal aleventhal commented Jan 7, 2020

Relates to issue #749

A definition should not change the accessible name of a term, and thus aria-labelledby is inappropriate. Rather, a definition is an annotation and should use aria-details.


Preview | Diff

@aleventhal aleventhal mentioned this pull request Jan 7, 2020
@scottaohara
Copy link
Member

with this change should we also update definition to be prohibited from having aria-label / labelledby?.

@aleventhal
Copy link
Contributor Author

with this change should we also update definition to be prohibited from having aria-label / labelledby?.

Yes, @jnurthen , do you think I should do that in this PR?

@jnurthen
Copy link
Member

jnurthen commented Jan 7, 2020

Yes, @jnurthen , do you think I should do that in this PR?

Sounds good to me.

@aleventhal
Copy link
Contributor Author

with this change should we also update definition to be prohibited from having aria-label / labelledby?.

Done, good catch.

index.html Outdated Show resolved Hide resolved
Copy link
Member

@scottaohara scottaohara left a comment

Choose a reason for hiding this comment

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

think we just need some capitalized SHOULDs here. otherwise i like these changes

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@aleventhal
Copy link
Contributor Author

@scottaohara Thanks for catching that. How's it look now?

Copy link
Member

@scottaohara scottaohara left a comment

Choose a reason for hiding this comment

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

just made a quick commit to remove an extra end paragraph tag, but now it looks good to me!

index.html Outdated Show resolved Hide resolved
Co-Authored-By: Carolyn MacLeod <Carolyn_MacLeod@ca.ibm.com>
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.

Should aria-label[ledby] should be prohibited on definition as well? It would sort-of defeat the purpose of using aria-details instead of aria-describedby if an author is able to flatten the definition with an aria-label?

@aleventhal
Copy link
Contributor Author

@carmacleod, that makes sense to me. Let's bring it up at next week's meeting? How about we get this in for now and think about what to do about that?

@carmacleod
Copy link
Contributor

How about we get this in for now and think about what to do about that?

Yep - agreed.

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.

Looks good - thanks!
+1

@scottaohara
Copy link
Member

oh, @aleventhal I actually thought you had updated definition prohibit aria-label and aria-labelledby per our previous comments. completely glazed over that, and see it's just term here.

i think we should prohibit the naming of definition here, it was one of the last things we talked about on the call this week.

index.html Outdated Show resolved Hide resolved
…term, require that a term with a matching definition MUST use aria-details to point to it
Copy link
Contributor Author

@aleventhal aleventhal left a comment

Choose a reason for hiding this comment

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

oh, @aleventhal I actually thought you had updated definition prohibit aria-label and aria-labelledby per our previous comments. completely glazed over that, and see it's just term here.

i think we should prohibit the naming of definition here, it was one of the last things we talked about on the call this week.

Ok, I misunderstood. I've made that change. LMK what you think.

aleventhal and others added 2 commits January 10, 2020 20:17
… that should provide a good screen reader experience in that case.
index.html Outdated
<p>A word or phrase with a corresponding definition. See related <rref>definition</rref>.</p>
<p>The <code>term</code> role is used to explicitly identify a word or phrase for which a <rref>definition</rref> has been provided by the author or is expected to be provided by the user.</p>
<p>A word or phrase with an optional corresponding definition. See related <rref>definition</rref>.</p>
<p>The <code>term</code> role is used to explicitly identify a word or phrase for which a <rref>definition</rref> has been provided by the author or is expected to be provided by the user. Where a matching <rref>definition</rref> exists, authors MUST set <pref>aria-details</pref> on the <code>term</code>, pointing to the <rref>definition</rref>. Otherwise, <pref>aria-details</pref> MUST point to a form or control that take user input that provides the definition.</p>
Copy link
Member

Choose a reason for hiding this comment

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

regarding:

MUST point to a form or control that take user input that provides the definition

  • that "takes" user input
  • could this not also point to a link that takes users to the definition?

i like this idea though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could take the user to a link with the definition, but I'm not sure we really have to say that.

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

carmacleod commented Jan 13, 2020

@aleventhal

Where a matching definition exists, authors MUST set aria-details on the term, pointing to the definition. Otherwise, aria-details MUST point to a form or control that takes user input to provide the definition.

If aria-details is required on a term, then it should be listed under "Required States and Properties:" in the characteristics table.

I guess that would mean that a term cannot be "standalone", i.e. we couldn't have a teacher say, "Here's the list of terms for tomorrow's test.", if the definitions are expected to be written on paper. :)
(contrived example... thanks for your patience as I slowly think this through... ;)

@aleventhal
Copy link
Contributor Author

True, maybe we should leave that as a possibility. What do others think?

@aleventhal
Copy link
Contributor Author

Thinking about this more. What's the user benefit from having term marked up, when there is no definition? Screen readers don't have any special navigation keystrokes, and the fact that it's a term is going to be obvious from context. I would have thought that the benefits of using term come when you can tie the definition to it.

@scottaohara
Copy link
Member

a role parity argument could be made. since dfn should be exposed as a term, there'd be no requirement that they be associated with a definition.

that said, i don't think it's a bad idea to strongly suggest that authors do create these associations...but it's not going to be possible to force a consistent pairing of terms and definitions for anyone using the dfn element

@aleventhal
Copy link
Contributor Author

So, we change it to SHOULD or MAY?

@carmacleod
Copy link
Contributor

SHOULD?

@carmacleod
Copy link
Contributor

carmacleod commented Jan 13, 2020

Actually, I think I like that. Only a couple of nits:

  • add the missing <rref>'s and <pref>'s
  • change "is visible" back to "exists"
  • use "form control" instead of "form"
  • consider changing "an optional corresponding definition" back to "a corresponding definition"
    (i.e. it seems wrong now to say "optional", since aria-details is required. i.e. there SHOULD always be either an element with role="definition", or a "virtual, user-entered definition" element)

Question: Should we also add a note to definition so that authors don't wipe out the semantics of the form control (if any)?

Authors SHOULD NOT use the definition role on interactive elements such as form controls because doing so could prevent users of assistive technologies from interacting with those elements.

@aleventhal
Copy link
Contributor Author

Actually, I think I like that. Only a couple of nits:

  • add the missing <rref>'s and <pref>'s

Done, except for the one where the user might enter a definition, since the definition doesn't actually exist yet.

  • change "is visible" back to "exists"

Basically did that, but it was word to have , exists, by itself, so I rearranged the sentence.

  • use "form control" instead of "form"

See if you like what I did for that.

  • consider changing "an optional corresponding definition" back to "a corresponding definition"

I think we've decided it's optional for role parity.

(i.e. it seems wrong now to say "optional", since aria-details is required. i.e. there SHOULD always be either an element with role="definition", or a "virtual, user-entered definition" element)

I don't think we are requiring aria-details, just saying you should use it if the optional thing is there, or a form at least.

Question: Should we also add a note to definition so that authors don't wipe out the semantics of the form control (if any)?

Done.

@carmacleod
Copy link
Contributor

Ok, looks good to me now!
I already officially "approved"... before @jnurthen made us run around the track a couple more times. 😄

@jnurthen jnurthen merged commit e496a4d into master Dec 16, 2020
@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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants