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

New rule: role is permitted #2084

Open
wants to merge 18 commits into
base: develop
Choose a base branch
from
Open

New rule: role is permitted #2084

wants to merge 18 commits into from

Conversation

Jym77
Copy link
Collaborator

@Jym77 Jym77 commented Jul 13, 2023

New rule to check that a role is permitted on this element.

  • It currently checks only the explicit role. Should it check every token in the role attribute instead? (same as "role has valid value")
  • There are very few examples. I'm struggling a bit to find the good balance… The spec here is essentially a list of cases and we definitely cannot have all of them. If you have ideas of relevant (=used in practice) examples to add, pleas let me know.
  • I am actually not sure that the rule should require the element to be in the accessibility tree. This is because presentation is not allowed on every element (e.g. it is allowed on h1, not button, I assume this is due to the focusability of button). But we cannot really test that if we restrict the rule to content in the accessibility tree… (well, of course, the presentational conflict would trigger but I'd rather not have the rule rely on that…) OTOH, checking display: none content seems really unecessarily…
  • I think the link to "ARIA in HTML" in the conformance mapping will need some website code update to be correctly picked up: Add possibility to link to ARIA in HTML act-tools#31.

Closes issue(s):

Need for Call for Review:
This will require a 2 weeks Call for Review (new rule)


Pull Request Etiquette

When creating PR:

  • Make sure you're requesting to pull a branch (right side) to the develop branch (left side).
  • Make sure you do not remove the "How to Review and Approve" section in your pull request description

After creating PR:

  • Add yourself (and co-authors) as "Assignees" for PR.
  • Add label to indicate if it's a Rule, Definition or Chore.
  • Link the PR to any issue it solves. This will be done automatically by referencing the issue at the top of this comment in the indicated place.
  • Optionally request feedback from anyone in particular by assigning them as "Reviewers".

When merging a PR:

  • Close any issue that the PR resolves. This will happen automatically upon merging if the PR was correctly linked to the issue, e.g. by referencing the issue at the top of this comment.

How to Review And Approve

  • Go to the “Files changed” tab
  • Here you will have the option to leave comments on different lines.
  • Once the review is completed, find the “Review changes” button in the top right, select “Approve” (if you are really confident in the rule) or "Request changes" and click “Submit review”.
  • Make sure to also review the proposed Call for Review period. In case of disagreement, the longer period wins.

@Jym77 Jym77 added Rule Use this label for a new rule that does not exist already reviewers wanted labels Jul 13, 2023
@Jym77 Jym77 self-assigned this Jul 13, 2023
Copy link
Collaborator

@daniel-montalvo daniel-montalvo left a comment

Choose a reason for hiding this comment

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

So glad to see this new rule coming along. I have one minor editorial and one question.

What is this rule supposed to do with role="generic" or with ARIA deprecated roels such as directory? According to how this is written I'd say these pass, but different implementers may have different interpretations in the form of stronger warnings against of these that we may want to qualify.

_rules/aria-role-permitted-j7zzqr.md Outdated Show resolved Hide resolved
_rules/aria-role-permitted-j7zzqr.md Outdated Show resolved Hide resolved
_rules/aria-role-permitted-j7zzqr.md Outdated Show resolved Hide resolved
_rules/aria-role-permitted-j7zzqr.md Outdated Show resolved Hide resolved
_rules/aria-role-permitted-j7zzqr.md Show resolved Hide resolved
@Jym77
Copy link
Collaborator Author

Jym77 commented Aug 17, 2023

I've done most changes. I've also removed the inclusion in the accessibility tree from the Applicability in order to be able to also test role="presentation".

This `button` element is not [included in the accessibility tree][].

```html
<button role="list" style="display:none;">Click me</button>
Copy link
Collaborator

Choose a reason for hiding this comment

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

From the background:

This rules apply to every element, even if they are not [included in the accessibility tree][].

Wouldn't that make this a failed example instead of an inapplicable example?

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @tbostic32

The applicability are elements with an explicit role defined which is the case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

See my other comment. I do think this should be inapplicable to this rule, and the note and applicability rather than this testcase should be modified. However, one or the other needs to change.

Copy link
Collaborator Author

@Jym77 Jym77 Sep 28, 2023

Choose a reason for hiding this comment

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

True, moved it to an Failed case.

I think that this is still failing ARIA in HTML, even if it doesn't cause an accessibility problem in the end, so I'd rather keep it that way. ARIA in HTML doesn't specify that the roles are only allowed on elements that are exposed. Moreover, if we restrict the rule to exposed elements, suddenly Failed Example 4 (<li role="presentation">) becomes Inapplicable when it's a clear failure since the only allowed role is listitem.

This is a bit similar to the fact that, in HTML¹, duplicate id are not allowed even though they might very well never cause a single problem. It is nonetheless invalid HTML. Here also, as far as I understand, using the wrong role on a hidden element is invalid ARIA, even if the user never encounter that.

¹: this is not about SC 4.1.1, but HTML/DOM specs

Copy link
Collaborator

Choose a reason for hiding this comment

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

In accordance with my comment #2084 (comment), to me, Failed Example 4 is not failing because of the presence of the "presentation" role in a DOM children. Rather, its failure is attributed to the absence of an accessibility child with the role "listitem" within the DOM child.

Hence, it is not the "role=presentation" itself that impacts the test result; rather, the test fails due to the absence of a child with the "listitem" role.

I agree with Tom that we should consider elements included into the accessibility tree.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@giacomo-petri The reason why role="presentation" was taken into account by this rule is that certain elements (e.g., article or aside) explicitly allow it in the list of allowed roles; while some other (like button) also have an explicit list of allowed roles but without presentation being listed.

On looking a bit more closely, it might be that the ones where presentation isn't listed are the elements which are normally focusable, so the conflict would trigger 🤔 I'll look a bit deeper.

Copy link
Contributor

Choose a reason for hiding this comment

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

just going to state one more time that i'd err on the side of flagging this - as otherwise it falls on making a developer/tester have to do more work to find errors, by ensuring they fully comb through a UI and reveal all hidden content. I do acknoweldge there are probably instances of devs hiding problematic markup, rather than fixing it... but should a dev really be surprised to get an error flagged for something they knew was an error and 'hid' it as their 'fix'?

Copy link
Member

@carlosapaduarte carlosapaduarte left a comment

Choose a reason for hiding this comment

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

Suggestions for further examples:

Something with role="presentation" and role="generic", both for failed and pass examples

Perhaps we might want an example with a list of tokens, where the first token is a valid token corresponding to a non permitted role and the second is a valid permitted role

Examples with aria-hidden (in addition to the one with display none)

This `button` element is not [included in the accessibility tree][].

```html
<button role="list" style="display:none;">Click me</button>
Copy link
Member

Choose a reason for hiding this comment

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

I agree with @tbostic32

The applicability are elements with an explicit role defined which is the case.

_rules/aria-role-permitted-j7zzqr.md Outdated Show resolved Hide resolved

[ARIA in HTML][aria in html document conformance] also defines the [implicit semantic role][implicit role] of each element. Setting the [explicit role][] as the same as the [implicit one][implicit role] is not recommended but nonetheless allowed. This rule doesn't use that in any of its test cases.

This rules apply to every element, even if they are not [included in the accessibility tree][]. This is because the roles of `none` or `presentation` are only allowed on certain elements. If the rule was only looking at content [included in the accessibility tree][], it wouldn't flag incorrect use of these roles.
Copy link
Collaborator

Choose a reason for hiding this comment

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

If an element and its children are not included in the accessibility tree, it doesn't really matter what the roles are. Therefore, it shouldn't be an accessibility problem if these roles are incorrectly used. While I agree that it's technically incorrect, I think it should be considered more as a recommendation rather than a violation - similar to the use of an explicit role that is redundant with the implicit role.

@scottaohara Care to comment on the ARIA in HTML position on this one?

Copy link
Contributor

Choose a reason for hiding this comment

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

i mean, yes - if the element is hidden form the accessibility tree then sure, no one can access it / there can't be an issue.

the counter point being that if it's rendered in the DOM but marked as hidden, is that because at some point it would be expected that a user could then unhide that content to interact with it? Could excluding something just because it's temporarily hidden not cause a bunch of gotchas for developers who might run a scan, it comes up clean, and then they still get bugs because someone had the audacity to try and use their UI? :)

