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 RULES] - Define author's responsibility when presentational roles conflict arises #2195

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

Conversation

giacomo-petri
Copy link
Collaborator

New Rules:

  • Check that elements with ARIA presentational role are not focusable
  • Check that allowed child element of another element with presentational role does not cause presentational roles conflicts
  • Check that elements with ARIA presentational role do not have global states or properties

Closes: #2193

Need for Call for Review:
This will require a 2 weeks 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.

allowed child element of another element with presentational role does not cause presentational roles conflicts
elements with ARIA presentational role do not have global states or properties
@giacomo-petri giacomo-petri added the Rule Use this label for a new rule that does not exist already label Jun 7, 2024
@giacomo-petri giacomo-petri self-assigned this Jun 7, 2024
Comment on lines +5 to +6
description: |
This rule checks that allowed child element of another element with presentational role does not cause presentational roles conflicts
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 seems like a subset of our ARIA required context role rule. It requires that elements that can't exist without a parent have that parent. The rule you propose is more focused, but it doesn't seem to cover anything that isn't also covered by the required context rule. Or at least, I can't find anything that isn't.

Would a better solution be to add the one or both of the failed examples to required context instead?

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 see the point.

The intent was to cover a specific ARIA requirement in detail and comprehensively (Presentational Roles Conflict Resolution) to guarantee authors are clear on what to do in these scenarios.

I'm uncertain about the next steps.

The other two existing ARIA requirements are rather more vague but do cover the necessary aspects. If we decide to revert these new rules, we'll still need to add examples to the existing rules.

@@ -0,0 +1,175 @@
---
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 think you should have more than one new rule in a PR.

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 apologize for this. I based my work on this pattern: #2167. These three rules are intended to fulfil the "Presentational Roles Conflict Resolution" topic from an author's perspective.

@@ -0,0 +1,175 @@
---
id: p8g918
name: ARIA presentational role does not have global states or properties
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 very sorry to do this a second time. I can tell you've put a lot of work into these, but this rule also seems like a duplicate to me. This time of Element marked as decorative is not exposed . Your version makes slightly different trade-offs about explicitly stating which props cause this problem, whereas the existing rule is vaguer about that. But then in being vaguer and just saying "not in the accessibility tree" its means it can handle changes to the ARIA spec like how aria-label is not a global attr if on role=none, but still fails for creating presentation conflict.

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 see the point.

The intent was to cover a specific ARIA requirement in detail and comprehensively (Presentational Roles Conflict Resolution) to guarantee authors are clear on what to do in these scenarios.

I'm uncertain about the next steps.

The other two existing ARIA requirements are rather more vague but do cover the necessary aspects. If we decide to revert these new rules, we'll still need to add examples to the existing rules.

@@ -0,0 +1,139 @@
---
id: 18pg11
name: ARIA presentational role not focusable
Copy link
Member

Choose a reason for hiding this comment

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

See above, the Element marked as decorative is not exposed rule covers this.

I think we can debate splitting the rule up into the two rules you propose. I'd personally lean towards leaving this as is, since the "not exposed" rule is already approved by AG, but if others feel there are good reasons to deprecate it in favor of this I'm open to that conversation.

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 see the point.

The intent was to cover a specific ARIA requirement in detail and comprehensively (Presentational Roles Conflict Resolution) to guarantee authors are clear on what to do in these scenarios.

I'm uncertain about the next steps.

The other two existing ARIA requirements are rather more vague but do cover the necessary aspects. If we decide to revert these new rules, we'll still need to add examples to the existing rules.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Agenda item 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.

[New Rule] Presentational Roles Conflict is not triggered
3 participants