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

New Rule: Create Semantic Focus (a20046) #559

Closed
wants to merge 8 commits into from

Conversation

ramitgarg
Copy link

<< Describe the changes >>

Closes issue: (ADD ISSUE NUMBER HERE)

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

@ramitgarg ramitgarg changed the title Create Scemantic Focus New Rule: Create Scemantic Focus May 16, 2019
Scemantic Focus Outdated

## Applicability

The rule applies to all element in the sequential focus navigation excepts scrollable content element.
Copy link
Member

Choose a reason for hiding this comment

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

  • "all elements" is pretty vague. I propose "all HTML and SVG elements"
  • "sequential focus navigation" must be defined
  • "scrollable content element" must be defined

Copy link
Collaborator

Choose a reason for hiding this comment

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

[sequential focus navigation](https://www.w3.org/TR/html/editing.html#sec-sequential-focus-navigation

Scemantic Focus Outdated Show resolved Hide resolved
Scemantic Focus Outdated

## Assumptions

- Giving focus to scrollable content elements is the only way to scroll non interactive content using a keyboard.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think I understand why this is an assumption. Can you clarify?

Copy link
Member

Choose a reason for hiding this comment

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

It would have been better to have this as note in the applicability instead of an assumption. But after reviewing the definition of focusable are I don't think this is needed, given that if a scrollable region of an element is focusable, then it should have a semantic role so that users of AT can understand what role that region plays.

Scemantic Focus Outdated Show resolved Hide resolved
Scemantic Focus Outdated
Element in sequential navigation doesnot have semantic role

```html
<a href="#desc" tabindex="0" aria-hidden="true">More</a>
Copy link
Member

Choose a reason for hiding this comment

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

This scenario is addressed by a different role: https://act-rules.github.io/rules/6cfa84 I suggest that we avoid overlap by adding "that is included in the accessibility tree" to the applicability.

Scemantic Focus Outdated
#### Passed Example 4


### Failed
Copy link
Member

Choose a reason for hiding this comment

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

Add a test where you override the role of an element that's focusable by default.

Scemantic Focus Outdated
<div href="#desc" role="presentation" tabindex="0">More</div>
```

### Inapplicable
Copy link
Member

Choose a reason for hiding this comment

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

Add a test where you disable an element to remove it from focus sequence.

Scemantic Focus Outdated Show resolved Hide resolved
Scemantic Focus Outdated Show resolved Hide resolved
Scemantic Focus Outdated
This rule checks that every element part of the sequential focus navigation has a semantic role.

accessibility_requirements:
wcag20:2.4.4: # Link Purpose (In Context)
Copy link
Member

Choose a reason for hiding this comment

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

I don't believe this is a failure of 2.4.4. More generally, while I'm totally on board with this rule, I don't believe this is a failure of WCAG 2.1 at all. I've had that discussion in the past, this rule looks quite a lot like something we have in axe-core today. Here's a link to that conversation:
dequelabs/axe-core#632

Copy link
Collaborator

@EmmaJP EmmaJP Jul 11, 2019

Choose a reason for hiding this comment

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

Anne suggested 4.1.2

Copy link
Collaborator

Choose a reason for hiding this comment

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

Anne suggested 4.1.2

This would also not fail 4.1.2

4.1.2 asks a role of all "user interface components". But if a piece of text with a span on it suddenly receives focus (because of tabindex for example) that doesn't make that piece of text a user interface component because it doesn't control anything.

I agree that it is a good rule (because most of the time it will point to a 4.1.2 violation) but it is currently not necessarily a WCAG failure to fail this rule.

@jeeyyy jeeyyy changed the title New Rule: Create Scemantic Focus New Rule: Create Semantic Focus May 29, 2019
Scemantic Focus Outdated Show resolved Hide resolved
Scemantic Focus Outdated Show resolved Hide resolved
Scemantic Focus Outdated Show resolved Hide resolved
@annethyme
Copy link
Collaborator

Siteimprove doesn't think this rule is testing for anything required by WCAG.

@annethyme annethyme added the Should we proceed? Do people agree that this rule tests for WCAG and that the general concept is good? label Jun 13, 2019
@ShadowBB
Copy link
Collaborator

Although I think this is an interesting rule that can definitely indicate accessibility failures, it probably doesn't explicitly test anything in WCAG.

@WilcoFiers , I think this also depends on the definition of "user interface components" because if user interface components include things that receive focus, those things will need a role or fail 4.1.2.

Scemantic Focus Outdated Show resolved Hide resolved
@carlosapaduarte carlosapaduarte self-assigned this Jul 11, 2019
@annethyme annethyme added the Rule Use this label for a new rule that does not exist already label Jul 12, 2019
Revised the rule addressing previous comments. As an outcome of the revision, I do believe that now this addressed 4.1.2. More comments welcomed
semantic-focus-a20046.md Outdated Show resolved Hide resolved
EmmaJP
EmmaJP previously approved these changes Jul 26, 2019
Copy link
Collaborator

@EmmaJP EmmaJP left a comment

Choose a reason for hiding this comment

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

If everyone else is agreed that this rule would be part of checks for conformance for 4.1.2, which I'm OK with, then I think this is looking good. I've suggested a grammatical change in the description, but the rest seems to be fine.

This rule checks that every element that is part of the sequential focus navigation order has a semantic role.

accessibility_requirements:
wcag20:4.1.2: # Name, Role, Value (A)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would (still) not say this fails WCAG 4.1.2.

A Div with tabindex is not a user interface component and doesn't need a role.

@CLAassistant
Copy link

CLAassistant commented Aug 13, 2019

CLA assistant check
All committers have signed the CLA.

@annethyme annethyme changed the title New Rule: Create Semantic Focus New Rule: Create Semantic Focus (a20046) Aug 14, 2019
@jeeyyy
Copy link
Collaborator

jeeyyy commented Sep 11, 2019

@ramitgarg any updates on these?

@carlosapaduarte
Copy link
Member

@JKODU we decided last week to drop this one since it does not address any SC. do you want us to keep working on it?

@jeeyyy
Copy link
Collaborator

jeeyyy commented Oct 2, 2019

This seems to have "Should we proceed" for a while? Have we reached a conclusion on the same?

@carlosapaduarte
Copy link
Member

carlosapaduarte commented Oct 2, 2019

As far as I recall, the last time this was discussed in a meeting, the conclusion was to leave it on hold because this is not testing a SC, but it's just a best practice.

@carlosapaduarte carlosapaduarte added On hold and removed Should we proceed? Do people agree that this rule tests for WCAG and that the general concept is good? reviewers wanted Agenda item labels Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
On hold 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.

8 participants