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

Split aria-state-or-property-permitted-5c01ea.md #2036

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

Conversation

tbostic32
Copy link
Collaborator

@tbostic32 tbostic32 commented Feb 23, 2023

Splitting the aria-state-or-property permitted into multiple rules. Essentially changing from having the 2 expectation where Expectation 1 is the state or property is permitted and Expectation 2 is the state or property is not prohibited, to 2 separate rules for each of the expectations. We also expect to have a rule for aria state or property is not deprecated in the future.

Need for Call for Review:
This will require a 2 week Call for Review


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.

Updating to remove expectation 2 as we are going to move that to a new rule.
Adding myself as a possible author.
package.json Show resolved Hide resolved
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.

Please correct "Expectation 1" to "Expectation"

Copy link
Collaborator

@Jym77 Jym77 left a comment

Choose a reason for hiding this comment

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

I'm not a fan of splitting the rule.
It seems the new rule (#2037) will have exactly the same Applicability, and the former Expectation 2 as its only Expectation. I do not think this add value compared to having two expectation in a rule (this is pretty much the reason why rules can have more than one expectation).

It feels we are going to duplicate bits, need to maintain in two places (e.g. if we figure out the Applicability needs fixing), … So I get the feeling we are making things more complex to maintain without bringing any real value (we are even diluting the rule and loosing a bit of cohesion).

Thus said, I won't block it for that reason.

@tbostic32
Copy link
Collaborator Author

It feels we are going to duplicate bits, need to maintain in two places (e.g. if we figure out the Applicability needs fixing), … So I get the feeling we are making things more complex to maintain without bringing any real value (we are even diluting the rule and loosing a bit of cohesion).

@Jym77 I do agree, and I left a comment on that new rule proposal (#2037) that we were reusing the applicability and the the passed examples from this rule. I think the larger conversation we had is that there is also likely to be a third rule around deprecated properties which will follow a very similar format to each of these rules as well. In the end, I think the decision was to just split them fully apart (each to it own atomic rule) for simplicity in covering each of those cases specifically with the knowledge that it does introduce a bit more overhead.

authors:
- Anne Thyme Nørregaard
- Jean-Yves Moyen
- Trevor Bostic
Copy link
Collaborator Author

@tbostic32 tbostic32 Apr 5, 2023

Choose a reason for hiding this comment

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

Should I keep all of the authors? Much of the content was created by Anne and Jean-Yves so I want to make sure proper credit is given, but not have people feel like I am putting words in their mouth.

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 significant change. Probably better to put them under previous_authors

@Jym77 Jym77 changed the title Update aria-state-or-property-permitted-5c01ea.md Split aria-state-or-property-permitted-5c01ea.md Apr 13, 2023

## Expectation

No test target (i.e., state or property) is prohibited on the semantic role of the element on which it is specified.
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 it would be good for the word "prohibited" to link to the corresponding section in WAI-ARIA. There are lots of ways in which an attribute may not be allowed, and without a link this could be misunderstood to be broader than it actually is.


## Expectation

No test target (i.e., state or property) is prohibited on the semantic role of the element on which it is specified.
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 fan of the "(i.e. state or property)" bit. If we were to do this in all rules I'm concerned this would get messy. If we feel that "test target" as a term is confusing we should have a separate conversation about that, and then adjust all rules to fit the outcome. I don't think we want to introduce changes like this without thinking through the broader implications.


## Test Cases

### Passed
Copy link
Member

Choose a reason for hiding this comment

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

aria-pressed is never prohibited. I think it would be good to (also?) include examples of attributes that can be prohibited.

@tbostic32 tbostic32 dismissed stale reviews from WilcoFiers and carlosapaduarte June 26, 2023 17:47

Need re-review


## Expectation

No test target is [prohibited](https://www.w3.org/TR/wai-aria-1.2/#prohibitedattributes) on the semantic role of the element on which it is specified.
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if semantic role is right here. It creates this strange interaction with presentational conflict resolution. If you agree with my suggestion on the applicability, the perhaps what needs to go here is something like this:

Suggested change
No test target is [prohibited](https://www.w3.org/TR/wai-aria-1.2/#prohibitedattributes) on the semantic role of the element on which it is specified.
No test target is [prohibited](https://www.w3.org/TR/wai-aria-1.2/#prohibitedattributes) on the element's role. The role here is the [explicit role][] if one exists, or the [implicit role][] otherwise.
**note** The role here is different from the [semantic role][], as it does not [consider presentational role conflict resolution][].


## Applicability

This rule applies to any [WAI-ARIA state or property][] that is specified on an [HTML or SVG element][namespaced element] that is [included in the accessibility tree][].
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if you meant for this rule to skip prohibited attributes on role=none/presentation/generic, but by using Included in the accessibility tree that's what happens. Those aren't in the accessibility tree. That stuff is covered by Decorative element not exposed though, so maybe that's okay? I don't think ARIA WG intended for those to be ignored. In ARIA 1.2 that includes almost all prohibited attributes.

One thing you could do here is to use programmatically hidden instead:

Suggested change
This rule applies to any [WAI-ARIA state or property][] that is specified on an [HTML or SVG element][namespaced element] that is [included in the accessibility tree][].
This rule applies to any [WAI-ARIA state or property][] that is specified on an [HTML or SVG element][namespaced element] that is not [programmatically hidden][].

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Failed example 1 shows a failure on a generic role, so we would either need to change the applicability here, or we need to remove that example.

Assessing the value of the attribute is out of scope for this rule.

### Related rules

Copy link
Member

Choose a reason for hiding this comment

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

This rule has an interesting interaction with Decorative element not exposed. It almost seems to me that this rule should cover everything that rule does, except that you've sidestepped all the overlapping cases by looking at elements included in the accessibility tree, which avoid anything role=none/presentation.

Suggested change
- [Element marked as decorative is not exposed](https://www.w3.org/WAI/standards-guidelines/act/rules/46ca7f/)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

With your suggested change to the applicability, we would have some overlapping cases, which I think might just complicate things. As I've been digging more into it, I'm becoming more inclined to leave the applicability and expectation as they are.

With that said, I think its still work adding this as a related rule so I'm going to add this suggestion in.

The `aria-label` property is [prohibited](https://www.w3.org/TR/wai-aria-1.2/#prohibitedattributes) for an element with a generic role.

```html
<div aria-label="Bananas"></div>
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 sure this element is actually in the accessibility tree. ARIA 1.3 draft contradicts itself on that:

https://w3c.github.io/aria/#scope
user agents MUST ignore any elements with the role generic or none.

https://w3c.github.io/aria/#generic
However, unlike elements with role presentation, generic elements are exposed in accessibility APIs

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I could leave or delete this example, I think we could point to this sentence and say that it should be in the tree.

However, unlike elements with role presentation, user agents expose generic elements in accessibility APIs when permitted accessibility attributes have been specified.

@@ -0,0 +1,160 @@
---
id: yyw7k3
name: ARIA state or property is not prohibited
Copy link
Member

Choose a reason for hiding this comment

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

How would you feel about having this rule as a replacement of "Element marked as decorative is not exposed"? I think right now this rule avoids most overlap between the two rules, but it seems to me that the way the ARIA WG intends prohibited attributes to work, this rule isn't broad enough, and most of what it's missing is covered by the other rule. I left a couple suggestions, if you want to go that direction, I think we should consider deprecating the other rule.

@Jym77 I would appreciate it you'd weigh in on this too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, I apparently missed this comment on my first look through. I agree with the sentiment here though, if we want to expand this rule with the applicability you suggested, then I think we should deprecate "Element marked as decorative is not exposed". Additionally, since "Element marked as decorative" doesn't have any accessibility requirements, including those failed cases under this rule would give more direction on what we think reporting requirements are.

TL;DR - In favor of expanding this rule and deprecating "Element marked as decorative"

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seemed to have missed the initial comment also… No fully sure I remember all the details, but…

  • I believe that "element marked as decorative" (together with "aria-hidden no focusable content" and "presentational children no focusable content") can be consolidated into "presentational conflict is not triggered" (Merge "Element with aria-hidden has no focusable content" (6cfa84) and "Element with presentational children has no focusable content" (307n5z) #1507). It seems all 3 check a different aspect of the conflict. I've also gathered from a couple of discussions that ARIA group more or less considers triggering the conflict as an author error (even though it didn't make to the spec).
  • This rule is conceptually a bit different since prohibited attributes could be something else. Notably, some roles have prohibited naming (and thus attributes aria-label(led-by)) without triggering the conflict.
  • I somewhat remember a discussion (maybe here 😅) about the fact that role="none" aria-label="foo" does trigger the conflict, thus the role is not none and the element is included in the accessibility tree (plus, the aria-label may be not prohibited on the effective role). I think we concluded that ARIA intention was to consider this as a prohibited attribute nonetheless.

So, I'm rather in favour of deprecating "element marked as decorative" which is a bit half-baked and too narrow in its focus, and that we can do much better in terms of intention now. But I'm not sure that this rule is the best fit for the deprecation. I'm also not against a bit of overlap between "attribute not prohibited" and "conflict not triggered" rules, given that they do test with very different focus in mind, and also report a lot of different things on their own.

Co-authored-by: Wilco Fiers <WilcoFiers@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants