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

update ARIA state or property has valid value rule to current writing style #1433

Closed
wants to merge 17 commits into from

Conversation

jeeyyy
Copy link
Collaborator

@jeeyyy jeeyyy commented Aug 26, 2020

  • Updated applicability
  • Updated the example descriptions

Need for Final Call:

  • 1 week

Note: There is an outstanding issue to tackle the expectation around ID(REFs) - #1432

Jym77
Jym77 previously requested changes Sep 2, 2020
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 change in Applicability is breaking the rule.
The rest are mostly editorials.

_rules/aria-state-or-property-valid-value-6a7281.md Outdated Show resolved Hide resolved
Comment on lines 35 to 38
**Note:** 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, 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.

The notes could probably move to the Background section. The first one might be expanded a bit to explain why this extra requirement is only for required properties (why is it OK for non-required properties to reference non-existing IDs).
(the second note is fairly obvious: the rule checks the syntax of the URL, but is not a broken link checker)

_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
_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
@jeeyyy
Copy link
Collaborator Author

jeeyyy commented Sep 7, 2020

@Jym77 thanks for the round of review. I am not entirely following your argument around - "not mandatory for non-required properties to reference non-existing IDs". Help appreciated.

Other comments have been addressed.

@jeeyyy jeeyyy requested a review from Jym77 September 10, 2020 14:17
Co-authored-by: Jean-Yves Moyen <jym@siteimprove.com>
Jym77
Jym77 previously requested changes Sep 21, 2020
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.

#1433 (comment) and #1433 (comment) still need to be addressed.

@jeeyyy jeeyyy requested a review from Jym77 October 4, 2020 16:28

**Note:**
For value type `URI`, this rule does not require that the destination URI exists.
Each test target has a valid value according to its [WAI-ARIA 1.1 value type][]. For [WAI-ARIA required properties][] with [value type][] of `ID reference` and `ID reference list` at least one of the referenced elements with the given ids should exist in the same [document tree][] or [shadow tree][].
Copy link
Member

Choose a reason for hiding this comment

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

Can you please review the comments again? This rule stats that these only need to exist if the ARIA attribute is required for that role.

@WilcoFiers
Copy link
Member

Closing, going to start from scratch.

@WilcoFiers WilcoFiers closed this Dec 2, 2020
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.

3 participants