but seriously, i can see the rational to just expose this as a recommendation / needs review - in the hidden state, as long as if it becomes shown it then would be marked as a violation. it just seems like a delay in flagging some issues because maybe someone did a hack-job at 'removing' an errant element, and that shouldn't be called out?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@scottaohara With the way that UIs are built these days, what is in the DOM changes with almost every state change. There are often scaffolds that are hidden, and then made whole when they're actually used / made visible. We don't report on a lot of issues in hidden content because of this - teams were frustrated by all of the false positives.

While I agree that it's unlikely that an incorrectly used role will be corrected on such a state change, I don't think we should call out a tool as incorrect for ignoring that content completely.

This `button` element is not [included in the accessibility tree][].

```html
<button role="list" style="display:none;">Click me</button>
Copy link
Collaborator

Choose a reason for hiding this comment

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

See my other comment. I do think this should be inapplicable to this rule, and the note and applicability rather than this testcase should be modified. However, one or the other needs to change.

@tombrunet
Copy link
Collaborator

One additional comment - a colleague of mine suggested that we might want to more clearly indicate the applicability to explicit roles in the title.

@Jym77
Copy link
Collaborator Author

Jym77 commented Oct 11, 2023

@scottaohara Thanks a lot for the feedback. This is a rule designed for ARIA, not WCAG, so it is ultimately up to ARIA group to decide what fails and what is a false positive 😉

@giacomo-petri Does that change your opinion on whether Applicability should be restricted to exposed content? Or should we take the discussion to a CG call?

@giacomo-petri
Copy link
Collaborator

@Jym77 @scottaohara,

I understand and generally agree with the points Scott has raised. However, there are scenarios where these principles may not be easily applicable.

Generally speaking, I've come across several instances where functional code remains hidden from all users. For example, developers often use "display:none" containers to enclose functional inputs within a form. While some argue in favor of using "input type hidden" elements, the former approach doesn't necessarily lead to accessibility issues.
There are also situations where code dynamically changes based on user interactions, such as a hidden list or a combobox with initial placeholders that become visible after their content is replaced.

Since we can't anticipate the outcome until we actually interact with the page, I believe it's not our responsibility to assume the author's intent. Instead, we should concentrate on evaluating the output produced by user interactions, as it effectively reveals the author's intent and the solutions they've implemented.

@Jym77
Copy link
Collaborator Author

Jym77 commented Oct 11, 2023

@giacomo-petri I think this is where I make the difference between ARIA (authoring) error and end user accessibility problem (more or less the same as WCAG violation).

It's a bit the same that happens when we check for validity or aria-* attributes or the role (or triggers of the presentation conflict):

<button aria-name="hello">world</button>
<a role="btn" href="#">Hello</a>
<button role="presentation">Hello</button>
<h1 role="presentation" aria-label="Hello">world</h1>

None of these create an actual problem for the end user, but these are clearly bad ARIA and author errors, that some of the rules we have are flagging.

This proposed rule is also checking for author error, rather than end user problems. If <button role="list" style="display: none">Hello</button> is considered as an author error by the ARIA group, I think the rule should follow suit and flag it as well, even if end users are never going to experience that button.

@tombrunet
Copy link
Collaborator

For the purposes of ACT, it's not really a question of whether or not it's correct / incorrect for the invalid role to be used on hidden content, but rather should a tool be flagged as inconsistent for choosing not to report that. I believe that it's perfectly acceptable for a tool to choose to fail a hidden invalid role, and it's also perfectly acceptable to completely ignore that since the role will not be exposed outside of the DOM.

The two ways we tend to handle that are to either scope this rule so that the questionable scenario isn't included, or to intentionally not include related test cases.

@scottaohara
Copy link
Contributor

i had written a response, but with @tombrunet's comment, i see that would just be going off topic... so i'll refrain.

if i understand your comment, Tom, are you saying that by marking such instances as "inapplicable" it then allows tools to make their own choices about whether to surface such instances or not? But otherwise, it would be expected for the checkers to either all pass or all fail the rule - and the choice to or mark something as a warning or needs review is off the table?

@tombrunet
Copy link
Collaborator

tombrunet commented Oct 11, 2023

For the purposes of this rule, if the test case says that this is a fail and a tool does not flag it, the tool would be considered as inconsistent. But, if the test case says inapplicable and the tool says it fails, it would also be considered inconsistent. So, if we don't want either of these tools to be considered as inconsistent, we can either:

  • Be clear that this situation is out of scope and have the test case as inapplicable - this doesn't imply anything regarding the testcase, just that that scenario isn't covered by this rule (it could be covered by some other rule).
  • Avoid this scenario completely and not have a testcase for it (ACT just doesn't take a position on the matter).

@Jym77
Copy link
Collaborator Author

Jym77 commented Oct 12, 2023

Given the amount of discussion we're having, I think it is safer to take it to the rest of the group on a call.

Feel free to continue discussing it here 😄 We'll need a common decision in the end, but I don't want to cut the current discussion short until then.

This rule applies to any [HTML element][namespaced element] which has an [explicit semantic role][explicit role] and for which at least one of the following is true:

- the element is [included in the accessibility tree][]; or
- the element has an [explicit semantic role][] of `presentation` or `none`.
Copy link
Member

Choose a reason for hiding this comment

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

It's a little odd to me that this rule would apply to an element with role=none that's inside an aria-hidden=true container. Perhaps the applicability should just have an exception for elements that are programmatically hidden?

This `h1` element has an [explicit role][] of `tab`, which is allowed

```html
<h1 role="tab">ACT rules</h1>
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a huge fan of doing this, as there are more requirements for tab to be valid, like that it needs to be in a tablist. I think either a different role is needed here, or this needs to be made a fully valid tab.

