Skip to content

Commit

Permalink
fix: ensure label only focuses the first labelable child (#7553)
Browse files Browse the repository at this point in the history 
**Related Issue:** #5070 

## Summary

Ensures label clicks focus on the first labelable child and ignores
clicks on labelable children (as they will already receive focus).

### Noteworthy changes

#### `label.ts`

* no longer binds label click handler per labelable

#### `commonTests.ts`/`labelable` test helper

* now asserts that a clicked labelable child will own focus
* grouped tests for both wrapped and sibling use cases
* removed unnecessary `for` usage in wrapped test coverage

### `dom.ts`

* add `isBefore` util to determine DOM order of elements (useful for
sorting)
  • Loading branch information
@jcfranco
jcfranco authored Aug 22, 2023
1 parent 96e8a9f commit 426159c
Show file tree
Hide file tree
Showing 6 changed files with 292 additions and 123 deletions.
5 changes: 4 additions & 1 deletion packages/calcite-components/src/components/inline-editable/inline-editable.e2e.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,10 @@ describe("calcite-inline-editable", () => {
labelable(
`<calcite-inline-editable controls>
<calcite-input value="John Doe"></calcite-input>
</calcite-inline-editable>`
</calcite-inline-editable>`,
{
focusTargetSelector: "calcite-input",
}
);
});

Expand Down
242 changes: 138 additions & 104 deletions packages/calcite-components/src/tests/commonTests.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { html } from "../../support/formatting";
import { JSX } from "../components";
import { hiddenFormInputSlotName } from "../utils/form";
import { MessageBundle } from "../utils/t9n";
import { GlobalTestProps, skipAnimations } from "./utils";
import { GlobalTestProps, isElementFocused, skipAnimations } from "./utils";
import { InteractiveHTMLElement } from "../utils/interactive";

expect.extend(toHaveNoViolations);
Expand Down Expand Up @@ -383,10 +383,10 @@ async function assertLabelable({
focusTargetSelector?: string;
shadowFocusTargetSelector?: string;
}): Promise<void> {
let component: E2EElement;
let initialPropertyValue: boolean;
const component = await page.find(componentTag);

if (propertyToToggle) {
component = await page.find(componentTag);
initialPropertyValue = await component.getProperty(propertyToToggle);
}

Expand Down Expand Up @@ -422,6 +422,12 @@ async function assertLabelable({
await page.waitForChanges();
expect(await component.getProperty(propertyToToggle)).toBe(toggledPropertyValue);
}

// assert clicking on labelable keeps focus
await component.callMethod("click");
await page.waitForChanges();

expect(await isElementFocused(page, focusTargetSelector)).toBe(true);
}

interface LabelableOptions extends Pick<FocusableOptions, "focusTargetSelector" | "shadowFocusTargetSelector"> {
Expand Down Expand Up @@ -459,118 +465,146 @@ export function labelable(componentTagOrHtml: TagOrHTML, options?: LabelableOpti
return html.includes("id=") ? html : html.replace(componentTag, `${componentTag} id="${id}" `);
}

it("is labelable when component is wrapped in a label", async () => {
const wrappedHtml = html`<calcite-label> ${labelTitle} ${componentHtml}</calcite-label>`;
const wrappedPage: E2EPage = await newE2EPage({ html: wrappedHtml });
await wrappedPage.waitForChanges();

await assertLabelable({
page: wrappedPage,
componentTag,
propertyToToggle,
focusTargetSelector,
shadowFocusTargetSelector,
describe("label wraps labelables", () => {
it("is labelable when component is wrapped in a label", async () => {
const wrappedHtml = html`<calcite-label>${labelTitle} ${componentHtml}</calcite-label>`;
const wrappedPage: E2EPage = await newE2EPage({ html: wrappedHtml });
await wrappedPage.waitForChanges();

await assertLabelable({
page: wrappedPage,
componentTag,
propertyToToggle,
focusTargetSelector,
shadowFocusTargetSelector,
});
});
});

it("is labelable with label set as a sibling to the component", async () => {
const siblingHtml = html`
<calcite-label for="${id}">${labelTitle}</calcite-label>
${componentHtml}
`;
const siblingPage: E2EPage = await newE2EPage({ html: siblingHtml });
await siblingPage.waitForChanges();

await assertLabelable({
page: siblingPage,
componentTag,
propertyToToggle,
focusTargetSelector,
shadowFocusTargetSelector,
it("is labelable when wrapping label is set prior to component", async () => {
const labelFirstWrappedPage: E2EPage = await newE2EPage();
await labelFirstWrappedPage.setContent(`<calcite-label></calcite-label>`);
await labelFirstWrappedPage.waitForChanges();
await labelFirstWrappedPage.evaluate((componentHtml: string) => {
const template = document.createElement("template");
template.innerHTML = `${componentHtml}`.trim();
const componentNode = template.content.firstChild;
const labelEl = document.querySelector("calcite-label");
labelEl.append(componentNode);
}, componentHtml);
await labelFirstWrappedPage.waitForChanges();

await assertLabelable({
page: labelFirstWrappedPage,
componentTag,
propertyToToggle,
focusTargetSelector,
shadowFocusTargetSelector,
});
});

it("is labelable when a component is set first before being wrapped in a label", async () => {
const componentFirstWrappedPage: E2EPage = await newE2EPage();
await componentFirstWrappedPage.setContent(`${componentHtml}`);
await componentFirstWrappedPage.waitForChanges();
await componentFirstWrappedPage.evaluate((id: string) => {
const componentEl = document.querySelector(`[id='${id}']`);
const labelEl = document.createElement("calcite-label");
document.body.append(labelEl);
labelEl.append(componentEl);
}, id);
await componentFirstWrappedPage.waitForChanges();

await assertLabelable({
page: componentFirstWrappedPage,
componentTag,
propertyToToggle,
focusTargetSelector,
shadowFocusTargetSelector,
});
});
});

it("is labelable when sibling label is set prior to component", async () => {
const labelFirstSiblingPage: E2EPage = await newE2EPage();
await labelFirstSiblingPage.setContent(`<calcite-label for="${id}"></calcite-label>`);
await labelFirstSiblingPage.waitForChanges();
await labelFirstSiblingPage.evaluate((componentHtml: string) => {
const template = document.createElement("template");
template.innerHTML = `${componentHtml}`.trim();
const componentNode = template.content.firstChild;
document.body.append(componentNode);
}, componentHtml);
await labelFirstSiblingPage.waitForChanges();

await assertLabelable({
page: labelFirstSiblingPage,
componentTag,
propertyToToggle,
focusTargetSelector,
shadowFocusTargetSelector,
it("only sets focus on the first labelable when label is clicked", async () => {
const firstLabelableId = `${id}`;
const componentFirstWrappedPage: E2EPage = await newE2EPage();
await componentFirstWrappedPage.setContent(html`
<calcite-label>
<!-- duplicate tags should be fine as assertion uses first match -->
${componentHtml.replace(id, firstLabelableId)} ${componentHtml.replace(id, `${id}-2`)}
${componentHtml.replace(id, `${id}-3`)}
</calcite-label>
`);
await componentFirstWrappedPage.waitForChanges();

const firstLabelableSelector = `#${firstLabelableId}`;

await assertLabelable({
page: componentFirstWrappedPage,
componentTag,
propertyToToggle,
focusTargetSelector:
focusTargetSelector === firstLabelableSelector
? firstLabelableSelector
: `${firstLabelableSelector} ${focusTargetSelector}`,
});
});
});

