New rule: role is permitted

Jym77 commented 1 year ago

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: https://github.com/act-rules/act-tools/pull/31.~

Closes issue(s):

closes #2042

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

Pull Request Etiquette

When creating PR:

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

After creating PR:

[X] Add yourself (and co-authors) as "Assignees" for PR.
[X] Add label to indicate if it's a Rule, Definition or Chore.
[X] 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.
[X] 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 commented 1 year ago

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".

tombrunet commented 10 months ago

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 commented 10 months ago

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.

@daniel-montalvo As far as I can read, both are "NOT RECOMMANDED" (in ARIA in HTML) or "SHOULD NOT" (for generic in ARIA 1.2) cases, but are not conformance requirements. I think that this rule should therefore not flag them. For deprecated roles, I think that "Role attribute has valid value" will catch it 🤔 For generic, we could have a separate rule checking for that, but that would not map to any requirement given that it is only a "SHOULD NOT" case.

I've added a Background note to explicit that reasoning.

scottaohara commented 10 months ago

@Jym77 per your requesting my review - i personally dont' agree that it makes sense to designate <button role=list> as inapplicable because it's hidden from the a11y tree with display none.

if display none was used to hide errant code for the purpose of solving a bug, then it should be of no surprise to the developer that this would be flagged to them. hiding something under the rug of CSS isn't really solving the problem, after all.

if display none is there because the element is temporarily hidden until rendered by the user, then why make someone have to render that UI element before calling out the error with it? That to me seems inconsistent results, and would, I assume, mean that this test couldn't be performed at the source code level? It could only find the error on a rendered web page? please correct me if i'm wrong.

if classifying that as inapplicable is to try and avoid any false positives. i mean... sure? i can't really argue with that. i don't agree with it, but i'm not sure i see the point in diving into that topic any further. i'd at least submit that it should be in a 'needs review' category rather than ignoring it until someone happens to make the element visible.

Jym77 commented 10 months ago

@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 commented 10 months ago

@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 commented 10 months ago

@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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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.

Jym77 commented 6 months ago

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 commented 6 months ago

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 commented 5 months ago

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?

act-rules / act-rules.github.io