This rule checks that explicit WAI-ARIA roles are allowed for the element they are specified on.
accessibility_requirements:
html-aria:docconformance:
title: ARIA in HTML, 4. Document conformance requirements for use of ARIA attributes in HTML
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, this is going to be very nit picky.

The third cell in each row defines the ARIA role values and aria-* attributes which authors MAY specify on the element

I don't think the way you're interpreting this requirement is right. Or quite possibly, I think the requirement is written incorrectly. MAY in RFC2119 speak does not create a prohibitive statement. You can use these roles on those elements, and browsers have to support it when you do. But that's not the same as saying authors are not allowed to use other roles. It would have to be something like "if an author specifies a role it MUST be one from the third column."

I think this is a solid rule, and we should definitely have it, but I'm a little reluctant to say this is a conformance requirement for ARIA in HTML. Even if that was the intended meaning, that's not what it says. I don't really want to hold this rule up over a technicality like this, but I do think something needs to be done about this.

Copy link
Contributor

Choose a reason for hiding this comment

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

@WilcoFiers the sentence you quote needs to be put into the context of section 1 Author requirements fo ruse of ARIA in HTML

Authors MUST NOT use the ARIA role and aria-* attributes in a manner that conflicts with the semantics described in the 4. Document conformance requirements for use of ARIA attributes in HTML and 4.2 Requirements for use of ARIA attributes in place of equivalent HTML attributes tables. ...