it("is labelable when wrapping label is set prior to component", async () => {
const labelFirstWrappedPage: E2EPage = await newE2EPage();
await labelFirstWrappedPage.setContent(`<calcite-label for="${id}"></calcite-label>`);
await labelFirstWrappedPage.waitForChanges();
await labelFirstWrappedPage.evaluate((componentHtml: string) => {
const template = document.createElement("template");
template.innerHTML = `${componentHtml}`.trim();
const componentNode = template.content.firstChild;
const labelEl = document.querySelector("calcite-label");
labelEl.append(componentNode);
}, componentHtml);
await labelFirstWrappedPage.waitForChanges();

await assertLabelable({
page: labelFirstWrappedPage,
componentTag,
propertyToToggle,
focusTargetSelector,
shadowFocusTargetSelector,
describe("label is sibling to labelables", () => {
it("is labelable with label set as a sibling to the component", async () => {
const siblingHtml = html`
<calcite-label for="${id}">${labelTitle}</calcite-label>
${componentHtml}
`;
const siblingPage: E2EPage = await newE2EPage({ html: siblingHtml });
await siblingPage.waitForChanges();

await assertLabelable({
page: siblingPage,
componentTag,
propertyToToggle,
focusTargetSelector,
shadowFocusTargetSelector,
});
});
});

it("is labelable for a component set before sibling label", async () => {
const componentFirstSiblingPage: E2EPage = await newE2EPage({ html: componentHtml });
await componentFirstSiblingPage.waitForChanges();
await componentFirstSiblingPage.evaluate((id: string) => {
const label = document.createElement("calcite-label");
label.setAttribute("for", `${id}`);
document.body.append(label);
}, id);
await componentFirstSiblingPage.waitForChanges();

await assertLabelable({
page: componentFirstSiblingPage,
componentTag,
propertyToToggle,
focusTargetSelector,
shadowFocusTargetSelector,
it("is labelable when sibling label is set prior to component", async () => {
const labelFirstSiblingPage: E2EPage = await newE2EPage();
await labelFirstSiblingPage.setContent(`<calcite-label for="${id}"></calcite-label>`);
await labelFirstSiblingPage.waitForChanges();
await labelFirstSiblingPage.evaluate((componentHtml: string) => {
const template = document.createElement("template");
template.innerHTML = `${componentHtml}`.trim();
const componentNode = template.content.firstChild;
document.body.append(componentNode);
}, componentHtml);
await labelFirstSiblingPage.waitForChanges();

await assertLabelable({
page: labelFirstSiblingPage,
componentTag,
propertyToToggle,
focusTargetSelector,
shadowFocusTargetSelector,
});
});
});

it("is labelable when a component is set first before being wrapped in a label", async () => {
const componentFirstWrappedPage: E2EPage = await newE2EPage();
await componentFirstWrappedPage.setContent(`${componentHtml}`);
await componentFirstWrappedPage.waitForChanges();
await componentFirstWrappedPage.evaluate((id: string) => {
const componentEl = document.querySelector(`[id='${id}']`);
const labelEl = document.createElement("calcite-label");
labelEl.setAttribute("for", `${id}`);
document.body.append(labelEl);
labelEl.append(componentEl);
}, id);
await componentFirstWrappedPage.waitForChanges();

await assertLabelable({
page: componentFirstWrappedPage,
componentTag,
propertyToToggle,
focusTargetSelector,
shadowFocusTargetSelector,
it("is labelable for a component set before sibling label", async () => {
const componentFirstSiblingPage: E2EPage = await newE2EPage({ html: componentHtml });
await componentFirstSiblingPage.waitForChanges();
await componentFirstSiblingPage.evaluate((id: string) => {
const label = document.createElement("calcite-label");
label.setAttribute("for", `${id}`);
document.body.append(label);
}, id);
await componentFirstSiblingPage.waitForChanges();

await assertLabelable({
page: componentFirstSiblingPage,
componentTag,
propertyToToggle,
focusTargetSelector,
shadowFocusTargetSelector,
});
});
});
}
Expand Down
21 changes: 21 additions & 0 deletions packages/calcite-components/src/utils/dom.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
slotChangeHasAssignedNode,
slotChangeHasTextContent,
slotChangeHasContent,
isBefore,
} from "./dom";
import { guidPattern } from "./guid.spec";

Expand Down Expand Up @@ -573,4 +574,24 @@ describe("dom", () => {
expect(getShadowRootNode(document.body.querySelector("div"))).toBe(null);
});
});

describe("isBefore", () => {
let div1: HTMLDivElement;
let div2: HTMLDivElement;

beforeEach(() => {
div1 = document.createElement("div");
div2 = document.createElement("div");
});

it("should return true if element A is before element B", () => {
document.body.append(div1, div2);
expect(isBefore(div1, div2)).toBe(true);
});

it("should return false if element A is after element B", () => {
document.body.append(div2, div1);
expect(isBefore(div1, div2)).toBe(false);
});
});
});
17 changes: 17 additions & 0 deletions packages/calcite-components/src/utils/dom.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -583,3 +583,20 @@ export const focusElementInGroup = (
focusElement(focusTarget);
return focusTarget;
};

/**
* This helper determines if an element is before another element in the DOM.
*
* @param a the reference element to compare
* @param b the element to compare against
*
* @returns true when a is before b in the DOM
*/
export function isBefore(a: HTMLElement, b: HTMLElement): boolean {
if (a.parentNode !== b.parentNode) {
return false;
}

const children = Array.from(a.parentNode.children);
return children.indexOf(a) < children.indexOf(b);
}
Loading

0 comments on commit 426159c

Please sign in to comment.