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

Prohibit label on certain roles #985

Merged
merged 17 commits into from Jun 28, 2019
Merged

Prohibit label on certain roles #985

merged 17 commits into from Jun 28, 2019

Conversation

jnurthen
Copy link
Member

@jnurthen jnurthen commented May 23, 2019

@jnurthen jnurthen added the WIP label May 23, 2019
@carmacleod
Copy link
Contributor

  • The Prohibited section for blockquote is before the Inherited section, but for all of the others (caption, code, deletion, emphasis, insertion, note, paragraph, presentation, strong, subscript, superscript), the Prohibited section is after the Inherited section.

  • Consider removing "Name from: N/A" rows from the characteristics table for consistency with other N/A characteristics rows.

@jnurthen
Copy link
Member Author

* Consider removing "Name from: N/A" rows from the characteristics table for consistency with other N/A characteristics rows.

I considered this but thought it might be worth calling it out explicitly. I'm certainly willing to go either way on it, but I was also thinking of adding a corresponding type in the section https://pr-preview.s3.amazonaws.com/w3c/aria/pull/985.html#namecalculation corresponding with whatever we decide we would call the N/A value.

@carmacleod
Copy link
Contributor

I considered this but thought it might be worth calling it out explicitly.

Right. Hard to say. On the one hand, "Prohibited States and Properties: aria-label, aria-labelledby" is pretty explicit and right above where "Name from:" would be, but on the other hand I know what you mean - it is probably worth doing as much as possible to point out this particular change.

I was also thinking of adding a corresponding type in the section https://pr-preview.s3.amazonaws.com/w3c/aria/pull/985.html#namecalculation corresponding with whatever we decide we would call the N/A value.

Right. If we do that, then we need to keep Name From.
So, we would have author, contents, encapsulation, legend, and ... none? prohibited?

@jnurthen jnurthen added the Agenda label Jun 5, 2019
@jnurthen
Copy link
Member Author

jnurthen commented Jun 5, 2019

@stevefaulkner very interested in your feedback on this

@stevefaulkner
Copy link
Contributor

@jnurthen from looking at the changes it is unclear to me which roles are effected, is there a list of roles or a rendered view of the branch I can look at?

@carmacleod
Copy link
Contributor

carmacleod commented Jun 6, 2019

@stevefaulkner
Here's the list of the roles that prohibit aria-label[ledby] in this PR:

  • blockquote
  • caption
  • code
  • deletion
  • emphasis
  • insertion
  • note
  • paragraph
  • presentation
  • strong
  • subscript
  • superscript

It's mostly new roles (except presentation) that we recently added for HTML role parity, so fairly minimal impact at the moment. This is paving the way for adding the "generic" role, which is intended to be the default role for div and span (and some others). Our current thinking is that generics should be nameless.

Here's the PR Preview. If you look for the word "prohibit" in the preview, then you will hit all of the salient changes.

common/script/aria.js Outdated Show resolved Hide resolved
index.html Outdated
@@ -12325,10 +12439,11 @@ <h3>States and Properties</h3>
</ul>
</li>
</ul>
<p>If the author specfies a WAI-ARIA property on a role where that property is prohibited, the user agent SHOULD treat the property as if it were allowed. </p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we could put an editor's note after this statement to further discourage authors from ignoring the prohibition? Candidate text:

Note: At the present time, user agents have no obligation to ignore prohibited states and properties. However, this may change in a future version of ARIA in order to ensure interoperability.

Copy link
Member Author

Choose a reason for hiding this comment

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

We agreed at the meeting to remove the text from the author errors section I believe. Could this go after line 498 instead?

@jnurthen
Copy link
Member Author

jnurthen commented Jun 6, 2019

I will need to split this into 2 (aria-common and aria) - leaving as one until ready for convenience

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.

Attributes, States, Properties

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
@stevefaulkner
Copy link
Contributor

@jnurthen wondering about the reasons for choosing paragraph/note/blockquote. is there related discussion?

@jnurthen
Copy link
Member Author

jnurthen commented Jun 7, 2019

@stevefaulkner there is some discussion in #833 - although note looks like a mistake - thanks for catching it. I am removing that - it was intended to be on none - but that doesn't follow the normal role structure.

@scottaohara
Copy link
Member

shouldn't term be changed to prohibited as well, especially since it's no longer related to dt?

definition could be given it's name from the term

<span role="term" id="t">Word</span>
<span role="definition" aria-labelledby="t">
  words to define word.