per section 1, it's an Authors MUST NOT - where the Author MAYs specificy what it allowed.

if you think this is unclear because one has to piece together these different parts of the specification, then that's fair. The spec can be updated to restate the author requirements more overtly in tandem with the text you quoted.


#### Passed Example 1

This `a` element with an `href` attribute has an [explicit role][] of `button`, which is allowed.
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 these descriptions would read better if we flip the role and element around:

Suggested change
This `a` element with an `href` attribute has an [explicit role][] of `button`, which is allowed.
The `button` role is allowed on an `a` element with an `href` attribute.

This `dialog` element has an [explicit role][] of `alertdialog`, which is allowed. Even though the `alert` role is not allowed, the first token is a valid role and is therefore used.

```html
<dialog role="alertdialog alert">This is not right.</dialog>
Copy link
Member

Choose a reason for hiding this comment

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

I'm not 100% sure about this one. The requirement for it is this:

The third cell in each row defines the ARIA role values and aria-* attributes which authors MAY specify on the element.

That to me reads that fallback roles apply here too. I don't know that I would want them too. That feels a little on the strict side, but it might be good to put something in the background section about fallback roles, and straight up invalid or abstract roles being out of scope for this rule, even if they do seem like they'd fail the requirement.

This `label` element has an [explicit role][] of `generic`, which is not allowed.

```html
<label role="generic"> Name: <input /> </label>
Copy link
Member

Choose a reason for hiding this comment

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

ARIA is a little vague on whether generic elements are supposed to be in the accessibility tree. I think the applicability as you have it now probably means this passes. My suggested update to it should solve that.

Copy link
Contributor

Choose a reason for hiding this comment

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

'passes' though a should not from aria.

i think aria in html's rules for label are a bit too strict here. i'm going to make an issue to address this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I understood the point of this example to be that generic is a user-agent assigned role, but not one that you are allowed to explicitly specify. Now that I think about it though, that may be more appropriate for https://www.w3.org/WAI/standards-guidelines/act/rules/674b10/proposed/

Copy link
Contributor

Choose a reason for hiding this comment

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

@Jym77
Copy link
Collaborator Author

Jym77 commented Jan 25, 2024

CG decision: restrict rule to elements in the applicability tree and explicit role of presentation/none (or not in hidden state), add background note that stuff out of the accessibility tree still need to respect this but won't cause user problem.

@scottaohara
Copy link
Contributor

add background note that stuff out of the accessibility tree still need to respect this but won't cause user problem

will "wont' cause user problem" be qualified in this note to state "won't cause the user a problem so long as elements with non-permitted roles don't become rendered/available to users. ??

@Jym77 Jym77 removed the Agenda item label Feb 21, 2024
@Jym77 Jym77 requested a review from WilcoFiers February 29, 2024 13:00
@Jym77
Copy link
Collaborator Author

Jym77 commented Feb 29, 2024

add background note that stuff out of the accessibility tree still need to respect this but won't cause user problem

will "wont' cause user problem" be qualified in this note to state "won't cause the user a problem so long as elements with non-permitted roles don't become rendered/available to users. ??

@scottaohara Should be good now. Can you check if the new note is alright for you?

Co-authored-by: Scott O'Hara <[email protected]>
@Jym77 Jym77 requested a review from scottaohara March 1, 2024 07:49
@Jym77 Jym77 requested a review from scottaohara March 1, 2024 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
reviewers wanted Rule Use this label for a new rule that does not exist already
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rule idea: role is permitted for element
9 participants