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

feat: Add aria-structure-name rule #2431

Closed
wants to merge 1 commit into from

Conversation

eps1lon
Copy link
Contributor

@eps1lon eps1lon commented Jul 30, 2020

Adds two new rules that catch elements which require an accessible name but are unnamed. Two separate rules since not every naming technique applies to all elements. If we can give actionable advise based on the role without having to create multiple rules then I'd be in favor of that.

Closes issue: None. But it is part of #2421.

Reviewer checks

Required fields, to be filled out by PR reviewer(s)

  • Follows the commit message policy, appropriate for next version
  • Code is reviewed for security

@eps1lon eps1lon requested a review from a team as a code owner July 30, 2020 20:30
@CLAassistant
Copy link

CLAassistant commented Jul 30, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@straker straker left a comment

Choose a reason for hiding this comment

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

Thanks for putting together the pr. I like where this is going. I believe if we were to add a matcher to both of these rules then we could account for all the roles that require an accessible name.

What I'm thinking is we add a new property to the ARIA roles standards object that signifies the role requires an accessible name. We've been trying to keep the property names similar to the spec names as possible, so maybe something like accessibleNameRequired: true?

Then using that and the nameFromContent property, you should be able to match any [role] and determine which rule it belongs to. Something like:

// name-from-content-matches.js
import getExplicitRole from '../commons/aria/get-explicit-role';
import standards from '../commons/standards';

// these roles already have a rule that checks their name
const excludedRoles = [
  // aria-input-field-name
  'combobox',
  'listbox',
  'searchbox',
  'slider',
  'spinbutton',
  'textbox',

  // aria-toggle-field-name
  'menuitemcheckbox',
  'menuitemradio',
  'radio',
  'switch',

  // button-name
  'button',

  // empty-heading
  'heading',

  // link-name
  'link',

  // role-img-alt
  'img'
];

function nameFromContentMatches(node, virtualNode) {
  const role = getExplicitRole(virtualNode);

  if (excludedRoles.includes(role)) {
    return false;
  }

  const roleDesc = standards.ariaRoles[role];
  return roleDesc.accessibleNameRequired && roleDesc.nameFromContent;
}

export default nameFromContentMatches;
{
  "id": "name-author-contents",
  "selector": "[role]",
        "matches": "name-from-content-matches",
  "tags": [],
  "metadata": {
    "description": "Ensures elements with ARIA roles that require an accessible name have one from author or contents.",
    "help": "Required accessible name must be present (author or contents)"
  },
  "all": [],
  "any": [
    "aria-label",
    "aria-labelledby",
    "has-visible-text",
    "non-empty-title"
  ],
  "none": []
}

We'll want to specifically prevent roles that already have a name rule from going through this rule, so rather than using a selector with a ton of :not()s it'd be easier to use the matcher to prevent them.

Lastly, @WilcoFiers has the final say on rules, so it might be best to wait for his input before implementing my suggestions :)

@@ -0,0 +1,17 @@
{
"id": "name-author-contents",
Copy link
Contributor

Choose a reason for hiding this comment

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

The rule name is a bit vague. Ideally it would tell me what the rule is checking, like button-name or image-alt. name-author-contents does that, but I may not understand what name from author or contents means.

Maybe something like role-required-name? I'm not sure how to appropriately differentiate between having visible text or not.

"selector": "[role=\"treeitem\"]",
"tags": [],
"metadata": {
"description": "Ensures elements with ARIA roles that require an accessible name have one from author or contents.",
Copy link
Contributor

Choose a reason for hiding this comment

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

I would avoid talking about author or content here for the same reasons as stated above.

Copy link
Contributor

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

In addition to Steven's comments, we should break this PR down. Each new rule should have its own PR. We use commit messages to generate the changelog, by putting each new rule in a separate PR, we get a changelog that clearly includes which rules were added.

@eps1lon eps1lon changed the title feat: Catch more unnamed roles which require a name feat: Add aria-structure-name rile Aug 21, 2020
@eps1lon eps1lon changed the title feat: Add aria-structure-name rile feat: Add aria-structure-name rule Aug 21, 2020
@eps1lon eps1lon marked this pull request as draft August 21, 2020 17:42
@WilcoFiers
Copy link
Contributor

@eps1lon Would you be able to get this updated in the next few days? We're working towards a 4.1 release and would need to have this merged by November 2nd.

@eps1lon
Copy link
Contributor Author

eps1lon commented Oct 26, 2020

Can't make any promises with regards to a timeline. As far as I can tell this PR is outdated anyway since we went back and forth on the implementation.

@WilcoFiers
Copy link
Contributor

I have received some feedback about these ARIA name rules. See my update here: #2421 (comment). This particular rule will not be added. There are a few more rules in this batch that needs to be done. @eps1lon would you be up for taking on the aria-dialog-name rule?

@eps1lon
Copy link
Contributor Author

eps1lon commented Oct 28, 2020

would you be up for taking on the aria-dialog-name rule?

Will do 👍 I'm closing this PR since most of the conversation is obsolete anyway.

@eps1lon eps1lon closed this Oct 28, 2020
@eps1lon eps1lon deleted the feat/name-required branch October 28, 2020 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants