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

"ARIA required context role" (ff89c9): resolve TF feedback #1554

Merged
merged 7 commits into from
Jun 17, 2021

Conversation

Jym77
Copy link
Collaborator

@Jym77 Jym77 commented Mar 11, 2021

As per w3c/wcag-act#507
This mostly inlines our definition of "owned by" into "child in the accessibility tree" to avoid the confusion with the ARIA definition. Notes are added to the background to explain a bit the reasoning behind that choice.

Note that the definition is contested inside ARIA: w3c/aria#748 which is a good reason to not use it 😀

Closes issue(s):

  • N/A

Need for Final Call:
This will require a 1 week Final Call (technically only editorial, but changes structure quite a bit)


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 Final Call period. In case of disagreement, the longer period wins.

Copy link
Collaborator

@ajanec01 ajanec01 left a comment

Choose a reason for hiding this comment

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

Description for passed example 3 seems incorrect to me. Happy to approve otherwise.

_rules/aria-required-context-role-ff89c9.md Outdated Show resolved Hide resolved
_rules/aria-required-context-role-ff89c9.md Outdated Show resolved Hide resolved
The definition of [owned by][] used in this rule is different from the definition of ["owned element" in WAI-ARIA](https://www.w3.org/TR/wai-aria-1.1/#dfn-owned-element). In this rule "owned by" includes only direct children of the owner element, whereas an "owned element" is any descendant of the owner.
Being a child in the [accessibility tree][] is different from being a child in the DOM tree. Some DOM nodes have no corresponding node in the [accessibility tree][] (for example, because they are marked with `role="presentation"`). A child in the [accessibility tree][] can thus correspond to a descendant in the DOM tree. Additionally, the use of `aria-owns` attribute can change the tree structure to something which is not a subtree of the DOM tree.

This rule is restricted to direct parent-child relation in the [accessibility tree][] which is more strict than the definition of ["owned element" in WAI-ARIA](https://www.w3.org/TR/wai-aria-1.1/#dfn-owned-element). This rule mimics, on the roles level, the [content model](https://html.spec.whatwg.org/#concept-element-content-model) of HTML.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we delete the sentence where we are saying that this rule mimics the HTML content model? Asking because the definition of the content model is suggesting that it's both children and descendants of the element/ test target.

It's contradictory to the beginning of the paragraph where we insist that this rule is restricted to direct parent-child relation.

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 Aron's suggestion

Copy link
Member

Choose a reason for hiding this comment

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

I can go either way on this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, it depends where we look 🤯
The link used here indeed says

A normative description of what content must be included as children and descendants of the element.

but a couple of line below we have 3.2.5 Content models stating

Each element defined in this specification has a content model: a description of the element's expected contents. An HTML element must have contents that match the requirements described in the element's content model. The contents of an element are its children in the DOM.

🤷


As far as I remember, HTML content model only refers to children, not further descendants (despite what the def says).


Additionally, the idea is not just to consider a single level in the tree (e.g., the rule allows the table > rowgroup > row chain which is 2 levels) but more than the chain have to be considered between parent/children and not ancestors/descendant.

Using the ARIA def of "owned element", something like table > rowgroup > navigation > row would be allowed because the row is "owned by" the rowgroup (as a descendant) but is not a "child in the accessiblity tree".

So the difference is not about only looking at children, but about looking one step at a time and not skipping any.

The reason why we do this stepwise checks (disregarding ARIA specs) is that the content model is doing likewise. That is <table><tbody><nav><tr> is not OK because <nav> is not allowed in a <tbody>. The content model is only looking one step at a time, and individual elements' content model only mention children, but builds up a model that restrict descendants further away.

Essentially, performing the same checks on implicit roles (only) should flag violations of the content model (not all of them, but some of them).


So, this is what I was trying to say there…

  • We stray away from ARIA def.
  • We do so because we take inspiration from content model.

I'm afraid that keeping only the first part (the action) without the second (its justification) could lead to much confusion ("why are you not following ARIA specs?")

Not sure how to rephrase it to convey that information… I'll let it simmer a bit in case any of you has a brillant idea 😁
Easy fix might just be to switch the link and use the other one that does not mention descendants…

Copy link
Member

Choose a reason for hiding this comment

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

I understand, and agre, with your reasoning.
Unfortunately, I don't have a better idea than what you suggested (i.e. change the link target).

The definition of [owned by][] used in this rule is different from the definition of ["owned element" in WAI-ARIA](https://www.w3.org/TR/wai-aria-1.1/#dfn-owned-element). In this rule "owned by" includes only direct children of the owner element, whereas an "owned element" is any descendant of the owner.
Being a child in the [accessibility tree][] is different from being a child in the DOM tree. Some DOM nodes have no corresponding node in the [accessibility tree][] (for example, because they are marked with `role="presentation"`). A child in the [accessibility tree][] can thus correspond to a descendant in the DOM tree. Additionally, the use of `aria-owns` attribute can change the tree structure to something which is not a subtree of the DOM tree.

This rule is restricted to direct parent-child relation in the [accessibility tree][] which is more strict than the definition of ["owned element" in WAI-ARIA](https://www.w3.org/TR/wai-aria-1.1/#dfn-owned-element). This rule mimics, on the roles level, the [content model](https://html.spec.whatwg.org/#concept-element-content-model) of HTML.
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 Aron's suggestion

_rules/aria-required-context-role-ff89c9.md Outdated Show resolved Hide resolved
_rules/aria-required-context-role-ff89c9.md Outdated Show resolved Hide resolved
_rules/aria-required-context-role-ff89c9.md Outdated Show resolved Hide resolved
Copy link
Member

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

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

I agree with much of what Carlos has said.

The definition of [owned by][] used in this rule is different from the definition of ["owned element" in WAI-ARIA](https://www.w3.org/TR/wai-aria-1.1/#dfn-owned-element). In this rule "owned by" includes only direct children of the owner element, whereas an "owned element" is any descendant of the owner.
Being a child in the [accessibility tree][] is different from being a child in the DOM tree. Some DOM nodes have no corresponding node in the [accessibility tree][] (for example, because they are marked with `role="presentation"`). A child in the [accessibility tree][] can thus correspond to a descendant in the DOM tree. Additionally, the use of `aria-owns` attribute can change the tree structure to something which is not a subtree of the DOM tree.

This rule is restricted to direct parent-child relation in the [accessibility tree][] which is more strict than the definition of ["owned element" in WAI-ARIA](https://www.w3.org/TR/wai-aria-1.1/#dfn-owned-element). This rule mimics, on the roles level, the [content model](https://html.spec.whatwg.org/#concept-element-content-model) of HTML.
Copy link
Member

Choose a reason for hiding this comment

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

I can go either way on this.

@Jym77 Jym77 added Review Call 1 week Call for review for small changes and removed ACT TF request reviewers wanted labels Jun 10, 2021
@Jym77
Copy link
Collaborator Author

Jym77 commented Jun 10, 2021

Call for review ends on June 17th.

@Jym77
Copy link
Collaborator Author

Jym77 commented Jun 17, 2021

Call for review has ended. Merging.

@Jym77 Jym77 merged commit 1fd9a3f into develop Jun 17, 2021
@Jym77 Jym77 deleted the aria-required-context-role-tf-review branch June 17, 2021 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review Call 1 week Call for review for small changes Rule Update Use this label for an existing rule that is being updated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants