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

aria-required-children "unallowed" message not actionable #3842

Closed
1 task done
dbjorge opened this issue Jan 3, 2023 · 8 comments · Fixed by #3951
Closed
1 task done

aria-required-children "unallowed" message not actionable #3842

dbjorge opened this issue Jan 3, 2023 · 8 comments · Fixed by #3951
Labels
pr A pr has been created for the issue rule metadata Issues in the rule metadata code (lib/rules)
Milestone

Comments

@dbjorge
Copy link
Contributor

dbjorge commented Jan 3, 2023

Product

axe-core

Product Version

4.6.1

Latest Version

  • I have tested the issue with the latest version of the product

Issue Description

The message for the unallowed type of aria-required-children failure introduced in axe-core 4.5 via #3597 is a little confusing for folks that aren't already familiar with ARIA ownership rules because the message doesn't summarize why the given children would be disallowed. We think this would be improved by updating the message to be more clear that the reason the children are disallowed is that they have roles which are disallowed, and what those roles are.

We understand that not including the roles in the message was an intentional choice to avoid the complexity of cases with children that don't have roles, but we'd like to request revisiting that choice - we think the amount of complexity this would add to the message calculation would be a worthwhile tradeoff to avoid user confusion.

Expectation

The message should contain enough information for a user unfamiliar with the rule to understand why it's being triggered in a given specific case. For example:

Element with role "menu" cannot have children with roles img,link

or:

Element with role "list" cannot have children visible to screen readers with no role

Actual

The message currently reads:

Element has children which are not allowed (see related nodes)

...without explaining what causes the children to be not allowed.

How to Reproduce

Codepen containing examples of each of the 6 cases suggested below, along with an embedded working example of the suggested output below

Additional context

Suggested new message templates to replace the rule's current "unknown" template:

"unallowedRoleSingular": "Element with role \"${data.role}\" cannot have child with role ${data.unallowedRoles}",
"unallowedRolePlural": "Element with role \"${data.role}\" cannot have children with roles ${data.unallowedRoles}",
"unallowedNoRoleSingular": "Element with role \"${data.role}\" cannot have child visible to screen readers with no role",
"unallowedNoRolePlural": "Element with role \"${data.role}\" cannot have children visible to screen readers with no role",
"unallowedMixedRoleSingular": "Element with role \"${data.role}\" cannot have children visible to screen readers with no role or with role ${data.unallowedRoles}",
"unallowedMixedRolePlural": "Element with role \"${data.role}\" cannot have children visible to screen readers with no role or with roles ${data.unallowedRoles}"

...and suggested aria-required-children-evaluate.js snippet (to replace current L87-89) to populate the necessary data/messageKey:

    const unallowedRoles = unallowed.map(({ role }) => role).filter(role => role !== null);
    const unallowedNullRoleCount = unallowed.length - unallowedRoles.length;
    
    let messageKey;
    if(unallowedRoles.length === 0) {
      if (unallowedNullRoleCount === 1) {
        messageKey = 'unallowedNoRoleSingular';
      } else {
        messageKey = 'unallowedNoRolePlural';
      }
    } else if (unallowedRoles.length === 1) {
      if (unallowedNullRoleCount === 0) {
        messageKey = 'unallowedRoleSingular';
      } else {
        messageKey = 'unallowedMixedRoleSingular';
      }
    } else { // unallowedRoles.length > 1
      if (unallowedNullRoleCount === 0) {
        messageKey = 'unallowedRolePlural';
      } else {
        messageKey = 'unallowedMixedRolePlural';
      }
    }

    this.data({
      messageKey,
      role,
      unallowedRoles,
    });
No-role terminology

The terminology "visible to screen readers" was the closest analogue I found in existing messages (aria-errormessage, aria-labelledby, button-has-visible-text, has-visible-text) for the case that would trigger an unknown failure for a no-role child here ("has a global aria property OR is focusable"). I think this is easier to understand than the current text, but it might be even more actionable to be more specific and literally tell the user that the issue is that there's a child "with global ARIA property {data.propertyName}" or "which is focusable".

