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

Rule update: ARIA state/property has valid value #441

Conversation

annethyme
Copy link
Collaborator

@annethyme annethyme commented Mar 14, 2019

Rule limited to only looking at required states and properties that are included in the accessibility tree and does not have a fallback value.

My suggestion is that we keep this rule very strict and limited , since we want to map it to a WCAG success criterion - and then (in time) create another rule that does not mention any WCAG success criteria, but has WAI-ARIA 1.1 as it's requirement, and then let that rule check if everything has a valid value.

Closes issue:

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

@annethyme annethyme self-assigned this Mar 14, 2019
@annethyme annethyme added Rule Update Use this label for an existing rule that is being updated Rule Use this label for a new rule that does not exist already March 31 deadline labels Mar 14, 2019
@annethyme annethyme changed the title [WIP] Rule update: ARIA state/property has valid value Rule update: ARIA state/property has valid value Mar 22, 2019
- This rule assumes that elements that are not [included in the accessibility tree](#included-in-the-accessibility-tree) or are [focusable](#focusable) can still impact users. Therefore the applicability of this rule is not limited to [WAI-ARIA 1.1 states and properties](https://www.w3.org/TR/wai-aria-1.1/#state_prop_def) on elements that are included in the accessibility tree or are focusable.
**Note:** For example, anything referenced through `aria-labelledby` does not have to be [included in the accessibility tree](#included-in-the-accessibility-tree) in order for it to become part of the [accessible name](#accessible-name).
- The ARIA `state` or `property` is being used to comply to WCAG.
- This rule assumes that elements that are not [included in the accessibility tree](#included-in-the-accessibility-tree) or are [focusable](#focusable) can still impact users if they are used for the [accessible name computation](https://www.w3.org/TR/accname-1.1/#mapping_additional_nd_te). Therefore the applicability of this rule also includes `aria-*` attributes that participate in the accessible name computation as well as [WAI-ARIA properties](https://www.w3.org/TR/wai-aria-1.1/#state_prop_def) that are specified on elements that are included in the accessibility tree or are focusable.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand why there's a note on elements that are not included in the accessibility tree, but I don't understand why you point "or are focusable" ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Whole assumption removed now, since scope of rule has been changed once again.

Copy link
Collaborator

@ShadowBB ShadowBB left a comment

Choose a reason for hiding this comment

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

Only passed example 11 and failed example 10 are relevant now.

The applicability only refers to "aria-controls" and no other property. Why not simply limit the applicability to "aria-controls" and mention aria-controls in the name of the rule?


Each test target has a valid value according to its [WAI-ARIA 1.1 value type](https://www.w3.org/TR/wai-aria-1.1/#propcharacteristic_value).

For value types `ID Reference` and `ID Reference List` for [WAI-ARIA required properties](https://www.w3.org/TR/wai-aria-1.1/#requiredState) at least one of the elements with the given ids exists in the same [document tree](https://www.w3.org/TR/dom41/#document-trees) or [shadow tree](https://www.w3.org/TR/dom41/#shadow-trees) as the element that specifies the target attribute.
For value types `ID Reference` and `ID Reference List` at least one of the elements with the given ids exists in the same [document tree](https://www.w3.org/TR/dom41/#document-trees) or [shadow tree](https://www.w3.org/TR/dom41/#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).
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is irrelevant now and can be removed.


For value type `URI` the value matches the [generic URI syntax](https://www.ietf.org/rfc/rfc3986.txt).

**Note:** Only for [WAI-ARIA required properties](https://www.w3.org/TR/wai-aria-1.1/#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.

**Note:**
For value type `URI`, this rule does not require that the destination URI exists.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is irrelevant now and can be removed.

- This rule assumes that elements that are not [included in the accessibility tree](#included-in-the-accessibility-tree) or are [focusable](#focusable) can still impact users. Therefore the applicability of this rule is not limited to [WAI-ARIA 1.1 states and properties](https://www.w3.org/TR/wai-aria-1.1/#state_prop_def) on elements that are included in the accessibility tree or are focusable.
**Note:** For example, anything referenced through `aria-labelledby` does not have to be [included in the accessibility tree](#included-in-the-accessibility-tree) in order for it to become part of the [accessible name](#accessible-name).
- The ARIA `state` or `property` is being used to comply to WCAG.
- This rule assumes that elements that are not [included in the accessibility tree](#included-in-the-accessibility-tree) or are [focusable](#focusable) can still impact users if they are used for the [accessible name computation](https://www.w3.org/TR/accname-1.1/#mapping_additional_nd_te). Therefore the applicability of this rule also includes `aria-*` attributes that participate in the accessible name computation as well as [WAI-ARIA properties](https://www.w3.org/TR/wai-aria-1.1/#state_prop_def) that are specified on elements that are included in the accessibility tree or are focusable.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is irrelevant now and can be removed.

Any [non-empty](#non-empty) [WAI-ARIA 1.1 state or property](https://www.w3.org/TR/wai-aria-1.1/#state_prop_def) that is specified on an HTML or SVG element.
Any [WAI-ARIA required state or property](https://www.w3.org/TR/wai-aria-1.1/#requiredState) that
- is specified on an HTML or SVG element
- is one of the `aria-*` attributes included in the [accessible name computation](https://www.w3.org/TR/accname-1.1/#mapping_additional_nd_te), or is specified on an element that is [included in the accessibility tree](#included-in-the-accessibility-tree) or is [focusable](#focusable),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Accessible name computation can be removed.

@@ -21,26 +21,27 @@ authors:

### Applicability

Any [non-empty](#non-empty) [WAI-ARIA 1.1 state or property](https://www.w3.org/TR/wai-aria-1.1/#state_prop_def) that is specified on an HTML or SVG element.
Any [WAI-ARIA required state or property](https://www.w3.org/TR/wai-aria-1.1/#requiredState) that
Copy link
Collaborator

Choose a reason for hiding this comment

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

The applicability now only refers to aria-controls and no other property. We could just state that in the applicability and the name of the rule.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ShadowBB , we discussed internally if that was the way forward for this rule.
While it's the only required property in WAI-ARIA 1.1, there could potentially also be roles and requirements specified in other standards, e.g. EPUB.
Having "broader" scoped rule could actually catch something in other standards, that maps to a completely different success criterion, I guess.
So I guess I am for your suggestions of limiting the rule to explicitly aria-controls, and then we can be completely sure what we are testing...

Copy link
Collaborator Author

@annethyme annethyme Mar 27, 2019

Choose a reason for hiding this comment

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

aria-controls is required for the roles scrollbar and combobox. I have now limited the rule to only look for aria-required on role scrollbar, since combobox is a story of its own, see w3c/aria#716.

@annethyme annethyme dismissed stale reviews from kasperisager and ShadowBB March 27, 2019 09:10

Changed the whole name of the rule...

…value.md to SC-1-3-1-aria-controls-for-scrollbar-has-valid-value.md
@annethyme
Copy link
Collaborator Author

For naming of the rule I am not sure whether to go with:

  • aria-controls for scrollbar has valid value
  • aria-controls for scrollbar references existing element

I think the latter is more precise, but also a bit longer.
In the first one it is a bit unclear whether we are just looking for the right format or also the existence of the elements.

@ShadowBB
Copy link
Collaborator

For naming of the rule I am not sure whether to go with:

* aria-controls for scrollbar has valid value

* aria-controls for scrollbar references existing element

I think the latter is more precise, but also a bit longer.
In the first one it is a bit unclear whether we are just looking for the right format or also the existence of the elements.

I would go with the second name as it is more precise and only slightly longer.

Copy link
Collaborator

@ShadowBB ShadowBB left a comment

Choose a reason for hiding this comment

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

Although it is a really limited rule now, the rule itself is correct as far as I can see. Approved.

@audreymaniez
Copy link
Collaborator

I don't really understand how you get from all the properties to only one. But if I understand the general idea, you want to focus on required properties only, and the role scrollbar seems to have other required ARIA properties. Does that mean that it will be one rule per required property (and per role) ?

@annethyme
Copy link
Collaborator Author

@audreymaniez, great question!
The thing is, that for most required states and properties, a fallback value is specified for that state/property for that role in the table section "Implicit Value for Role:"
For scrollbar the only required state/property that does not have an implicit value defined is aria-controls.
And through the entire WAI-ARIA 1.1 spec, aria-controls for scrollbar is the only required state/property that does not have an implicit value defined. And then there is the combobox, but here it has to be on a descendent of a specific type, and only when the thing is expanded.

…e.md to SC-1-3-1-aria-controls-for-scrollbar-references-existing-element.md
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.

Hey Anne.
I haven't had a chance to review the rule yet, but I definitely don't agree with deleting another rule as part of this PR. Can you undo that change? We can discuss whether or not that rule needs to be deprecated or changed, but we can't simply delete it.

@annethyme
Copy link
Collaborator Author

@WilcoFiers, I think it's because I opened the rule, started working on it, and then renamed the file somewhere down the line.
I guess I'll just have to add the original rule file again to make it un-deleted...

@annethyme
Copy link
Collaborator Author

@WilcoFiers, and it's back again...

@annethyme
Copy link
Collaborator Author

By the way, I don't personally like this rule very much.
There is a lot of code to check for one specific edge case, just because we really want to connect a WAI-ARIA validation issue back to a WCAG success criterion.

This non-WCAG rule checks for all cases of invalid state/property values, but we list any WCAG success criteria as an accessibility requirement, because it could be many different success criteria, or non at all, that are impacted depending on the state/property, and the semantic role of the element that it is specified on: #464

But is this even a problem?

@WilcoFiers
Copy link
Member

Anne wrote:
By the way, I don't personally like this rule very much.

xD Then why are you writing it? I agree, this seems far too much of an edge case to worry about. As an implementor, I would certainly have my doubts about taking on the overhead for a rule that has such a narrow scope.

@annethyme seriously, write the rules you feel good about writing.

@annethyme annethyme dismissed WilcoFiers’s stale review April 1, 2019 11:20

Rule un-deleted as requested.

@jeeyyy jeeyyy changed the base branch from master to develop April 12, 2019 11:34
@annethyme
Copy link
Collaborator Author

FYI, @WilcoFiers: I am going to close this PR down for now to unclutter, until we have a solution for #365.

@annethyme annethyme closed this Apr 29, 2019
@Jym77 Jym77 deleted the update-SC-1-3-1-required-aria-state-or-property-has-valid-value branch January 22, 2020 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
On hold Rule Update Use this label for an existing rule that is being updated 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.

6 participants