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

fix(tree,tree-item): replace checkbox with div and a11y attributes #9849

Merged
merged 18 commits into from
Aug 12, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
8462d8b
fix(tree-item): provide a valid a11y label for form fields
aPreciado88 Jul 24, 2024
6ea4dd3
fix(tree,tree-item): provide a valid a11y label for form fields
aPreciado88 Jul 24, 2024
7bf7434
fix(tree,tree-item): add label prop to storybook tests
aPreciado88 Jul 24, 2024
cda50ab
fix(tree,tree-item): update indeterminate state checkbox icon and end…
aPreciado88 Jul 25, 2024
8bf68f3
fix(tree,tree-item): remove checkbox on hover css
aPreciado88 Jul 25, 2024
3cbb666
Merge branch 'dev' into aPreciado88/5615-provide-valid-label-for-form…
aPreciado88 Jul 26, 2024
e3c3bb0
fix(tree-item): update a11y props and end to end test
aPreciado88 Jul 29, 2024
eee773b
Merge branch 'aPreciado88/5615-provide-valid-label-for-form-fields' o…
aPreciado88 Jul 29, 2024
f0053a1
fix(tree-item): add aria-selected or aria-checked based on selectionMode
aPreciado88 Jul 29, 2024
b2643ee
fix(tree-item): remove tabIndex prop from checkbox div tag
aPreciado88 Jul 30, 2024
a472f5a
fix(tree-item): remove div tag wrapping checkbox icon tag
aPreciado88 Jul 30, 2024
c60efd7
fix(tree-item): update end to end tests
aPreciado88 Jul 30, 2024
da40476
Merge branch 'dev' into aPreciado88/5615-provide-valid-label-for-form…
aPreciado88 Jul 30, 2024
bd3f229
fix(tree-item): remove focused checkbox styling
aPreciado88 Jul 30, 2024
5fe1cfc
Merge branch 'aPreciado88/5615-provide-valid-label-for-form-fields' o…
aPreciado88 Jul 30, 2024
6b69401
fix(tree-item): merge dev
aPreciado88 Jul 30, 2024
d8fda37
Merge branch 'dev' into aPreciado88/5615-provide-valid-label-for-form…
aPreciado88 Aug 1, 2024
ca0770a
Merge branch 'dev' into aPreciado88/5615-provide-valid-label-for-form…
aPreciado88 Aug 12, 2024
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
8 changes: 8 additions & 0 deletions packages/calcite-components/src/components.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5657,6 +5657,10 @@ export namespace Components {
* In ancestor selection mode, show as indeterminate when only some children are selected.
*/
"indeterminate": boolean;
/**
* Accessible name for the component.
*/
"label": string;
"lines": boolean;
"parentExpanded": boolean;
"scale": Scale;
Expand Down Expand Up @@ -13700,6 +13704,10 @@ declare namespace LocalJSX {
* In ancestor selection mode, show as indeterminate when only some children are selected.
*/
"indeterminate"?: boolean;
/**
* Accessible name for the component.
*/
"label"?: string;
"lines"?: boolean;
"onCalciteInternalTreeItemSelect"?: (event: CalciteTreeItemCustomEvent<TreeItemSelectDetail>) => void;
"parentExpanded"?: boolean;
Expand Down
16 changes: 10 additions & 6 deletions packages/calcite-components/src/components/tree-item/resources.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,16 @@
export const CSS = {
actionsEnd: "actions-end",
checkboxLabel: "checkbox-label",
bulletPointIcon: "bullet-point",
checkbox: "checkbox",
checkboxContainer: "checkbox-container",
checkboxLabel: "checkbox-label",
checkmarkIcon: "checkmark",
chevron: "chevron",
nodeContainer: "node-container",
childrenContainer: "children-container",
bulletPointIcon: "bullet-point",
checkmarkIcon: "checkmark",
itemExpanded: "item--expanded",
iconStart: "icon-start",
itemExpanded: "item--expanded",
nodeAndActionsContainer: "node-actions-container",
nodeContainer: "node-container",
};

export const SLOTS = {
Expand All @@ -18,8 +19,11 @@ export const SLOTS = {
};

export const ICONS = {
blank: "blank",
bulletPoint: "bullet-point",
checkmark: "check",
checkSquareF: "check-square-f",
chevronRight: "chevron-right",
blank: "blank",
minusSquareF: "minus-square-f",
square: "square",
} as const;
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { E2EPage, newE2EPage } from "@stencil/core/testing";
import { accessible, defaults, disabled, hidden, renders, slots } from "../../tests/commonTests";
import { html } from "../../../support/formatting";
import { SLOTS } from "./resources";
import { CSS, SLOTS } from "./resources";

describe("calcite-tree-item", () => {
describe("renders", () => {
Expand Down Expand Up @@ -331,8 +331,8 @@ describe("calcite-tree-item", () => {
</calcite-tree>
`);
const container = await page.find("calcite-tree-item >>> .node-container");
const label = await container.find("label");
const checkbox = await label.find("calcite-checkbox");
const checkbox = await container.find(`.${CSS.checkbox}`);
const label = await container.find(`label`);

const icon = await container.find(`[data-test-id="icon"]`);
await icon.click();
Expand All @@ -342,24 +342,24 @@ describe("calcite-tree-item", () => {
expect(isVisible).toBe(true);

await container.click();
expect(checkbox).toHaveAttribute("checked");
expect(checkbox).toEqualAttribute("aria-checked", true);
expect(isVisible).toBe(true);
await container.click();
expect(checkbox).not.toHaveAttribute("checked");
expect(checkbox).toEqualAttribute("aria-checked", false);
expect(isVisible).toBe(true);

await label.click();
expect(checkbox).toHaveAttribute("checked");
expect(checkbox).toEqualAttribute("aria-checked", true);
expect(isVisible).toBe(true);
await label.click();
expect(checkbox).not.toHaveAttribute("checked");
expect(checkbox).toEqualAttribute("aria-checked", false);
expect(isVisible).toBe(true);

await checkbox.click();
expect(checkbox).toHaveAttribute("checked");
expect(checkbox).toEqualAttribute("aria-checked", true);
expect(isVisible).toBe(true);
await checkbox.click();
expect(checkbox).not.toHaveAttribute("checked");
expect(checkbox).toEqualAttribute("aria-checked", false);
expect(isVisible).toBe(true);
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -134,16 +134,23 @@
@apply flex self-stretch flex-row items-center;
}

.checkbox {
line-height: 0;
.checkbox-container {
display: flex;
align-items: center;
}

.checkbox-label {
@apply pointer-events-none flex items-center;
.checkbox {
line-height: 0;
color: var(--calcite-color-border-input);
}

.checkbox:focus {
@apply outline-none;
color: var(--calcite-color-brand);
}

.checkbox-label {
@apply pointer-events-none flex items-center;
}

.children-container {
Expand Down Expand Up @@ -253,6 +260,15 @@
.bullet-point {
color: var(--calcite-color-brand);
}
.checkbox {
color: var(--calcite-color-brand);
}
}

:host([has-children][indeterminate]) {
.checkbox {
color: var(--calcite-color-brand);
}
}

@include base-component();
32 changes: 23 additions & 9 deletions packages/calcite-components/src/components/tree-item/tree-item.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,9 @@ export class TreeItem implements ConditionalSlotComponent, InteractiveComponent
*/
@Prop({ reflect: true }) disabled = false;

/** Accessible name for the component. */
@Prop() label: string;
aPreciado88 marked this conversation as resolved.
Show resolved Hide resolved

/** When `true`, the component is expanded. */
@Prop({ mutable: true, reflect: true }) expanded = false;

Expand Down Expand Up @@ -210,6 +213,7 @@ export class TreeItem implements ConditionalSlotComponent, InteractiveComponent
const showCheckmark =
this.selectionMode === "multiple" || this.selectionMode === "multichildren";
const showBlank = this.selectionMode === "none" && !this.hasChildren;
const checkboxIsIndeterminate = this.hasChildren && this.indeterminate;

const chevron = this.hasChildren ? (
<calcite-icon
Expand All @@ -227,17 +231,27 @@ export class TreeItem implements ConditionalSlotComponent, InteractiveComponent

const checkbox =
this.selectionMode === "ancestors" ? (
<label class={CSS.checkboxLabel} key="checkbox-label">
<calcite-checkbox
checked={this.selected}
<div class={CSS.checkboxContainer}>
<div
aria-checked={toAriaBoolean(this.selected)}
Copy link
Member

Choose a reason for hiding this comment

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

I think this div might need a role (maybe checkbox). I ran the test app through aXe toolbar and it showed this:

image

@geospatialem do you know what this might need?

I'm not sure we even need aria-checked here because the tree-item role has aria-selected so that should really account for selection to a screen reader right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@driskull Adding role="checkbox" gets rid of the error above. Do we want to get rid of aria-checked entirely?

cc @geospatialem

Copy link
Member

Choose a reason for hiding this comment

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

From testing, it looks like we should omit this div to only contain the class and tabIndex. For instance:

<div class={CSS.checkboxContainer}>
  <div class={CSS.checkbox} tabIndex={-1}>
    <calcite-icon
      ...

Copy link
Member

Choose a reason for hiding this comment

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

Instead we'd add the contents to the Host tree-item element to announce the selection via aria-live="polite":

<Host
  aria-checked={this.selectionMode === "ancestors" ? toAriaBoolean(this.selected) : undefined}
  aria-expanded={this.hasChildren ? toAriaBoolean(isExpanded) : undefined}
  aria-hidden={toAriaBoolean(hidden)}
  aria-live="polite"
  calcite-hydrated-hidden={hidden}
  role="treeitem"
  tabIndex={this.disabled ? -1 : 0}
>

The above would also remove the aria-selected attribute, which currently contains:

  ...
  aria-selected={this.selected ? "true" : showCheckmark ? "false" : undefined}
  ...

Not sure if there is an impact there, but I didn't notice one in testing.

The above two code changes allow JAWS and NVDA to hear the selections and live changes, in addition to passing all axe conditions.

aria-disabled={this.disabled}
aria-label={this.label}
class={CSS.checkbox}
data-test-id="checkbox"
indeterminate={this.hasChildren && this.indeterminate}
scale={this.scale}
tabIndex={-1}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@driskull Got rid of tabIndex in the latest update 🐱‍💻

/>
{defaultSlotNode}
</label>
>
<calcite-icon
icon={
this.selected
? ICONS.checkSquareF
: checkboxIsIndeterminate
? ICONS.minusSquareF
: ICONS.square
}
scale={getIconScale(this.scale)}
/>
geospatialem marked this conversation as resolved.
Show resolved Hide resolved
</div>
<label class={CSS.checkboxLabel}>{defaultSlotNode}</label>
</div>
) : null;
const selectedIcon = showBulletPoint
? ICONS.bulletPoint
Expand Down
4 changes: 1 addition & 3 deletions packages/calcite-components/src/components/tree/tree.e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -434,9 +434,7 @@ describe("calcite-tree", () => {
</calcite-tree>
`,
});
const checkbox = await page.find(
`calcite-tree-item >>> .${CSS.nodeContainer} .${CSS.checkboxLabel} .${CSS.checkbox}`,
);
const checkbox = await page.find(`calcite-tree-item >>> .${CSS.nodeContainer} .${CSS.checkboxContainer}`);
expect(checkbox).not.toBeNull();
});
});
Expand Down
Loading
Loading