I chose not to include that in this suggestion since it exploded the number of messages even further; I think if we wanted to go down that route, it might be better to separate this logical check out of aria-required-children into a separate rule which creates a separate failure per bad parent-child relationship (maybe `aria-allowed-child' or something).

@dbjorge dbjorge added the ungroomed Ticket needs a maintainer to prioritize and label label Jan 3, 2023
@WilcoFiers WilcoFiers added rule metadata Issues in the rule metadata code (lib/rules) and removed ungroomed Ticket needs a maintainer to prioritize and label labels Jan 4, 2023
@straker
Copy link
Contributor

straker commented Jan 9, 2023

Thanks for the suggestion. Rather than multiple messages, we could do what we do for definition-list when there are elements or roles that are not allowed as children. The only-dlitems check uses a single message and uses the invalid-children-evaluate method. The method generates a simple list of selectors that are invalid which covers both invalid elements and roles. That way we aren't pointing them to related nodes and instead list exactly what's wrong.

So taking the following HTML, it produces the following violation:

<dl>
  <li>hello</li>
  <li>baz</li>
  <div role="button">bar</div>
</dl>
Fix all of the following:
  dl element has direct children that are not allowed: li, [role=button]

@dbjorge
Copy link
Contributor Author

dbjorge commented Jan 9, 2023

@straker Yes, I think something along those lines would work. I think it would be good for the message to include some clarification of why the children are not allowed though (because they have an unallowed ARIA role). Maybe something like this, where data.parentSelector is derived similarly to invalid-children-evaluate's values?

${data.parentSelector} element has children with ARIA roles that are not allowed: ${data.values}

Or possibly with this extra clause, depending if #3850 is accepted:

${data.parentSelector} element has children visible to screen readers with ARIA roles that are not allowed: ${data.values}

@dbjorge
Copy link
Contributor Author

dbjorge commented Jan 17, 2023

We've started receiving feedback about this message being confusing from our users, so we're likely to include a local patch implementing the message update here as part of our update to axe-core 4.6 in Accessibility Insights. We'd like to have that patch use wording similar to what axe-core 4.7 is likely to end up with if possible.

@straker, do either of the specific proposals from my previous comment look good to you? Or do you have a different specific wording you'd prefer to use instead?

@straker
Copy link
Contributor

straker commented Jan 17, 2023

The parentSelector is already available from the node itself and the target property, so I don't think it's needed in the message. The problem with calling out that ARIA roles are not allowed is that unless you know that an li element has an implicit listitem role, seeing the message element has children with ARIA roles that are not allowed: li would be confusing as well since the li does not have a role attribute.

Do we need to say exactly why the child is not allowed?

@dbjorge
Copy link
Contributor Author

dbjorge commented Jan 17, 2023

I included parentSelector because, since the rule only applies to parents with explicit roles, in practice it will be of the form role=specific_aria_role, which I think would be clear about identifying the specific part of the parent selector that is related to the problem at hand. I think something like element with role ${data.parentRole} would also be equally fine. If the message needs to be shortened for brevity, I think dropping the parent role would be better than dropping child roles.

The feedback we're receiving from users is that they're confused about what they need to change to resolve the issue. I think it it would help to say exactly why the child is not allowed. I agree with you that saying li would probably still be confusing because of the implicit role. Specific points of feedback we're seeing are:

  • "I don't know which children are unallowed" (from users that don't understand that they need to look inside the individual check to find the "related nodes" the current message references)
  • "I don't know why the child would be unallowed" (from users that can identify the problematic children but either don't understand that ARIA roles are the issue or don't understand how to map children to their implicit roles)
  • "I don't know how to identify what roles would be allowed" (from users that understand that the ARIA roles are the issue but don't understand that they need to look up the ARIA spec for the parent role and look at the "required owned" characteristic)

I think something like this would be ideal from the standpoint of "including enough information to be self-actionable for all of the above cases", but I appreciate that it's a bit verbose compared to most other rules:

Children of element with ARIA role ${data.parentRole} must have an ARIA role in ${data.allowedChildRoles}, but found children: ${data.unallowedChildren}

...where ${data.unallowedChildren} elements are of form role=listitem for children with explicit roles, li (implicit role listitem) for children with implicit roles, and div (no role) for children with no implicit or explicit role.

Does that sound reasonable? Can you think of a good way to include comparable information that's less verbose? Probably would be better to come up with an option that doesn't require translating individual elements of a data property, that's probably rough.

@dbjorge
Copy link
Contributor Author

dbjorge commented Jan 18, 2023

I wonder if it would be better to split out this specific check into a different check/rule that produces violations per-parent-child-pair instead of per-parent to help make it less verbose and easier to template the translatable parts of the message. Maybe something like aria-unallowed-child with message templates of forms:

  • Child with role: Element has ARIA role ${data.explicitRole}, but its parent's role (${data.parentRole}) requires that children visible to screen readers have one of the following roles: ${data.allowedChildRoles}
  • Child with no role: Element has no ARIA role, but its parent's role (${data.parentRole}) requires that children visible to screen readers have one of the following roles: ${data.allowedChildRoles}

@straker
Copy link
Contributor

straker commented Jan 19, 2023

I think the approach we should take is that the message should be informative enough that if you know what the problem is you can fix it based on the message, but if you don't understand the why then you'll need to refer to the help page to get that information.

With that in mind, I think we should just list out the selectors that are invalid and then update the the rule help page with more information if it doesn't have enough to help with those specific points of feedback.

dbjorge added a commit to microsoft/accessibility-insights-web that referenced this issue Jan 30, 2023
…ock and meta-viewport (#6334)

#### Details

This PR updates `axe-core` to its latest version, 4.6.2, from 4.4.1.

Notable code changes beyond the simple version bump include:

* `link-in-text-block` failures have been promoted from "needs review"
to "automated checks" (removed from needs review with explicit code
change, added to automated checks implicitly due to new axe-core
tagging)
* `meta-viewport` failures have been promoted from "unreported" to
"automated checks" (implicitly due to new axe-core tagging)
* WCAG mappings for a few rules have changed (we pull these
automatically from axe-core's tags, so there is no corresponding code
change)
* Per discussion with PM, `link-in-text-block` incomplete results are
staying unreported (not being promoted to "needs review")
* `frame-title-unique` and `no-autoplay-audio` are explicitly disabled.
They were promoted in axe-core from experimental/best-practice to
"wcag-correspondant, but needs-review-only", but we want to omit them
from our needs review checks pending further investigation

##### Motivation

See 1960423

##### Context

* We are considering a separate PR to pull in a temporary version of the
`aria-required-children` message improvements we've suggested upstream
as [axe-core#3842](dequelabs/axe-core#3842)
(which would hopefully be removed later once we update to axe-core 4.7).
Leaving that for a separate PR.
* While testing this locally, I noticed an issue where the target page
console would show an error during scans suggesting axe-core scans were
being run multiple times concurrently. I also observed this is `main`
without this PR - I don't think it's a regression and will be following
up separately.

#### Pull request checklist
<!-- If a checklist item is not applicable to this change, write "n/a"
in the checkbox -->
- [x] Addresses an existing issue: 1960423
- [x] Ran `yarn fastpass`
- [x] Added/updated relevant unit test(s) (and ran `yarn test`)
- [x] Verified code coverage for the changes made. Check coverage report
at: `<rootDir>/test-results/unit/coverage`
- [x] PR title *AND* final merge commit title both start with a semantic
tag (`fix:`, `chore:`, `feat(feature-name):`, `refactor:`). See
`CONTRIBUTING.md`.
- [n/a] (UI changes only) Added screenshots/GIFs to description above
- [n/a] (UI changes only) Verified usability with NVDA/JAWS

---------

Co-authored-by: JGibson2019 <jacqueline.gibson@microsoft.com>
Co-authored-by: SB <shanisebarona@gmail.com>
dbjorge added a commit to microsoft/accessibility-insights-web that referenced this issue Feb 1, 2023
…ard row (#6388)

#### Details

This PR implements a feature to add a new "Related paths" row to result
cards that represent axe-core results which included any check results
with `relatedNodes` properties.

It adds this information to:
* Cards in the fast pass automated checks and needs review results
* FastPass exported reports
* "axe results" style reports in the report package
* "combined" style reports in the report package (what we usually
consider as "service reports") - the service already sends enough
information for us to populate this

Screenshot of FastPass Automated Checks showing an
`aria-required-children` violation with Related paths:
![screenshot of FastPass card with "Related paths" card row added in
between "Snippet" and "How to fix" rows, showing a list of targets
formatted similarly to the old "Path"
row](https://user-images.githubusercontent.com/376284/215625581-3fe63469-bc8b-49ee-8a08-3bf1c4e67542.png)

Screenshot of FastPass Report Export showing the same info:
![screenshot of FastPass report export with "Related paths" card row
added in between "Snippet" and "How to fix" rows, showing a list of
targets formatted similarly to the old "Path"
row](https://user-images.githubusercontent.com/376284/215625717-b7632986-e2d4-4328-80fd-7e419dc54674.png)

Screenshot of FastPass Automated Checks' `color-contrast` results on a
page where `axe-core` only reports related paths for some results - note
that cards for results where the data is present show it, and results
where axe-core didn't report that data omit it.
![screenshot of two FastPass cards, one with Related paths and one
without](https://user-images.githubusercontent.com/376284/215625877-2225d666-c98f-4d5b-a62a-5652daa7cf33.png)

Screenshot of combined report with related path field:
![screenshot of combined report card with a related
path](https://user-images.githubusercontent.com/376284/215629181-7a6866f7-d124-44eb-afbe-a327217db50d.png)

##### Motivation

This is primarily to improve the actionability of the
`aria-required-children` "unknown" violation path which we added support
for during our 4.6.3 update in #6334. That path uses a how to fix
message of the form `"Element has children which are not allowed (see
related nodes)"`, which isn't very actionable without this PR since we
don't expose "related nodes" anywhere. This specific message might be
updated in a future version of axe-core (see
dequelabs/axe-core#3842)

This secondarily improves the actionability of a few other rules that
axe-core exposes related nodes for - particularly, `color-contrast`
violations where axe knows the node that is responsible for the
background color of a violation will now present that background node as
a "related path".

##### Context

In addition to this change, we'll want to make a docs change to
https://accessibilityinsights.io/info-examples/web/aria-required-children/
to make this specific form of `aria-required-children` violation easier
for a user to understand and take action on.

#### Pull request checklist
<!-- If a checklist item is not applicable to this change, write "n/a"
in the checkbox -->
- [x] Addresses an existing issue: 2020748
- [x] Ran `yarn fastpass`
- [x] Added/updated relevant unit test(s) (and ran `yarn test`)
- [x] Verified code coverage for the changes made. Check coverage report
at: `<rootDir>/test-results/unit/coverage`
- [x] PR title *AND* final merge commit title both start with a semantic
tag (`fix:`, `chore:`, `feat(feature-name):`, `refactor:`). See
`CONTRIBUTING.md`.
- [x] (UI changes only) Added screenshots/GIFs to description above
- [x] (UI changes only) Verified usability with NVDA/JAWS
@straker straker added the pr A pr has been created for the issue label Mar 21, 2023
@straker straker added this to the Axe-core 4.7 milestone Mar 21, 2023
@padmavemulapati
Copy link

Validated with the latest axe-core develop branch code base,
for aria-required-children unallowed fix message, the message is changed from passing related nodes to data.values so that on which value, chidren not allowing can be identified and can be fixed easily when it fails.

for ex: for the below code snippet:

<dl>
  <li>hello</li>
  <li>baz</li>
  <div role="button">bar</div>
</dl>

definition-list fails and communicating the fix message as
dl element has direct children that are not allowed: li, [role=button]
Earlier it was
List element has direct children that are not allowed inside <dt> or <dd> elements

Now:

Image

Older:

Image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr A pr has been created for the issue rule metadata Issues in the rule metadata code (lib/rules)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants