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 state or property: various editorial improvements #2012

Merged
merged 13 commits into from
Apr 6, 2023
Merged

Conversation

WilcoFiers
Copy link
Member

@WilcoFiers WilcoFiers commented Jan 18, 2023

This is addressing feedback from the recent ACT TF survey. I've also updated several examples, and filled in a few gaps.

Closes #1849

Need for Call for Review: 1 week, since I'm updating a number of examples

This PR is closely related to #2041, which introduces a new rule that ensures required ID references exist in the page. We're basically splitting that part of this rule off into its own.


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 previously requested changes Jan 19, 2023
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.

The whole aria-controls on combobox may be better taken in a separate PR, but I think this PR won't close #1849 until it's done.
And it may also require us to have the same exception on "Element with role attribute has required states and properties".

I did tried to give it a shot before seeing this PR 😅 : #2014, maybe things are salvageable from it.

For value types `ID Reference` and `ID Reference List` for [WAI-ARIA required properties](https://www.w3.org/TR/wai-aria-1.2/#requiredState) at least one of the elements with the given ids exists in the same [document tree](https://dom.spec.whatwg.org/#document-trees) or in the same [shadow tree](https://dom.spec.whatwg.org/#shadow-trees) as the element that specifies the target attribute.

For value type `URI` the value matches the [generic URI syntax](https://www.ietf.org/rfc/rfc3986.txt).
**Exception**: For `ID Reference` and `ID Reference List` value types, if the test target is not a [WAI-ARIA required states and properties][] for the [semantic role][] of its element, no ID referenced elements are required. Otherwise at least one of the elements with the given IDs exists in the same [document tree][] or in the same [shadow tree][] as the test target's element.
Copy link
Collaborator

Choose a reason for hiding this comment

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

To fully close #1849, I think we need to consider aria-controls as not required on closed combobox (and thus include it in this exception).
This matches the combobox authoring practice, and follows suit with changes made on ARIA 1.3 on combobox. And this is what we discussed on June 23rd.

From the authoring practices (second bullet):

Note that aria-controls only needs to be set when the popup is visible.

ARIA Issue and PR: w3c/aria#1334 / w3c/aria#1335

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a fair point!

_rules/aria-state-or-property-valid-value-6a7281.md Outdated Show resolved Hide resolved
_rules/aria-state-or-property-valid-value-6a7281.md Outdated Show resolved Hide resolved
_rules/aria-state-or-property-valid-value-6a7281.md Outdated Show resolved Hide resolved
_rules/aria-state-or-property-valid-value-6a7281.md Outdated Show resolved Hide resolved
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 think that works.
Exploring a bit different ways of phrasing it, but the current version is good.

Comment on lines 35 to 39
Each test target has an [attribute value][] that is valid according to its [WAI-ARIA value type][value type], except if one of the following is true:

For value types `ID Reference` and `ID Reference List` for [WAI-ARIA required properties](https://www.w3.org/TR/wai-aria-1.2/#requiredState) at least one of the elements with the given ids exists in the same [document tree](https://dom.spec.whatwg.org/#document-trees) or in the same [shadow tree](https://dom.spec.whatwg.org/#shadow-trees) as the element that specifies the target attribute.
- <dfn id="off6ek:id-reference">ID Reference</dfn>: For `ID Reference` value types an ID referenced elements is only required for `aria-controls` with [semantic][semantic role] `scrollbar` elements, and with [semantic][semantic role] `combobox` elements that have an `aria-expanded` [attribute value][] of `true`. The ID referenced element must exist in the same [document tree][] or [shadow tree][] as the test target's element.

For value type `URI` the value matches the [generic URI syntax](https://www.ietf.org/rfc/rfc3986.txt).
- <dfn id="off6ek:id-reference-list">ID Reference List</dfn>: For `ID Reference List` value types, no ID referenced elements are required.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel the exceptions are getting somewhat out of hand and hard to read. Trying something in a different direction to see where it goes…

For each test target, depending on its type, either:

  • the type is neither ID reference, nor ID reference list, and the value is valid; or
  • the type is ID reference, and the attribute is required, and this is not aria-controls on a combobox with aria-expanded != true, and the value is an existing id; or
  • all other cases pass.

(plus adding Background note that currently no ID reference list is required, and that apart from combobox, the only required ID reference is aria-controls on a scrollbar)

I prefer listing the combobox as an exception as it makes the rule more future proof (if a new required ID ref comes up, we don't need to change the rule)


Maybe it can be make clearer by splitting the requirements:

Expectation 1
For each test target, either the type is ID reference/ID reference list; or the value is valid.

Expectation 2
For each test target, either the the type is not ID reference, or it is not required, or it is aria-controls on an closed combobox, or the value is an existing id.

(again with Background note to explain the reasoning and effect)
This one might makes it harder to see when Expectation 2 triggers since it gets all the negation 🤔

Only for [WAI-ARIA required properties](https://www.w3.org/TR/wai-aria-1.2/#requiredState) with value types `ID Reference` and `ID Reference List` is there a requirement that the elements with the given ids actually exists. For non-required properties, this is not a requirement. For example, the value of the `aria-errormessage` attribute on an `input` does not need to reference an `id` that exists within the same document, because an [HTML element](https://html.spec.whatwg.org/#htmlelement) with such and `id` may be created in response to an [event](https://dom.spec.whatwg.org/#event) that may or may not happen.

For value type `URI`, this rule does not require that the destination URI exists.
Only for [WAI-ARIA required properties][] with value types `ID Reference` and `ID Reference List` is there a requirement that the elements with the given IDs actually exists. For non-required properties, having the referenced element is optional. For example, `aria-errormessage` attribute on an `input` element may have a fixed value, but the element with the error message is only added to the page when an error actually occurred.
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should expand on that to explain the combobox exception.

@carlosapaduarte
Copy link
Member

From the CG call: remove the exceptions from this rule and create a second rule to address the exceptions (in the first rule include a background note stating the second rule is under development)

Jym77
Jym77 previously requested changes Mar 16, 2023
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.

(requesting change for one description update)


I'm not a huge fan of splitting the rule. I feel the other rule could more or less just be an Expectation 2 in this one. I won't block anything on that account, but it feels we are diluting things a bit too much.

_rules/aria-state-or-property-valid-value-6a7281.md Outdated Show resolved Hide resolved
@WilcoFiers
Copy link
Member Author

@Jym77 The reason to split this is because that second expectation got unnecessarily complicated. A new rule lets us keep things simple. It also fits better with the requirement to write atomic rules. There are other rules for which we may do things like this too. For example an allowed ARIA attribute is different from a prohibited attribute, from a deprecated attribute.

@WilcoFiers WilcoFiers added the Review Call 1 week Call for review for small changes label Mar 30, 2023
@WilcoFiers WilcoFiers merged commit 10d7307 into develop Apr 6, 2023
@WilcoFiers WilcoFiers deleted the aria-idref branch April 6, 2023 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority Review Call 1 week Call for review for small changes reviewers wanted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should ARIA IDREFs existing be required? [6a7281]
5 participants