</span>

@joanmarie
Copy link
Contributor

definition could be given it's name from the term

<span role="term" id="t">Word</span>
<span role="definition" aria-labelledby="t">
  words to define word.
</span>

When something has a valid name, screen readers might present that name and NOT present the associated text. So if there's no aria-labelledby the term "word" has a presented-by-the-screenreader definition of "words to define word". But with the aria-labelledby present, some screen readers might present the definition of "word" as "word". Unless they have smarts to say definition, like paragraph and friends really shouldn't have a name.

TL;DR: Should we prohibit naming of definition?

@jnurthen
Copy link
Member Author

jnurthen commented Jun 7, 2019

TL;DR: Should we prohibit naming of definition?

We would have to change definition's definition

Authors SHOULD identify the element being defined by giving that element a role of term and referencing it with the aria-labelledby attribute or by making the element with role term a descendant of the element with role definition.

@scottaohara
Copy link
Member

scottaohara commented Jun 7, 2019

@joanmarie I had originally had that thought about definition when doing the review for the revised term and definition work. But I put that aside because I assumed definition would work like group, where it could have a name, and its contents still remain accessible, per the definition of definition as James noted.

For example, a definition that consists of two paragraphs:

<span role="term" id="f">
  Funny
</span>
<div role="definition" aria-labelledby="f">
  <p>Causing laughter or amusement.</p>
  <p>Making or given to making amusing jokes or witticisms.</p>
</div>

@joanmarie
Copy link
Contributor

It will be interesting to see how screen readers present definition.

@stevefaulkner
Copy link
Contributor

Of the list provided the only definitely questionable one is blockquote I can envisage scenarios where it would be useful to associate a label/description for a blockquote pertaining to it source (for example). In a quick test I found that both JAWS and NVDA announce the presence of a blockquote and NVDA also announces the accessible name if it has one.

I am on the fence about <p> as i could envisage situation where annotating a paragraph could be useful, although I have not encountered AT support for announcing the presence of <p> elements.

@mcking65
Copy link
Contributor

@stevefaulkner wrote:

Of the list provided the only definitely questionable one is blockquote I can envisage scenarios where it would be useful to associate a label/description for a blockquote pertaining to it source (for example). In a quick test I found that both JAWS and NVDA announce the presence of a blockquote and NVDA also announces the accessible name if it has one.

I am on the fence about <p> as i could envisage situation where annotating a paragraph could be useful, although I have not encountered AT support for announcing the presence of <p> elements.

I agree that it is feasible for screen readers to support named blockquotes. Most screen readers present the boundaries of blockquotes in reading mode, which provides a reasonable way of supporting names without confusing users. While it doesn't seem like a practice to recommend, prohibiting it may be unnecessary.

However, I strongly believe prohibiting named paragraphs is necessary.

index.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@jnurthen
Copy link
Member Author

w3c/accname#53 shows how accname could change to enable this

@jnurthen jnurthen marked this pull request as ready for review June 20, 2019 22:23
@carmacleod
Copy link
Contributor

I think this PR is good to go.
The initial set of roles that will prohibit naming makes sense, and more can be added as/if needed.
The wording also allows for other attributes to be prohibited on specific roles in future, as/if needed.

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.

Looks great! Nice work! Let's ship it!

@jnurthen jnurthen dismissed carmacleod’s stale review June 28, 2019 19:50

commented "I think this PR is good to go."

@jnurthen jnurthen removed the WIP label Jun 28, 2019
@jnurthen
Copy link
Member Author

I think this is ready to go. @joanmarie do you want to merge or should I?

@joanmarie
Copy link
Contributor

I think this is ready to go. @joanmarie do you want to merge or should I?

You did all the work. :) Go for it! Do please update the wiki so we can add it to the list of things which will need tests.

@jnurthen jnurthen changed the title [wip] Do not allow label on certain roles Do not allow label on certain roles Jun 28, 2019
@jnurthen jnurthen changed the title Do not allow label on certain roles Prohibit label on certain roles Jun 28, 2019
@jnurthen jnurthen merged commit 834a8ba into master Jun 28, 2019
@jnurthen jnurthen deleted the DisallowLabel branch October 14, 2019 21:17
jnurthen added a commit that referenced this pull request Oct 24, 2019
Prohibits Labeling of the following roles:
caption, code, deletion, emphasis, insertion, paragraph, presentation, strong, subscript, superscript
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.

None yet

7 participants