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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 6 commits into from
Oct 14, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f163929
docs: ring uses css prop for track-width.
christoshrousis Oct 10, 2021
eee97d7
test: cover progress-ring
christoshrousis Oct 10, 2021
34447a3
test: migrate progress-ring tests to progress-bar
christoshrousis Oct 10, 2021
449f5e6
style: typo.
christoshrousis Oct 10, 2021
c4cbc89
revert: misunderstood part/slot definition.
christoshrousis Oct 10, 2021
9a19cc2
revert: misunderstood part/slot definition.
christoshrousis Oct 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions docs/components/progress-ring.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,10 @@ Use the `--size` custom property to set the diameter of the progress ring.

### Track Width

Use the `track-width` attribute to set the width of the progress ring's track.
Use the `--track-width` custom property to set the width of the progress ring's track.

```html preview
<sl-progress-ring value="50" stroke-width="10"></sl-progress-ring>
<sl-progress-ring value="50" style="--track-width: 10px;"></sl-progress-ring>
```

### Colors
Expand Down
89 changes: 89 additions & 0 deletions src/components/progress-bar/progress-bar.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
import { expect, fixture, html } from '@open-wc/testing';

import '../../../dist/shoelace.js';
import type SlProgressBar from './progress-bar';

describe('<sl-progress-bar>', () => {
let el: SlProgressBar;

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();
});
});
Comment on lines +9 to +17
Copy link
Contributor Author

@christoshrousis christoshrousis Oct 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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


describe('when provided a title, and value parameter', async () => {
let base: HTMLDivElement;
let indicator: HTMLDivElement;

before(async () => {
el = await fixture<SlProgressBar>(
html`<sl-progress-bar title="Titled Progress Ring" value="25"></sl-progress-bar>`
);
base = el.shadowRoot?.querySelector('[part="base"]') as HTMLDivElement;
indicator = el.shadowRoot?.querySelector('[part="indicator"]') as HTMLDivElement;
});

it('should render a component that passes accessibility test.', async () => {
await expect(el).to.be.accessible();
});

it('uses the value parameter on the base, as aria-valuenow', async () => {
expect(base).attribute('aria-valuenow', '25');
});

it('appends a % to the value, and uses it as the the value parameter to determine the width on the "indicator" part', async () => {
expect(indicator).attribute('style', 'width:25%;');
});
});

describe('when provided an indeterminate parameter', async () => {
let base: HTMLDivElement;

before(async () => {
el = await fixture<SlProgressBar>(
html`<sl-progress-bar title="Titled Progress Ring" indeterminate></sl-progress-bar>`
);
base = el.shadowRoot?.querySelector('[part="base"]') as HTMLDivElement;
});

it('should render a component that passes accessibility test.', async () => {
await expect(el).to.be.accessible();
});

it('should append a progress-bar--indeterminate class to the "base" part.', async () => {
expect(base.classList.value.trim()).to.eq('progress-bar progress-bar--indeterminate');
});
});

describe('when provided a ariaLabel, and value parameter', async () => {
before(async () => {
el = await fixture<SlProgressBar>(
html`<sl-progress-bar ariaLabel="Labelled Progress Ring" value="25"></sl-progress-bar>`
);
});

it('should render a component that passes accessibility test.', async () => {
await expect(el).to.be.accessible();
});
});

