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

Custom Elements using ElementInternals to set role are flagged when aria-label is provided #4259

Open
1 task done
DRiFTy17 opened this issue Nov 28, 2023 · 13 comments
Open
1 task done
Labels
feat New feature or enhancement info needed More information or research is needed to continue standards Issues in the ARIA standards objects (lib/standards)
Milestone

Comments

@DRiFTy17
Copy link

Product

axe-core

Product Version

4.8.2

Latest Version

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

Issue Description

We are currently migrating our Web Component library to use the ElementInternals API to set default ARIA attributes, such as role, on all of our semantic host elements instead of sprouting these role attributes when the element is connected. This allows for setting default semantics, while still allowing for consumers to override these semantics if necessary. Upon doing so, axe is flagging some of these custom elements as critical issues that appear to be false positives.

It's worth noting that axe will not flag these elements if any non-empty text nodes appear in the shadow DOM of the element, or if the role attribute is set directly on the element.

Expectation

Custom Elements that use ElementInternals to set their default role should not be flagged by axe-core.

Actual

The extension produced the following critical error:

aria-label attribute cannot be used on a custom-progress-internals with no valid role attribute.

How to Reproduce

The following codepen is a reproduction in the simplest form that shows a custom element with a default role of progressbar set via ElementInternals, along with an aria-label set in the light DOM on the host:
https://codepen.io/drifty17/full/WNPgvoM

Open the axe extension in DevTools and scan the page to see the error:
image

Upon viewing the accessibility tree you can also see that the semantics appear to be set properly (tested with VoiceOver as well and it interpreted it correctly):
image

Additional context

I added an additional (working) example of a separate custom element for comparison that is identical except that it will sprout the role attribute when it connects to the DOM. When the role attribute is present on the element, axe does not flag the element, but when the same role is set via ElementInternals instead it does get flagged.

@DRiFTy17 DRiFTy17 added the ungroomed Ticket needs a maintainer to prioritize and label label Nov 28, 2023
@straker
Copy link
Contributor

straker commented Nov 29, 2023

Thanks for the issue. We'll need to do some extensive research to see how much assistive technologies support different element internal properties. If they support it, adding support to axe-core will take a lot of time as we have exclusively been looking at just the available element attributes to get ARIA information. Supporting this will require an extensive change in how we access ARIA information throughout the entire codebase.

@straker straker added feat New feature or enhancement info needed More information or research is needed to continue standards Issues in the ARIA standards objects (lib/standards) and removed ungroomed Ticket needs a maintainer to prioritize and label labels Nov 29, 2023
@straker straker added this to the Axe-core 4.9 milestone Nov 29, 2023
bennypowers added a commit to patternfly/patternfly-elements that referenced this issue Mar 19, 2024
bennypowers added a commit to patternfly/patternfly-elements that referenced this issue Mar 20, 2024
* docs(button): fix demo

* fix(button)!: remove shadow <button>

Closes #2706

* fix(chip): use the new pf-button

* test(button): form submit

* fix(button): click event on space/enter

* test(chip): refactor tests

* test: update tests

* test(back-to-top): fix a11y assertion

* test(dropdown): skip aria attrs assertion

see dequelabs/axe-core#4259
@Westbrook
Copy link

Would love to see this added the library!!

@Westbrook
Copy link

Ooof, the move to axe-core being part of Lighthouse 12 makes not supporting this even more problematic. A red test (because the tool doesn't support the real world) doesn't get looked at when it gets more red, which opens to door for content to get less accessible under the guise of "but, I was using axe-core".

How could the community support bringing this feature to the project?

@straker
Copy link
Contributor

straker commented Jul 11, 2024

@Westbrook The biggest support we could get from the community is to determine how well ElementInternals is supported in various assistive technologies and browsers. We've seen before that some ATs and browsers haven't implemented the feature very well which would cause problems for users. How well it's supported would have to be a very detailed listing of different combinations of roles, uses, parent / child relationships, and common types of components (like comboboxes, forms, etc.), as well as shadow DOM support. Once we've determined the amount of support we can determine what the next step is in terms of what axe-core should allow / report back to the user on.

@Westbrook
Copy link

Westbrook commented Jul 11, 2024

Thanks for the feedback @straker! 🙇🏼

Digging into that issue a little and then will see what sort of test suite I might be able to work together to get some of this information. Is there a test context that the team uses for ensuring other accessibility tree relationships that would be good to work from, in hopes of reducing friction further down the road?

@Westbrook
Copy link

Westbrook commented Jul 11, 2024

Related, @straker have you ever worked with https://playwright.dev/docs/api/class-accessibility#accessibility-snapshot? While it's deprecated (pouring on our for it), interestingly enough for your tools, I've had pretty nice success with it. Would a test suite that tests for an equivelent accessibility tree between a native DOM implementation and a custom element implementation of similar being a useful first pass?

Playwright runs across WebKit, Firefox and Chromium and while it removes the use of multiple screen readers in the test phase the assumption that the trees being of similar shape should give enough signal, even before moving to a fully manual review of the various screen readers, right?

@Westbrook
Copy link

The above would allow us to write demos like this and then test their equality like this. dom and customElement should create the same tree in each browser with domNegative confirming the test isn't accidentally giving unexpected, but matching, results.

The logs therein come out like:

 🚧 Browser logs on Chromium:
      {
        "role": "WebArea",
        "name": "",
        "children": [
          {
            "role": "image",
            "name": "An image from picsum.photos"
          }
        ]
      }
      {
        "role": "WebArea",
        "name": "",
        "children": [
          {
            "role": "image",
            "name": "An image from picsum.photos: DOM"
          }
        ]
      }
      {
        "role": "WebArea",
        "name": "",
        "children": [
          {
            "role": "image",
            "name": "An image from picsum.photos"
          }
        ]
      }
 🚧 Browser logs on Firefox:
      {
        "role": "document",
        "name": "",
        "children": [
          {
            "role": "img",
            "name": "An image from picsum.photos"
          }
        ]
      }
      {
        "role": "document",
        "name": "",
        "children": [
          {
            "role": "img",
            "name": "An image from picsum.photos: DOM"
          }
        ]
      }
      {
        "role": "document",
        "name": "",
        "children": [
          {
            "role": "img",
            "name": "An image from picsum.photos"
          }
        ]
      }
 🚧 Browser logs on Webkit:
      {
        "role": "WebArea",
        "name": "",
        "children": [
          {
            "role": "image",
            "name": "An image from picsum.photos"
          }
        ]
      }
      {
        "role": "WebArea",
        "name": "",
        "children": [
          {
            "role": "image",
            "name": "An image from picsum.photos: DOM"
          }
        ]
      }
      {
        "role": "WebArea",
        "name": "",
        "children": [
          {
            "role": "image",
            "name": "An image from picsum.photos"
          }
        ]
      }

@straker
Copy link
Contributor

straker commented Jul 11, 2024

Thanks for that information. Unfortunately what the browser reports / logs at the accessibility tree layer is only a partial understanding. In the end what matters is what screen readers do. So it will require using screen readers to understand what they do with the information they're given.

To answer the prior question, I talked with the team and we don't have a single page where we have test cases. The closet we would have is all our individual integration tests which shows how we expect different combinations of things to work. However they feel that before we ask for community help in testing that we'll need to come up with the testing matrix first to make sure we understand what needs to be covered before testing.

@Westbrook
Copy link

So the reason I think this approach works is that if this DOM:

	<div>
		<ul>
			<li>One</li>
			<li>Two</li>
			<li>Three</li>
		</ul>
		<ol>
			<li>One</li>
			<li>Two</li>
			<li>Three</li>
		</ol>
		<div role="list">
			<div role="listitem">One</div>
			<div role="listitem">Two</div>
			<div role="listitem">Three</div>
		</div>
	</div>

And this DOM:

	<style>
		/**
		* When applied via ElementInternals, Firefox adds the "list item marker" to the tree.
		* This applies them in a way that is equivelent to the native elements.
		* An in project implemenation would likely want to apply this via shadow DOM, adoptedStyleSheets, and a <slot> element.
		*/ 
		axe-list {
			list-style-type: none;
		}
		axe-list[unordered] {
			list-style-type: disc;
		}
		axe-list[ordered] {
			list-style-type: decimal;
		}
		axe-list-item {
			display: list-item;
		}
	</style>
	<div>
		<axe-list unordered>
			<axe-list-item>One</axe-list-item>
			<axe-list-item>Two</axe-list-item>
			<axe-list-item>Three</axe-list-item>
		</axe-list>
		<axe-list ordered>
			<axe-list-item>One</axe-list-item>
			<axe-list-item>Two</axe-list-item>
			<axe-list-item>Three</axe-list-item>
		</axe-list>
		<axe-list>
			<axe-list-item>One</axe-list-item>
			<axe-list-item>Two</axe-list-item>
			<axe-list-item>Three</axe-list-item>
		</axe-list>
	</div>

were to both create the same tree, it no longer matters what the screen reader does with with the accessibility tree. Either the screen readers already leverage DOM incorrectly or they will do the same things as the native elements. I understand that it isn't the same as manual testing, but I do see it as having a high quality signal as to the direction this work should go in. However, if that's still not a valid enough step, then we can work towards a different solution.

Having a testing matrix in either case will be very helpful. Would you and the team be able to prioritize preparing something like that? I'm the chairperson at https://github.com/w3c/webcomponents-cg and I'd love to get a concrete path forward that we could spread out across a number of accessibility interested members of the group (which should be all of them, 😉).

@WilcoFiers
Copy link
Contributor

As far as I can tell there is no reliable way to get to the ElementInternals object. First up, ElementInternals may not be saved as a prop on the element, or it may be a private prop. Axe-core won't ever be able to get to it if that's the case.

Even if ElementInternals is a string prop on the element, axe won't know what that is. The only way to have a chance of finding it is to loop over all props of the element, and check if any of them are an ElementInternals object. We can't just read those properties out either. There may be getters o that element. Axe can't just invoke a whole bunch of getters. That can result in all sorts of destructive things. Object.getOwnPropertyDescriptor() can help us figure out what's a getter and what isn't.

There are a lot of opportunities for things to go wrong here. And if axe can't detect an attached ElementInternals object, you are opening the floodgate to false positives. I'm more inclined to start a conversation with the standards folks to try and get an API for accessing ElementInternals. Without that I think this is far too much of a foot gun. I'd be more inclined to explicitly not support ElementInternals in axe-core and recommend against using it (or at least, its accessibility props). As much as I'd like for axe-core to support this, if it can't be done well, I'm hesitant to try and do it at all.

@Westbrook
Copy link

While the complexity of possibly expanding browser APIs moves forward...

Thinking about this more, I wonder if there is a configuration point that I've not yet found wherein a consuming developer could expand the values exported by https://github.com/dequelabs/axe-core/blob/develop/lib/commons/standards/implicit-html-roles.js#L72? In this way, while a developer would be taking on some level of responsibility for the truth of the role applied to an element, the wide ranging benefits of axe-core could still largely be leveraged across a project.

@bennypowers
Copy link

bennypowers commented Jul 19, 2024

@WilcoFiers can you patch attachInternals?

https://codepen.io/bennyp/pen/RwzaWwd

const aXeMap = new WeakMap;
const orig = HTMLElement.prototype.attachInternals;
HTMLElement.prototype.attachInternals = function () {
  const internals = orig.call(this);
  aXeMap.set(this, internals);
  return internals;
};

@dbjorge
Copy link
Contributor

dbjorge commented Dec 17, 2024

can you patch attachInternals?

No, not reliably. The most important reason is that axe-core is often injected onto a page after the page is loaded, rather than being preloaded before other scripts execute. We don't want to introduce rule differences based on whether you injected axe ahead of time vs on demand; inconsistencies in scan results for the same page like that create a lot of user confusion and support cost.

Secondarily, axe-core is often injected as part of a different javascript context from the rest of the page (eg, as part of a webextension content script) - there are workarounds for that (eg, injecting a script tag into the DOM to implement the patch), but they involve weakening the affected cases' security models.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or enhancement info needed More information or research is needed to continue standards Issues in the ARIA standards objects (lib/standards)
Projects
None yet
Development

No branches or pull requests

6 participants