Rule: SC4-1-2-role-has-required-states-and-properties #167

jeeyyy · 2018-06-24T21:06:02Z

Checklist for Pull Request

🚨Please review the guidelines for contributing to this repository.

Make sure you are requesting to pull a issue/feature/bugfix branch (right side). Don't request your master branch!
Make sure you are making a pull request against the master branch (left side). Also you should start your branch off master branch.
Kindly prefix the pull request name with DRAFT or FINAL accordingly. For more details, read Rule Design Guidelines
Provide a reference to the issue being addressed.

#Reviewing Guidelines

For DRAFT PR's, leave any comments you have and approve if you’ve reviewed the entire pull request.
Once there have been three such reviews the pull request can be merged.
Any comments left will be taken up in an updated pull request, which will be necessary before a FINAL release”.
For FINAL PR's, review and approve changes to be merged.

How to Review And Approve

Go to the “files changed” tab, there 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” and click “Submit review”

Description

Final PR based on Draft - #163

💔Thank you!

ShadowBB

My comment on the original draft was not implemented:

"This read like a note. I think the assumption itself could be slightly more concise. Something along the lines of "leaving out required states and properties if an implicit value for role is specified in WAI-ARIA is not fully accessibility supported.""

Was there something wrong with this comment?

WilcoFiers

I don't understand why this second expectation is present - wouldn't it be easier to have a separate rule that just says that any aria-* attribute that's used has a valid value?

Second question, why do we not consider it sufficient for required properties to rely on the default values? It seems to me like that's why that stuff is there - and why so many attributes are required as of 1.1.

I'd also like to see a few more examples for this (and have the '' replaced with ")

kasperisager

I second @WilcoFiers' point on leaving out the second expectation and implementing a second rule that validates used ARIA attributes, required or otherwise.

As for not relying on implicit values, this has been quite a point of discussion internally at Siteimprove. Our conclusion was that implicit values cannot be relied on for required states and properties based on the following paragraph from the ARIA spec:

Content authors MUST provide a non-empty value for required states and properties. Content authors MUST NOT use the value undefined for required states and properties, unless undefined is an explicitly-supported value of that state or property.

Source: https://www.w3.org/TR/wai-aria/#requiredState

WilcoFiers · 2018-07-03T08:25:12Z

@kasperisager I'm not sure you're right about that. I did some quick testing, and browsers certainly seem to just assume the default value if you don't set anything. Having default values doesn't also make a whole lot of sense if you than don't allow content authors to use them. I've created an issue on the ARIA github repo, hopefully someone there can give some insight:
w3c/aria#787

kasperisager · 2018-07-03T08:39:22Z

I sure hope I'm wrong for the same reason you mention; it would make implicit values pretty useless.

fixed quotation mark in second failed test case.

Removed Expectation 2 (created new rule to address this #176) Removed notes under Expectation 2 that relate to #176 Removed second assumption that relates to #176 Removed 3rd Background reference that relates to #176

Replaces ' ' with " " in passed test case

Removed second test case which relates to #176

Removed aria-controls value from Passed text case as the property value is now out of scope.

@annethyme

Added second test case to cover the assumption that @annethyme will update.

Updating "Assumptions" to handle comments from ShadowBB, WilcoFiers and kasperisager about support for implicit values.

Split out Passed test case in two, one for state and one for property, and edited note on second Failed test case. Added note for Expectation.

Split out Expectation 2 in seperate rule and edited Assumption. Please see if you can approve changes.

Split out Expectation 2 in seperate rule and edited Assumption. Please see if you can approve changes

annethyme · 2018-07-11T08:51:36Z

@ShadowBB and @WilcoFiers, I think @Brynanders and I have handled all of your comments.
Could you guys review again?

WilcoFiers

@annethyme I do not believe that attributes required attributes with default values should be mandatory. We've not had a clear answer from the ARIA group, but it seems fairly obvious that this is how it's intended. As far as I can tell when testing with AT, the default values are used when attributes are omitted.

Requested change, argued as not mandatory. Review again.

kasperisager · 2018-10-01T13:53:28Z

_rules/SC4-1-2-role-has-required-states-and-properties.md


 ### Expectation

-For each test target all attributes listed under [required states and properties](https://www.w3.org/TR/wai-aria/#requiredState) in [WAI-ARIA](https://www.w3.org/TR/wai-aria) for that role are present and [non-empty](#non-empty), unless they have a default value listed under [implicit value for role](https://www.w3.org/TR/wai-aria-1.1/#implictValueForRole).
+For each test target, the [required states and properties](https://www.w3.org/TR/wai-aria/#requiredState) for the role is present, unless the attribute has a default value listed under [implicit value for role](https://www.w3.org/TR/wai-aria-1.1/#implictValueForRole). The list of required attributes for a role can be found in [WAI-ARIA 1.1](https://www.w3.org/TR/wai-aria)"


"for the role are".

All comments are hopefully handled. Please review again

Should be fixed now. Please review again.

carlosapaduarte

Besides all my comments, make sure to replace ´ with ` in several of the examples' comments

carlosapaduarte · 2018-10-01T20:00:26Z

_rules/SC4-1-2-role-has-required-states-and-properties.md

+
+### Expectation
+
+For each test target, the [required states and properties](https://www.w3.org/TR/wai-aria/#requiredState) for the role are present, unless the state or property has a default value listed under [implicit value for role](https://www.w3.org/TR/wai-aria-1.1/#implictValueForRole). The list of required states and properties for a role can be found in [WAI-ARIA 1.1](https://www.w3.org/TR/wai-aria)"


The second sentence can be moved to a note, since it is not part of the expectation, but just something to help assess it.

carlosapaduarte · 2018-10-01T20:30:48Z

_rules/SC4-1-2-role-has-required-states-and-properties.md

+
+**Note:** This rule does not test whether the required states and properties has a correct value, only that the attributes are present and [non-empty](#non-empty).
+
+## Assumptions


This assumption is confusing to me for the following reasons:
1 - The applicability is for elements with an explicit semantic role. This assumption applies to elements with implicit semantic roles.
2 - The assumption's reason to exist seems to be to justify the applicability. If that is case, then why isn't it just a note on the applicability?
3 - It is hard to read. There are too many negatives (no element that has an implicit semantic role + does not define native attributes). Can't this be rephrased?

carlosapaduarte · 2018-10-01T20:34:15Z

_rules/SC4-1-2-role-has-required-states-and-properties.md

+
+## Accessibility Support
+
+This rule relies on browsers and assistive technologies to support leaving out [required states and properties](https://www.w3.org/TR/wai-aria-1.1/#requiredState) when an [implicit value for role](https://www.w3.org/TR/wai-aria-1.1/#implictValueForRole) is specified in [WAI-ARIA 1.1](https://www.w3.org/TR/wai-aria-1.1).


Once more, the applicability is for elements with explicit semantic roles, and this section mentions issues for elements with implicit semantic roles. I'm sorry, but I must be having problems with understanding this rule.

@carlosapaduarte, actually here it is talking about implicit value for roles which is different from the implicit roles. I can see how this is totally confusing. Not sure what to do about it, though since "implicit value for roles" is the term used in the WAI-ARIA spec...

Maybe prefixing or suffixing these terms with "WAI-ARIA" will help clarify the source of these terms (as well as the target of the link)?

carlosapaduarte · 2018-10-01T20:43:40Z

_rules/SC4-1-2-role-has-required-states-and-properties.md

+
+#### Passed example 5
+
+Element has required properties (no states required for this role)


There is a required state (aria-expanded) but it has a default value, so this is a pass anyway. But the comment needs to be updated.

carlosapaduarte · 2018-10-01T20:46:08Z

_rules/SC4-1-2-role-has-required-states-and-properties.md

+
+## Test Cases
+
+### Passed


Add a test case with a value for a state or a property that is non-empty but not correct. The rule should still pass since it does not test the correctness of this value.

annethyme · 2018-10-02T09:24:18Z

Rule changed per comments. Please review again.

nitedog · 2018-10-02T14:52:36Z

_rules/SC4-1-2-role-has-required-states-and-properties.md

+
+For each test target, the [required states and properties](https://www.w3.org/TR/wai-aria-1.1/#requiredState) for the role are present, unless the state or property has a default value listed under [implicit value for role](https://www.w3.org/TR/wai-aria-1.1/#implictValueForRole). 
+
+**Note:** The list of required states and properties for each role can be found in [WAI-ARIA 1.1](https://www.w3.org/TR/wai-aria-1.1).


Minor suggestion: change the link text in the above paragraph from "required states and properties" to "WAI-ARIA required states and properties" or to "required states and properties in WAI-ARIA" or to "required states and properties (WAI-ARIA)", so that you clarify the link target in the link text and can get rid of this note.

nitedog · 2018-10-02T14:54:15Z

_rules/SC4-1-2-role-has-required-states-and-properties.md

+
+**Note:** The list of required states and properties for each role can be found in [WAI-ARIA 1.1](https://www.w3.org/TR/wai-aria-1.1).
+
+**Note:** This rule does not test whether the required states and properties has a correct value, only that the attributes are present and [non-empty](#non-empty).


"whether the required states and properties has a correct value" -> "whether the required states and properties have correct values"

nitedog

Pending some minor editorial comments, for editor's discretion

carlosapaduarte

Great update. I'm happy with it (@nitedog points out some minor editorial changes that should be addressed)

move to rule from drafts.

40004fb

jeeyyy assigned annethyme Jun 24, 2018

ShadowBB reviewed Jun 26, 2018

View reviewed changes

Editorial

a1f076a

WilcoFiers previously requested changes Jun 27, 2018

View reviewed changes

WilcoFiers added the reviewer wanted label Jul 2, 2018

kasperisager self-requested a review July 3, 2018 07:38

kasperisager previously requested changes Jul 3, 2018

View reviewed changes

Brynanders and others added 8 commits July 10, 2018 11:47

Update SC4-1-2-role-has-required-states-and-properties.md

3825f78

fixed quotation mark in second failed test case.

Update SC4-1-2-role-has-required-states-and-properties.md

9726bb8

Removed Expectation 2 (created new rule to address this #176) Removed notes under Expectation 2 that relate to #176 Removed second assumption that relates to #176 Removed 3rd Background reference that relates to #176

Update SC4-1-2-role-has-required-states-and-properties.md

286cd18

Replaces ' ' with " " in passed test case

Update SC4-1-2-role-has-required-states-and-properties.md

8f1b66f

Removed second test case which relates to #176

Update SC4-1-2-role-has-required-states-and-properties.md

2895eba

Removed aria-controls value from Passed text case as the property value is now out of scope.

Update SC4-1-2-role-has-required-states-and-properties.md

9164205

Added second test case to cover the assumption that @annethyme will update.

Updating "Assumptions"

fe0d8e3

Updating "Assumptions" to handle comments from ShadowBB, WilcoFiers and kasperisager about support for implicit values.

Updated test cases + added note to Expectation

592f57d

Split out Passed test case in two, one for state and one for property, and edited note on second Failed test case. Added note for Expectation.

WilcoFiers previously requested changes Jul 16, 2018

View reviewed changes

jeeyyy changed the title ~~FINAL: Rule - SC4-1-2-role-has-required-states-and-properties~~ Rule: SC4-1-2-role-has-required-states-and-properties Jul 16, 2018

ShadowBB approved these changes Jul 18, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

Updating to handle feedback

a98b91e

kasperisager previously requested changes Oct 1, 2018

View reviewed changes

Updated according to feedback

04ec472

Anne Thyme added 2 commits October 1, 2018 16:24

Editorial updates

927c58c

Editorial update

92f0ab9

annethyme requested review from WilcoFiers, kasperisager and nitedog October 1, 2018 14:28

carlosapaduarte requested changes Oct 1, 2018

View reviewed changes

Anne Thyme added 2 commits October 2, 2018 11:16

Updated per comments

04f2deb

Updated per comments

433dbf9

annethyme requested review from carlosapaduarte and ShadowBB October 2, 2018 09:23

kasperisager approved these changes Oct 2, 2018

View reviewed changes

nitedog reviewed Oct 2, 2018

View reviewed changes

nitedog approved these changes Oct 2, 2018

View reviewed changes

carlosapaduarte approved these changes Oct 2, 2018

View reviewed changes

Editorial updates per feedback

ad9467a

annethyme merged commit 4e7a3db into master Oct 3, 2018

jeeyyy deleted the rule-SC4-1-2-role-has-required-states-and-properties branch January 3, 2019 12:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rule: SC4-1-2-role-has-required-states-and-properties #167

Rule: SC4-1-2-role-has-required-states-and-properties #167

jeeyyy commented Jun 24, 2018 •

edited by DagfinnRomen

Loading

ShadowBB left a comment •

edited

Loading

WilcoFiers left a comment

kasperisager left a comment •

edited

Loading

WilcoFiers commented Jul 3, 2018

kasperisager commented Jul 3, 2018

annethyme commented Jul 11, 2018

WilcoFiers left a comment

This comment has been minimized.

kasperisager Oct 1, 2018

carlosapaduarte left a comment

carlosapaduarte Oct 1, 2018

carlosapaduarte Oct 1, 2018

carlosapaduarte Oct 1, 2018

annethyme Oct 2, 2018

nitedog Oct 2, 2018

carlosapaduarte Oct 1, 2018

carlosapaduarte Oct 1, 2018

annethyme commented Oct 2, 2018

nitedog Oct 2, 2018

nitedog Oct 2, 2018

nitedog left a comment

carlosapaduarte left a comment


		### Expectation

		For each test target, the [required states and properties](https://www.w3.org/TR/wai-aria/#requiredState) for the role are present, unless the state or property has a default value listed under [implicit value for role](https://www.w3.org/TR/wai-aria-1.1/#implictValueForRole). The list of required states and properties for a role can be found in [WAI-ARIA 1.1](https://www.w3.org/TR/wai-aria)"


		Note: This rule does not test whether the required states and properties has a correct value, only that the attributes are present and [non-empty](#non-empty).

		## Assumptions


		## Accessibility Support

		This rule relies on browsers and assistive technologies to support leaving out [required states and properties](https://www.w3.org/TR/wai-aria-1.1/#requiredState) when an [implicit value for role](https://www.w3.org/TR/wai-aria-1.1/#implictValueForRole) is specified in [WAI-ARIA 1.1](https://www.w3.org/TR/wai-aria-1.1).


		#### Passed example 5

		Element has required properties (no states required for this role)


		For each test target, the [required states and properties](https://www.w3.org/TR/wai-aria-1.1/#requiredState) for the role are present, unless the state or property has a default value listed under [implicit value for role](https://www.w3.org/TR/wai-aria-1.1/#implictValueForRole).

		Note: The list of required states and properties for each role can be found in [WAI-ARIA 1.1](https://www.w3.org/TR/wai-aria-1.1).

Rule: SC4-1-2-role-has-required-states-and-properties #167

Rule: SC4-1-2-role-has-required-states-and-properties #167

Conversation

jeeyyy commented Jun 24, 2018 • edited by DagfinnRomen Loading

Checklist for Pull Request

How to Review And Approve

Description

ShadowBB left a comment • edited Loading

Choose a reason for hiding this comment

WilcoFiers left a comment

Choose a reason for hiding this comment

kasperisager left a comment • edited Loading

Choose a reason for hiding this comment

WilcoFiers commented Jul 3, 2018

kasperisager commented Jul 3, 2018

annethyme commented Jul 11, 2018

WilcoFiers left a comment

Choose a reason for hiding this comment

This comment has been minimized.

Choose a reason for hiding this comment

carlosapaduarte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annethyme commented Oct 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nitedog left a comment

Choose a reason for hiding this comment

carlosapaduarte left a comment

Choose a reason for hiding this comment

jeeyyy commented Jun 24, 2018 •

edited by DagfinnRomen

Loading

ShadowBB left a comment •

edited

Loading

kasperisager left a comment •

edited

Loading