describe('when provided a ariaLabelledBy, and value parameter', async () => {
before(async () => {
el = await fixture<SlProgressBar>(
html`
<label id="labelledby">Progress Ring Label</label>
<sl-progress-bar ariaLabelledBy="labelledby" value="25"></sl-progress-bar>
`
);
});

it('should render a component that passes accessibility test.', async () => {
await expect(el).to.be.accessible();
});
});
});
15 changes: 14 additions & 1 deletion src/components/progress-bar/progress-bar.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { LitElement, html } from 'lit';
import { customElement, property } from 'lit/decorators.js';
import { ifDefined } from 'lit/directives/if-defined.js';
import { classMap } from 'lit/directives/class-map.js';
import { styleMap } from 'lit/directives/style-map.js';
import styles from './progress-bar.styles';
Expand Down Expand Up @@ -29,6 +30,15 @@ export default class SlProgressBar extends LitElement {
/** When true, percentage is ignored, the label is hidden, and the progress bar is drawn in an indeterminate state. */
@property({ type: Boolean, reflect: true }) indeterminate = false;

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

/** When set, will place a label on the progress bar. */
@property() ariaLabel: string;

/** When set, will place a labelledby on the progress bar. */
@property() ariaLabelledBy: string;

render() {
return html`
<div
Expand All @@ -38,9 +48,12 @@ export default class SlProgressBar extends LitElement {
'progress-bar--indeterminate': this.indeterminate
})}
role="progressbar"
title=${ifDefined(this.title)}
aria-label=${ifDefined(this.ariaLabel)}
aria-labelledby=${ifDefined(this.ariaLabelledBy)}
aria-valuemin="0"
aria-valuemax="100"
aria-valuenow="${this.indeterminate ? '' : this.value}"
aria-valuenow="${this.indeterminate ? 0 : this.value}"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

>
<div part="indicator" class="progress-bar__indicator" style=${styleMap({ width: this.value + '%' })}>
${!this.indeterminate
Expand Down
68 changes: 68 additions & 0 deletions src/components/progress-ring/progress-ring.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
import { expect, fixture, html } from '@open-wc/testing';

import '../../../dist/shoelace.js';
import type SlProgressRing from './progress-ring';

describe('<sl-progress-ring>', () => {
let el: SlProgressRing;

describe('when provided just a value parameter', async () => {
before(async () => {
el = await fixture<SlProgressRing>(html`<sl-progress-ring value="25"></sl-progress-ring>`);
});

it('should render a component that does not pass accessibility test.', async () => {
await expect(el).not.to.be.accessible();
});
});

describe('when provided a title, and value parameter', async () => {
let base: HTMLDivElement;

before(async () => {
el = await fixture<SlProgressRing>(
html`<sl-progress-ring title="Titled Progress Ring" value="25"></sl-progress-ring>`
);
base = el.shadowRoot?.querySelector('[part="base"]') as HTMLDivElement;
});

it('should render a component that passes accessibility test.', async () => {
await expect(el).to.be.accessible();
});

it('uses the value parameter on the base, as aria-valuenow', async () => {
expect(base).attribute('aria-valuenow', '25');
});

it('translates the value parameter to a percentage, and uses translation on the base, as percentage css variable', async () => {
expect(base).attribute('style', '--percentage: 0.25');
});
});

describe('when provided a ariaLabel, and value parameter', async () => {
before(async () => {
el = await fixture<SlProgressRing>(
html`<sl-progress-ring ariaLabel="Labelled Progress Ring" value="25"></sl-progress-ring>`
);
});

it('should render a component that passes accessibility test.', async () => {
await expect(el).to.be.accessible();
});
});

describe('when provided a ariaLabelledBy, and value parameter', async () => {
before(async () => {
el = await fixture<SlProgressRing>(
html`
<label id="labelledby">Progress Ring Label</label>
<sl-progress-ring ariaLabelledBy="labelledby" value="25"></sl-progress-ring>
`
);
});

it('should render a component that passes accessibility test.', async () => {
await expect(el).to.be.accessible();
});
});
});
13 changes: 13 additions & 0 deletions src/components/progress-ring/progress-ring.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { LitElement, html } from 'lit';
import { customElement, property, query, state } from 'lit/decorators.js';
import { ifDefined } from 'lit/directives/if-defined.js';
import styles from './progress-ring.styles';

/**
Expand Down Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

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?


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

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

updated(changedProps: Map<string, any>) {
super.updated(changedProps);

Expand All @@ -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)}
Copy link
Member

Choose a reason for hiding this comment

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

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?

aria-labelledby=${ifDefined(this.ariaLabelledBy)}
Copy link
Member

@claviska claviska Oct 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

aria-valuemin="0"
aria-valuemax="100"
aria-valuenow="${this.value}"
Expand Down