-
Notifications
You must be signed in to change notification settings - Fork 70
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
"Heading has non-empty accessible name" [ffd0e9]: Update from TF survey #2077
base: develop
Are you sure you want to change the base?
Conversation
Per review of survey heading non-empty task force conclusions, edited the following: * Pass ex 3, have the acc name be consistent with the text * Set 1.3.1 as a secondary requirement * Removed sentence on accessible name spec not being clear on whitespace * Removed inapp example 1
There is no [semantic][semantic role] `heading` element. | ||
|
||
```html | ||
<div></div> | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing this example breaks our own "test case design" guidelines of Test every condition 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Jym77 We felt this was not a useful test case, so we're thinking of leaving things like this out. Is that OK with you? Happy to update the test ever condition bit then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@WilcoFiers No strong feeling about that particular example which is, indeed, quite useless 🙂
It's more on the boundaries of things on how we decide that tests are useful or not.
From a TDD point of view, we should indeed have test cases for every condition, and since we do use the test cases to validate tools, this also sounds like a good idea from an engineering point of view.
From an "illustrate the rule to human" point of view, this is indeed a fairly useless case…
=> this tends to hint that we may want to have some test cases for the computers and only show a meaningful subset to the humans.
@WilcoFiers @kengdoj please let me know how you'd like to proceed per @Jym77 's comments. |
Co-authored-by: Jean-Yves Moyen <jym@siteimprove.com>
@WilcoFiers I committed @Jym77 feedback, please let me know if there is anything else needed to push this to close / done. Thanks for all your help @Jym77 and @WilcoFiers ! |
@@ -10,6 +10,8 @@ accessibility_requirements: | |||
failed: not satisfied | |||
passed: further testing needed | |||
inapplicable: further testing needed | |||
secondary_requirements: | |||
wcag20:1.3.1: # Info and Relationships (A) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not the correct syntax. What you should do is replace:
wcag20:1.3.1: # Info and Relationships (A)
forConformance: true
failed: not satisfied
passed: further testing needed
inapplicable: further testing needed
with this:
wcag20:1.3.1: # Info and Relationships (A)
secondary: true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@WilcoFiers am I to update within this PR or do I need to create a new one with the desired changes you recommend per secondary requirements? Not sure if you wanted another PR on 7fb7a37 for the highlight you are mentioning in this comment here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No please update in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@WilcoFiers when I view the file in "edit file" it shows your changes. I can't approve per admin settings, thus not sure what else to do to "close" this issue and "update the PR" from my side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@WilcoFiers I don't have access to approve the changes only to comment. I'll pursue updating the file and submitting the updated PR.
@WilcoFiers , I see this is still open and aged quite a bit. Did you want me to take any action on this or would you like to close as no longer applicable? |
Per review of survey heading non-empty task force conclusions, edited the following: