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

Changed succescriteria from 4.1.2 to 1.3.1 #449

Merged
merged 5 commits into from
Apr 5, 2019
Merged

Conversation

danistr
Copy link
Collaborator

@danistr danistr commented Mar 19, 2019

<< Describe the changes >>

Closes issue: (#448 )

Guidance for the PR (pull request) creator

When creating PR:

  • Make sure you requesting to pull a issue/feature/bugfix branch (right side) to the master branch (left side).

After creating PR:

  • Add yourself (and co-authors) as "Assignees" for PR
  • Add label to indicate if it's a Rule, Definition or Chore (more to the administrative side)
  • Add relevant project (e.g. "Q3 2018 Status") to PR
  • OPTIONAL: If you want anyone in particular to review your pull request, assign them as "Reviewers".
  • Close the issue that the PR resolves (and make sure the issue is referenced in the top of this comment)

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

ShadowBB
ShadowBB previously approved these changes Mar 19, 2019
Copy link
Collaborator

@annethyme annethyme left a comment

Choose a reason for hiding this comment

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

I tried to come up with an example, where it wouldn't fall under 1.3.1, but didn't succeed, so if no one else comes up with anything either, I approve this rule.

Copy link
Member

@carlosapaduarte carlosapaduarte left a comment

Choose a reason for hiding this comment

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

In the applicability's note there's a typo: change decendent to descendent

@annethyme
Copy link
Collaborator

@JKODU, I am assigning you to this one, since you are the person responsible for this rule within the WAI-Tools project.

@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 20, 2019
@annethyme annethyme added this to Needs Review in Rules Progress via automation Mar 20, 2019
@ShadowBB
Copy link
Collaborator

In the applicability's note there's a typo: change decendent to descendent

Done. Thanks!

@DagfinnRomen
Copy link
Collaborator

The background section should reference the 1.3.1 understanding article, not 4.1.2.
https://www.w3.org/WAI/WCAG21/Understanding/info-and-relationships.html

Also, I suggest a minor update to the description, from "This rule checks aria-hidden elements do not contain focusable elements" to "This rule checks that elements with an aria-hidden attribute do not contain focusable elements".

Reworded the description somewhat and removed a 4.1.2 references and replaced it with a 1.3.1 reference.
@ShadowBB
Copy link
Collaborator

The background section should reference the 1.3.1 understanding article, not 4.1.2.
https://www.w3.org/WAI/WCAG21/Understanding/info-and-relationships.html

Also, I suggest a minor update to the description, from "This rule checks aria-hidden elements do not contain focusable elements" to "This rule checks that elements with an aria-hidden attribute do not contain focusable elements".

Well spotted! Both done. Thanks!

@annethyme annethyme added Review call 2 weeks Call for review for new rules and big changes and removed Agenda item labels Mar 26, 2019
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.

I understand the argument for this being a failure of 4.1.2, but I don't quite understand the argument for why this wouldn't also be a failure for 4.1.2. Can someone explain this before we merge? I think at least it needs to be documented somewhere.

@ShadowBB
Copy link
Collaborator

I understand the argument for this being a failure of 4.1.2, but I don't quite understand the argument for why this wouldn't also be a failure for 4.1.2. Can someone explain this before we merge? I think at least it needs to be documented somewhere.

Sure. This rule checks that elements with an aria-hidden attribute do not contain focusable elements. Elements with an aria-hidden element that contain focusable elements can still have a name and role that can be programmatically determined which is all that is needed to pass 4.1.2 in some cases. So failing this rule is not a guarantee that 4.1.2 also fails.

@WilcoFiers
Copy link
Member

@ShadowBB Thanks for clarifying. I don't agree with you on this. Putting aria-hidden=true on an element removes it from the accessibility tree. Since the name / role / value props are derived from the accessibility tree, any element with aria-hidden=true no longer has a name, role, or any props / state. And since it is focusable (thus will be perceived by the user as an interface component), I would say that fails SC 4.1.2.

@ShadowBB
Copy link
Collaborator

@ShadowBB Thanks for clarifying. I don't agree with you on this. Putting aria-hidden=true on an element removes it from the accessibility tree. Since the name / role / value props are derived from the accessibility tree, any element with aria-hidden=true no longer has a name, role, or any props / state. And since it is focusable (thus will be perceived by the user as an interface component), I would say that fails SC 4.1.2.

I have thought about this further and deliberated with a colleague. We came to the conclusion that you are correct. It should indeed fail both SC. Our flaw was in the programmatically determined definition (we forgot that mentioned in needed to be visible to assistive technology too).

Sorry to waste all this time on this. We should probably mention both SC, also in the background.

@ShadowBB ShadowBB dismissed their stale review March 29, 2019 11:13

I now agree with Wilco that it should mention both SC.

Rules Progress automation moved this from Needs Review to CFC (Reviewer Approved) Apr 1, 2019
@@ -42,7 +43,7 @@ By adding `aria-hidden="true"` to an element, content authors ensure that assist

A focusable element with `aria-hidden="true"` is ignored as part of the reading order, but still part of the focus order, making it's state of visible or hidden unclear.

- https://www.w3.org/WAI/WCAG21/Understanding/name-role-value.html
Copy link
Collaborator

Choose a reason for hiding this comment

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

If both SCs are concerned, put the 4.1.2 link back in the background section :-)

@jeeyyy jeeyyy merged commit 8fc9fff into act-rules:master Apr 5, 2019
Rules Progress automation moved this from CFC (Reviewer Approved) to Done Apr 5, 2019
@annethyme
Copy link
Collaborator

@JKODU, this was actually out in a 2-week "Final Call" that doesn't end until April 9.

kasperisager added a commit that referenced this pull request Apr 10, 2019
* master:
  Rule update: "HTML page has a title" (#440)
  Rule update: "aria attribute allowed" and "aria required states and properties" (#436)
  Changed succescriteria from 4.1.2 to 1.3.1 (#449)
  Rename SC1-2-Video-description-track.md to SC1-2-video-description-track.md
  Glossary: add "Accessibility Support" to "Semantic role" term (#442)
  SC1-1-1-filename-is-valid-accessible-name (#263)
  Added assumption for SC3-1-2-lang-valid (#413)
  Update SC4-1-1-unique-id.md
  fix: update applicability
  fix: revert definitions
  fix: update glossary
  fix: applicability
kasperisager added a commit that referenced this pull request Apr 26, 2019
* develop: (76 commits)
  Update SC1-1-1-image-has-name.md (#468)
  chore: update test case names to be hash and not have outcome (#485)
  chore: update styles
  chore: fix styling and spacing on various resolutions
  chore: fix typo in rules
  chore: update dependencies
  chore: add unique id to all rules (#478)
  fix: move footer from nav
  fix: update cache version
  fix: bust dependency cache
  chore: WCAG ACT RULES CG Website Update (#437)
  chore: update site publish config
  Replace auto-wcag with act-r in readme
  Update _config.yml
  Rule update: "aria-hidden with focusable content" (#439)
  Rule update: "HTML page has a title" (#440)
  Rule update: "aria attribute allowed" and "aria required states and properties" (#436)
  Changed succescriteria from 4.1.2 to 1.3.1 (#449)
  Rename SC1-2-Video-description-track.md to SC1-2-video-description-track.md
  Glossary: add "Accessibility Support" to "Semantic role" term (#442)
  ...
@annethyme annethyme removed this from Published in Rules Progress Jul 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review call 2 weeks Call for review for new rules and big changes 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.

None yet

8 participants