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(standards): add html elems object #2325

Closed
wants to merge 15 commits into from
Closed

Conversation

straker
Copy link
Contributor

@straker straker commented Jun 25, 2020

Note: the object is incomplete and this pr is here to start discussing the object and structure. Since the object is so large multiple smaller prs that can be easier to verify the information is correct would be better.

The lookup table has a few different objects that try to convey HTML spec related things. elementsAllowedAnyRole, elementsAllowedNoRole, and evaluateRoleForElement. evaluateRoleForElement is a bunch of functions that can't even be serialized. Also, there are a few commons functions that need HTML spec information such as text.nativeElementTypes and subtree-text.js needing to know phrasing content type elements.

The aria roles object has a property allowedElements which is an inverse property lookup to the spec table found in https://www.w3.org/TR/html-aria/#docconformance. This means that to generate the list of allowed elements, you need to find each element the role is allowed on rather than looking at the element row and seeing which roles are allowed. This makes it extremely easy to miss things, for example:

This object puts all that information into a serializable standards object that can be configured as well. It removes the need for evaluateRoleForElement and makes that information configurable.

The object is defined as such and the information was taken from https://www.w3.org/TR/html-aria/#docconformance:

type Spec = {
  matches?: matcherObj, // if the spec should only apply if it matches the element
  allowedRole: boolean, // if the element is allowed a role
  allowedAttrs: boolean, // if the element is allowed global aria attrs and role attrs
  roles?: string[], // if only specific roles are allowed. if missing implies any role is allowed when allowedRole:true
  implicitAttrs:?: string[], // implicitly defined attrs per https://www.w3.org/TR/html-aam-1.0/#html-element-role-mappings
  namingMethods?: string | string[] // native-element-type.js naming methods
}

htlmEmls = {
  nodeName:string : Spec | {
    features: Spec[] 
  }
}

It also simplifies and enhances the check for aria-allowed-attr and aria-allowed-role. For example, the is-aria-role-allowed-on-element function has to perform 3 different matches functions looking at the elementsAllowedAnyRole, elementsAllowedNoRole, allowedElements and evaluateRoleForElement. Instead, it can just look at the HTML spec table and see which roles are allowed on the element:

isAriaRoleAllowedOnElement(vNode, role) {
  const nodeName = vNode.props.nodeName; 
  const implicitRole = getImplicitRole(vNode);

  let spec = standards.htlmElms[nodeName]; 
  if (spec.features) {
    spec = matches(vNode, spec.features);
  }

  // always allow the explicit role to match the implicit role
  if (role === implicitRole) {
    return true;
  }

  // element is not allowed a role
  if (!spec.allowedRole) {
    return false;
  }

  // element is allowed only certain roles
  if (spec.roles) {
    return spec.roles.includes(role);
  }

  // element is allowed any role
  return true;
}

The table also allows us to catch elements which aren't allowed global aria elements in aria-allowed-attr. For example, <col aria-label="foo"></col> should not pass as col is not allowed any aria attributes.

ariaAllowedAttrEvaluate(node) {
  let spec = standards.htlmElms[nodeName]; 
  if (spec.features) {
    spec = matches(vNode, spec.features);
  }

  if (!spec.allowedAttrs) {
    return false;
  }

  // ...
}

Lastly, the object defines implicitAttrs which allow us to remove the "hacks" in ariaRequiredAttrEvaluate by defining which HTML elements define default aria attributes. This means <h1 role="heading">foo</h1> should pass since the h1 element defaults aria-level to the level of the element.

We'll need to talk about how to handle the content types of each element. Having them in place would greatly help for #601 as knowing which elements are interactive content types would let us know to flag them. However, they get a bit messy in their requirements of when it's interactive or not and duplicates a some of the properties when that's the only difference between them. For example, audio is an interactive element only when it has a controls attribute, but the other properties of allowedRole and allowedAttrs is the same regardless.

Personally, I think it should be in it's own object so we don't have to conflict with this object. For example: img is an interactive content model if the usemap attribute is present, but should be part of the 3 other features of allowed roles or attrs. Doing this would require 6 different objects to ensure the combinations are intact, which is a bit much.

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

@straker straker requested a review from a team as a code owner June 25, 2020 06:48
lib/standards/html-elms.js Outdated Show resolved Hide resolved
lib/standards/html-elms.js Outdated Show resolved Hide resolved
Base automatically changed from standards-aria-attr to develop June 25, 2020 19:43
// Source: https://www.w3.org/TR/html-aam-1.0/#html-element-role-mappings
const htmlElms = {
a: {
features: [
Copy link
Contributor

Choose a reason for hiding this comment

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

Thought about this for a while... I think this is the right idea, but we can improve on this a little:

const htmlElms = {
  a: {
    variant: {
      'hyperlink': {
        matches: { 
          attributes: { href: '/.*/' }
        },
        implicitRole: 'link',
        allowedRoles: [...linkRoles]
      },
      'no href': {
        matches: {
          attributes: { href: null }
        },
        implicitRole: null,
        allowedRoles: true
      }
    }
  }
}

This has a bunch of advantages:

  • Using an array is problematic in configuration. With an array, there is no way to modify a single property inside one of the variants. You can't even update one, since our axe.configure replaces Arrays with whatever we pass in, instead of assigning props and leaving the existing stuff in tact.
  • Adding keys also makes it easier to understand what these things are doing at a glance. That's good for us when we work on this, it also makes configuration files easier to understand. I'm using language from html-aam here as a callback, should make these easier to look up.
  • I don't think "feature" describes what this is. It's a "type", but because that's an overloaded word, maybe "variant"?
  • I thought about instead of calling this "feature" just to use "allowedRoles" and "value", but that makes this noisy when adding implicitRole in, which we'll need to do

We should probably put text.nativeElementType into this structure in the future too.

lib/standards/html-elms.js Outdated Show resolved Hide resolved
lib/standards/html-elms.js Outdated Show resolved Hide resolved
lib/standards/html-elms.js Outdated Show resolved Hide resolved
lib/standards/html-elms.js Outdated Show resolved Hide resolved
@straker
Copy link
Contributor Author

straker commented Jun 29, 2020

This pr got messed up somewhere in the merging and branching. Going to open another clean one.

@straker straker closed this Jun 29, 2020
@straker straker mentioned this pull request Jun 29, 2020
2 tasks
@WilcoFiers WilcoFiers deleted the standards-html-elms branch January 30, 2023 16:08
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.

2 participants