test: components/progress-bar components/progress-ring #562

christoshrousis · 2021-10-10T02:31:49Z

This PR aims to cover props passed to progress-bar/progress-ring.

docs/components/progress-ring.md had the incorrect instructions for updating track width.

title/aria-label/aria-labelledby to give the best AXE coverage, I suspect that users will expect a title, which gives a native hoverable when you hover over the SVG of the graphs. A user may wish to omit this, or they may wish to have an external label, so all three are required.
https://dequeuniversity.com/rules/axe/4.1/aria-progressbar-name

- Add title to make ring accessibly hoverable. - Add label/labelledby as aria options. - Remove ununsed label slot.

- Match coverage with progress-ring - Attached titles/label/labelledby - Value '' on aria-valuenow is does not pass AXE

vercel · 2021-10-10T02:31:54Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/shoelace/shoelace/E9HADu39vq2z4AEE2FRsGExPSFxC
✅ Preview: https://shoelace-git-fork-christoshrousis-test-progress-shoelace.vercel.app

christoshrousis · 2021-10-10T02:32:26Z

src/components/progress-bar/progress-bar.test.ts

+  describe('when provided just a value parameter', async () => {
+    before(async () => {
+      el = await fixture<SlProgressBar>(html`<sl-progress-bar value="25"></sl-progress-bar>`);
+    });
+
+    it('should render a component that does not pass accessibility test.', async () => {
+      await expect(el).not.to.be.accessible();
+    });
+  });


The default progress isn't accessible, should this be reflected in the documentation?

For others reading, the test fails with:

Error: Accessibility Violations --- Rule: aria-progressbar-name Impact: serious ARIA progressbar nodes must have an accessible name (https://dequeuniversity.com/rules/axe/4.2/aria-progressbar-name?application=axeAPI) Issue target: sl-progress-ring,.progress-ring Context: <div part="base" class="progress-ring" role="progressbar" aria-valuemin="0" aria-valuemax="100" aria-valuenow="25" style="--percentage: 0.25"> Fix any of the following: aria-label attribute does not exist or is empty aria-labelledby attribute does not exist, references elements that do not exist or references elements that are empty Element has no title attribute --- aria-labelledby attribute does not exist, references elements that do not exist or references elements that are empty Element has no title attribute --- at processResults (node_modules/chai-a11y-axe/src/accessible.js:87:15) at node_modules/chai-a11y-axe/src/accessible.js:117:7 at async o.<anonymous> (src/components/progress-ring/progress-ring.test.ts:15:6)

Would using a localized term such as "Progress" be a good default? Seems better than nothing. Then the user can choose to customize it with something more specific, e.g. "File upload progress".

christoshrousis · 2021-10-10T02:33:35Z

src/components/progress-bar/progress-bar.ts

        aria-valuemin="0"
        aria-valuemax="100"
-        aria-valuenow="${this.indeterminate ? '' : this.value}"
+        aria-valuenow="${this.indeterminate ? 0 : this.value}"


While the progress-bar is indeterminate, I suspect that it's closest to "0". We require a value other that '' for it to pass AXE.

claviska

Thanks, as always 💙

After reviewing this and leaving some notes, I think it might be best to remove the base part element and defer title, aria-label, and aria-labelledby to the host element.

Let me know what you think. I'm happy to pick up this work if you prefer!

claviska · 2021-10-11T12:02:01Z

src/components/progress-ring/progress-ring.ts

@@ -27,6 +28,15 @@ export default class SlProgressRing extends LitElement {
  /** The current progress, 0 to 100. */
  @property({ type: Number, reflect: true }) value = 0;

+  /** When set, will place a hoverable title on the progress ring. */
+  @property() title: string;


This will technically put a title on the host element and the base part. Is it necessary to pass it through or can we leave it on the host and omit this declaration?

claviska · 2021-10-11T12:04:09Z

src/components/progress-ring/progress-ring.ts

@@ -50,6 +60,9 @@ export default class SlProgressRing extends LitElement {
        part="base"
        class="progress-ring"
        role="progressbar"
+        title=${ifDefined(this.title)}
+        aria-label=${ifDefined(this.ariaLabel)}


So far I've avoided using aria-like attributes as part of Shoelace's API. In some components, you'll see label attribute that are used to populate an internal aria label. The idea is that it's harder to mess up accessibility if the library handles it internally. Can we use label instead?

claviska · 2021-10-11T12:06:12Z

src/components/progress-ring/progress-ring.ts

@@ -50,6 +60,9 @@ export default class SlProgressRing extends LitElement {
        part="base"
        class="progress-ring"
        role="progressbar"
+        title=${ifDefined(this.title)}
+        aria-label=${ifDefined(this.ariaLabel)}
+        aria-labelledby=${ifDefined(this.ariaLabelledBy)}


I don't think this will work since the property is defined on the host (light DOM) and the aria attribute is on the base part (shadow DOM).

One way to make this "just work" is to remove the base part and make the <sl-progress-ring|bar> have role="progressbar". Then you'll get title, aria-label, and aria-labelled by for free.

If that sounds like a good idea, let me know and I'll merge this and make those updates. 😄

claviska · 2021-10-11T12:09:20Z

src/components/progress-bar/progress-bar.test.ts

+  describe('when provided just a value parameter', async () => {
+    before(async () => {
+      el = await fixture<SlProgressBar>(html`<sl-progress-bar value="25"></sl-progress-bar>`);
+    });
+
+    it('should render a component that does not pass accessibility test.', async () => {
+      await expect(el).not.to.be.accessible();
+    });
+  });


For others reading, the test fails with:

Error: Accessibility Violations --- Rule: aria-progressbar-name Impact: serious ARIA progressbar nodes must have an accessible name (https://dequeuniversity.com/rules/axe/4.2/aria-progressbar-name?application=axeAPI) Issue target: sl-progress-ring,.progress-ring Context: <div part="base" class="progress-ring" role="progressbar" aria-valuemin="0" aria-valuemax="100" aria-valuenow="25" style="--percentage: 0.25"> Fix any of the following: aria-label attribute does not exist or is empty aria-labelledby attribute does not exist, references elements that do not exist or references elements that are empty Element has no title attribute --- aria-labelledby attribute does not exist, references elements that do not exist or references elements that are empty Element has no title attribute --- at processResults (node_modules/chai-a11y-axe/src/accessible.js:87:15) at node_modules/chai-a11y-axe/src/accessible.js:117:7 at async o.<anonymous> (src/components/progress-ring/progress-ring.test.ts:15:6)

Would using a localized term such as "Progress" be a good default? Seems better than nothing. Then the user can choose to customize it with something more specific, e.g. "File upload progress".

christoshrousis · 2021-10-13T09:34:55Z

After reviewing this and leaving some notes, I think it might be best to remove the base part element and defer title, aria-label, and aria-labelledby to the host element.

I suspect this makes sense, but what does that mean for the other progress bar aria attributes like aria-valuemin ? They all have to reside on the same element? Can I place those on the host element? I assume I'm missing some knowledge about Web Components in general.

I'm happy to try review later in the week, or if you want to do it that's fine by me to.

claviska · 2021-10-14T12:10:41Z

I suspect this makes sense, but what does that mean for the other progress bar aria attributes like aria-valuemin ? They all have to reside on the same element?

Yeah they would need to be moved to the host element as well.

claviska · 2021-10-14T12:27:24Z

I went with a label attribute for now, which is more in line with how other components are handling it. Updated the tests to expect a pass, as the default label is "Progress." With #419, the default value can localized.

christoshrousis added 4 commits October 10, 2021 13:00

docs: ring uses css prop for track-width.

f163929

test: cover progress-ring

eee97d7

- Add title to make ring accessibly hoverable. - Add label/labelledby as aria options. - Remove ununsed label slot.

test: migrate progress-ring tests to progress-bar

34447a3

- Match coverage with progress-ring - Attached titles/label/labelledby - Value '' on aria-valuenow is does not pass AXE

style: typo.

449f5e6

vercel bot deployed to Preview October 10, 2021 02:31 View deployment

christoshrousis commented Oct 10, 2021

View reviewed changes

revert: misunderstood part/slot definition.

c4cbc89

vercel bot deployed to Preview October 10, 2021 02:36 View deployment

revert: misunderstood part/slot definition.

9a19cc2

vercel bot deployed to Preview October 10, 2021 02:37 View deployment

claviska requested changes Oct 11, 2021

View reviewed changes

claviska merged commit 45ceff4 into shoelace-style:next Oct 14, 2021

christoshrousis deleted the test/progress branch October 24, 2021 01:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: components/progress-bar components/progress-ring #562

test: components/progress-bar components/progress-ring #562

christoshrousis commented Oct 10, 2021

vercel bot commented Oct 10, 2021 •

edited

Loading

christoshrousis Oct 10, 2021 •

edited

Loading

claviska Oct 11, 2021

christoshrousis Oct 10, 2021

claviska left a comment

claviska Oct 11, 2021

claviska Oct 11, 2021

claviska Oct 11, 2021 •

edited

Loading

claviska Oct 11, 2021

christoshrousis commented Oct 13, 2021 •

edited

Loading

claviska commented Oct 14, 2021

claviska commented Oct 14, 2021

test: components/progress-bar components/progress-ring #562

test: components/progress-bar components/progress-ring #562

Conversation

christoshrousis commented Oct 10, 2021

vercel bot commented Oct 10, 2021 • edited Loading

christoshrousis Oct 10, 2021 • edited Loading

Choose a reason for hiding this comment

claviska Oct 11, 2021

Choose a reason for hiding this comment

christoshrousis Oct 10, 2021

Choose a reason for hiding this comment

claviska left a comment

Choose a reason for hiding this comment

claviska Oct 11, 2021

Choose a reason for hiding this comment

claviska Oct 11, 2021

Choose a reason for hiding this comment

claviska Oct 11, 2021 • edited Loading

Choose a reason for hiding this comment

claviska Oct 11, 2021

Choose a reason for hiding this comment

christoshrousis commented Oct 13, 2021 • edited Loading

claviska commented Oct 14, 2021

claviska commented Oct 14, 2021

vercel bot commented Oct 10, 2021 •

edited

Loading

christoshrousis Oct 10, 2021 •

edited

Loading

claviska Oct 11, 2021 •

edited

Loading

christoshrousis commented Oct 13, 2021 •

edited

Loading