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

fix(rating): adds focus outline on click #7341

Merged
merged 26 commits into from
Aug 26, 2023
Merged

fix(rating): adds focus outline on click #7341

Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
5f11d98
fix(combobox): announces all selected items to AT users
anveshmekala Jul 14, 2023
08e7208
Merge branch 'main' of https://github.com/Esri/calcite-components
anveshmekala Jul 18, 2023
eb62755
Merge branch 'main' of https://github.com/Esri/calcite-components
anveshmekala Jul 19, 2023
feaadb4
feat(rating,switch): updates focus outline
anveshmekala Jul 19, 2023
2e6fd9e
revert combobx changes
anveshmekala Jul 19, 2023
556d02b
fix e2e errors
anveshmekala Jul 20, 2023
6b59555
Merge branch 'main' into anveshmekala/4633-fix-hitbox-focus
anveshmekala Jul 20, 2023
fb1b9af
add key for label element
anveshmekala Jul 20, 2023
d393ea0
refactor
anveshmekala Aug 4, 2023
d74e04d
Merge branch 'main' into anveshmekala/4633-fix-hitbox-focus
anveshmekala Aug 4, 2023
4e6f24d
revert switch component changes
anveshmekala Aug 4, 2023
13cc13e
prevent focus from shifting to input on label click
anveshmekala Aug 7, 2023
d9265d1
fix tests
anveshmekala Aug 7, 2023
6ffdff5
refactor
anveshmekala Aug 8, 2023
314330e
remove hasFocus state property
anveshmekala Aug 8, 2023
c5fd09f
remove focusValue state property
anveshmekala Aug 8, 2023
a132136
Merge branch 'main' into anveshmekala/4633-fix-hitbox-focus
anveshmekala Aug 8, 2023
ebc255d
fix setFocus()
anveshmekala Aug 8, 2023
a05fc2d
cleanup
anveshmekala Aug 8, 2023
e467dff
fix focus tests
anveshmekala Aug 9, 2023
dd228b1
add focus test for chromatic
anveshmekala Aug 9, 2023
3df8cc1
feedback changes
anveshmekala Aug 9, 2023
efc577b
feedback changes
anveshmekala Aug 18, 2023
fa9c5d9
add comments
anveshmekala Aug 18, 2023
3ca7674
Merge branch 'main' into anveshmekala/4633-fix-hitbox-focus
anveshmekala Aug 25, 2023
77784d2
update selector in tests
anveshmekala Aug 25, 2023
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
3 changes: 1 addition & 2 deletions packages/calcite-components/src/components/rating/interfaces.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,13 @@ import { Scale } from "../interfaces";
export interface Star {
average: boolean;
checked: boolean;
focused: boolean;
fraction: number;
hovered: boolean;
id: string;
idx: number;
partial: boolean;
selected: boolean;
value: number;
tabIndex: number;
}

export interface StarIconProps {
Expand Down
79 changes: 50 additions & 29 deletions packages/calcite-components/src/components/rating/rating.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
renders,
t9n,
} from "../../tests/commonTests";
import { getFocusedElementProp, isElementFocused } from "../../tests/utils";

describe("calcite-rating", () => {
describe("common tests", () => {
Expand Down Expand Up @@ -38,19 +39,19 @@ describe("calcite-rating", () => {

describe("should focus input element in shadow DOM", () => {
focusable("calcite-rating", {
shadowFocusTargetSelector: "input",
shadowFocusTargetSelector: "label",
});
});

describe("focuses the first star when the label is clicked and no-rating value exists", () => {
labelable("calcite-rating", {
shadowFocusTargetSelector: "input[value='1']",
shadowFocusTargetSelector: "label[data-value='1']",
});
});

describe("focuses the value-matching star when the label is clicked", () => {
labelable("<calcite-rating value='3'></calcite-rating>", {
shadowFocusTargetSelector: "input[value='3']",
shadowFocusTargetSelector: "label[data-value='3']",
});
});

Expand All @@ -65,7 +66,6 @@ describe("calcite-rating", () => {
await page.setContent("<calcite-rating></calcite-rating>");
const icons = await page.findAll("calcite-rating >>> .icon");
const labels = await page.findAll("calcite-rating >>> .star");
const focusedEl = await page.findAll("calcite-rating >>> .star.focused");
const hoveredEl = await page.findAll("calcite-rating >>> .star.hovered");
const selectedEl = await page.findAll("calcite-rating >>> .star.selected");
const element = await page.find("calcite-rating");
Expand All @@ -74,7 +74,7 @@ describe("calcite-rating", () => {
expect(await page.find("calcite-rating >>> .fraction")).toBeNull();
expect(await page.find("calcite-rating >>> .partial")).toBeNull();
expect(hoveredEl.length).toBe(0);
expect(focusedEl.length).toBe(0);
expect(await isElementFocused(page, "calcite-rating")).toBe(false);
expect(selectedEl.length).toBe(0);
expect(element).toEqualAttribute("value", "0");
expect(changeEvent).toHaveReceivedEventTimes(0);
Expand Down Expand Up @@ -105,7 +105,7 @@ describe("calcite-rating", () => {
expect(labels[3]).not.toHaveClass("partial");
expect(labels[4]).not.toHaveClass("partial");
expect(hoveredEl.length).toBe(0);
expect(focusedEl.length).toBe(0);
expect(await isElementFocused(page, "calcite-rating")).toBe(false);
expect(selectedEl.length).toBe(0);
});

Expand All @@ -114,7 +114,6 @@ describe("calcite-rating", () => {
await page.setContent("<calcite-rating average=3.4></calcite-rating>");
const icons = await page.findAll("calcite-rating >>> .icon");
const labels = await page.findAll("calcite-rating >>> .star");
const focusedEl = await page.findAll("calcite-rating >>> .star.focused");
const hoveredEl = await page.findAll("calcite-rating >>> .star.hovered");
const selectedEl = await page.findAll("calcite-rating >>> .star.selected");
const element = await page.find("calcite-rating");
Expand All @@ -123,7 +122,7 @@ describe("calcite-rating", () => {
expect(await page.find("calcite-rating >>> .fraction")).not.toBeNull();
expect(await page.find("calcite-rating >>> .partial")).not.toBeNull();
expect(hoveredEl.length).toBe(0);
expect(focusedEl.length).toBe(0);
expect(await isElementFocused(page, "calcite-rating")).toBe(false);
expect(selectedEl.length).toBe(0);
expect(element).toEqualAttribute("value", "0");
expect(changeEvent).toHaveReceivedEventTimes(0);
Expand Down Expand Up @@ -162,7 +161,6 @@ describe("calcite-rating", () => {
await page.setContent("<calcite-rating value=4></calcite-rating>");
const icons = await page.findAll("calcite-rating >>> .icon");
const labels = await page.findAll("calcite-rating >>> .star");
const focusedEl = await page.findAll("calcite-rating >>> .star.focused");
const hoveredEl = await page.findAll("calcite-rating >>> .star.hovered");
const selectedEl = await page.findAll("calcite-rating >>> .star.selected");
const element = await page.find("calcite-rating");
Expand All @@ -171,7 +169,7 @@ describe("calcite-rating", () => {
expect(await page.find("calcite-rating >>> .fraction")).toBeNull();
expect(await page.find("calcite-rating >>> .partial")).toBeNull();
expect(hoveredEl.length).toBe(0);
expect(focusedEl.length).toBe(0);
expect(await isElementFocused(page, "calcite-rating")).toBe(false);
expect(selectedEl.length).toBe(4);
expect(element).toEqualAttribute("value", "4");
expect(changeEvent).toHaveReceivedEventTimes(0);
Expand Down Expand Up @@ -209,7 +207,6 @@ describe("calcite-rating", () => {
await page.setContent("<calcite-rating value=3 average=4.2></calcite-rating>");
const icons = await page.findAll("calcite-rating >>> .icon");
const labels = await page.findAll("calcite-rating >>> .star");
const focusedEl = await page.findAll("calcite-rating >>> .star.focused");
const hoveredEl = await page.findAll("calcite-rating >>> .star.hovered");
const selectedEl = await page.findAll("calcite-rating >>> .star.selected");
const element = await page.find("calcite-rating");
Expand All @@ -218,7 +215,7 @@ describe("calcite-rating", () => {
expect(await page.find("calcite-rating >>> .fraction")).toBeNull();
expect(await page.find("calcite-rating >>> .partial")).toBeNull();
expect(hoveredEl.length).toBe(0);
expect(focusedEl.length).toBe(0);
expect(await isElementFocused(page, "calcite-rating")).toBe(false);
expect(selectedEl.length).toBe(3);
expect(element).toEqualAttribute("value", "3");
expect(changeEvent).toHaveReceivedEventTimes(0);
Expand Down Expand Up @@ -362,14 +359,14 @@ describe("calcite-rating", () => {
await labels[2].click();
await page.waitForChanges();

const focusedEl = await page.findAll("calcite-rating >>> .star.focused");
const hoveredEl = await page.findAll("calcite-rating >>> .star.hovered");
const selectedEl = await page.findAll("calcite-rating >>> .star.selected");
const focusedElId = labels[2].getAttribute("for");

expect(await page.find("calcite-rating >>> .fraction")).toBeNull();
expect(await page.find("calcite-rating >>> .partial")).toBeNull();
expect(hoveredEl.length).toBe(3);
expect(focusedEl.length).toBe(0);
expect(await isElementFocused(page, `[for=${focusedElId}]`, { shadowed: true })).toBe(true);
expect(selectedEl.length).toBe(3);
expect(element).toEqualAttribute("value", "3");
expect(changeEvent).toHaveReceivedEventTimes(1);
Expand Down Expand Up @@ -413,14 +410,13 @@ describe("calcite-rating", () => {
await labels[2].hover();
await page.waitForChanges();

const focusedEl = await page.findAll("calcite-rating >>> .star.focused");
const hoveredEl = await page.findAll("calcite-rating >>> .star.hovered");
const selectedEl = await page.findAll("calcite-rating >>> .star.selected");

expect(await page.find("calcite-rating >>> .fraction")).toBeNull();
expect(await page.find("calcite-rating >>> .partial")).toBeNull();
expect(hoveredEl.length).toBe(3);
expect(focusedEl.length).toBe(0);
expect(await isElementFocused(page, "calcite-rating")).toBe(false);
expect(selectedEl.length).toBe(0);
expect(element).toEqualAttribute("value", "0");
expect(changeEvent).toHaveReceivedEventTimes(0);
Expand Down Expand Up @@ -462,11 +458,10 @@ describe("calcite-rating", () => {
await labels[3].hover();
await page.waitForChanges();

const focusedEl = await page.findAll("calcite-rating >>> .star.focused");
const hoveredEl = await page.findAll("calcite-rating >>> .star.hovered");
const selectedEl = await page.findAll("calcite-rating >>> .star.selected");

expect(focusedEl.length).toEqual(0);
expect(await isElementFocused(page, "calcite-rating")).toBe(false);
expect(hoveredEl.length).toEqual(4);
expect(selectedEl.length).toEqual(3);
expect(icons[0]).toEqualAttribute("icon", "star-f");
Expand Down Expand Up @@ -504,9 +499,16 @@ describe("calcite-rating", () => {
const changeEvent = await element.spyOnEvent("calciteRatingChange");

await ratingItem1.click();
await page.waitForChanges();

const hoveredEl = await page.findAll("calcite-rating >>> .star.hovered");
anveshmekala marked this conversation as resolved.
Show resolved Hide resolved
const selectedEl = await page.findAll("calcite-rating >>> .star.selected");

expect(element).toEqualAttribute("value", "4");
expect(changeEvent).toHaveReceivedEventTimes(0);
expect(hoveredEl.length).toBe(0);
expect(await isElementFocused(page, "calcite-rating")).toBe(false);
expect(selectedEl.length).toBe(4);
});

it("should reset the rating when the current value is equal to the value of the clicked input", async () => {
Expand All @@ -519,8 +521,15 @@ describe("calcite-rating", () => {
await labels[2].click();
await page.waitForChanges();

const hoveredEl = await page.findAll("calcite-rating >>> .star.hovered");
const selectedEl = await page.findAll("calcite-rating >>> .star.selected");
const focusedElId = labels[2].getAttribute("for");

expect(await element.getProperty("value")).toBe(0);
expect(changeEvent).toHaveReceivedEventTimes(1);
expect(hoveredEl.length).toBe(3);
expect(await isElementFocused(page, `[for=${focusedElId}]`, { shadowed: true })).toBe(true);
Copy link
Member

Choose a reason for hiding this comment

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

Not critical, but is it possible to query using the star class via label or label attributes? This would make the test less brittle in case the implementation changes.

expect(selectedEl.length).toBe(0);
});

it("should not allow rating to be cleared/reset when required props is set true", async () => {
Expand All @@ -534,8 +543,15 @@ describe("calcite-rating", () => {
await labels[2].click();
await page.waitForChanges();

const hoveredEl = await page.findAll("calcite-rating >>> .star.hovered");
const selectedEl = await page.findAll("calcite-rating >>> .star.selected");
const focusedElId = labels[2].getAttribute("for");

expect(await element.getProperty("value")).toBe(3);
expect(changeEvent).toHaveReceivedEventTimes(1);
expect(hoveredEl.length).toBe(3);
expect(await isElementFocused(page, `[for=${focusedElId}]`, { shadowed: true })).toBe(true);
expect(selectedEl.length).toBe(3);
});

it("should not allow click interaction when read-only is set", async () => {
Expand All @@ -545,7 +561,15 @@ describe("calcite-rating", () => {
const ratingItem1 = await page.find("calcite-rating >>> .star");

await ratingItem1.click();
await page.waitForChanges();

const hoveredEl = await page.findAll("calcite-rating >>> .star.hovered");
const selectedEl = await page.findAll("calcite-rating >>> .star.selected");

expect(element).toEqualAttribute("value", "4");
expect(hoveredEl.length).toBe(0);
expect(await isElementFocused(page, "calcite-rating")).toBe(false);
expect(selectedEl.length).toBe(4);
});
});

Expand All @@ -558,12 +582,11 @@ describe("calcite-rating", () => {
await page.keyboard.press("Tab");
await page.waitForChanges();

const focusedEl = await page.findAll("calcite-rating >>> .star.focused");
const hoveredEl = await page.findAll("calcite-rating >>> .star.hovered");
const selectedEl = await page.findAll("calcite-rating >>> .star.selected");

expect(hoveredEl.length).toBe(3);
expect(focusedEl.length).toBe(1);
expect(await getFocusedElementProp(page, `tagName`, { shadow: true })).toBe("LABEL");
expect(selectedEl.length).toBe(3);
expect(element).toEqualAttribute("value", "3");
});
Expand All @@ -576,12 +599,11 @@ describe("calcite-rating", () => {
await page.keyboard.press("Tab");
await page.waitForChanges();
await page.waitForTimeout(200);
const focusedEl = await page.findAll("calcite-rating >>> .star.focused");
const hoveredEl = await page.findAll("calcite-rating >>> .star.hovered");
const selectedEl = await page.findAll("calcite-rating >>> .star.selected");

expect(hoveredEl.length).toBe(0);
expect(focusedEl.length).toBe(0);
expect(await getFocusedElementProp(page, "tagName", { shadow: true })).not.toBe("LABEL");
expect(selectedEl.length).toBe(0);
expect(element).toEqualAttribute("value", "0");
});
Expand All @@ -598,12 +620,11 @@ describe("calcite-rating", () => {
await page.keyboard.up("Shift");
await page.waitForChanges();

const focusedEl = await page.findAll("calcite-rating >>> .star.focused");
const hoveredEl = await page.findAll("calcite-rating >>> .star.hovered");
const selectedEl = await page.findAll("calcite-rating >>> .star.selected");

expect(hoveredEl.length).toBe(3);
expect(focusedEl.length).toBe(1);
expect(await getFocusedElementProp(page, `tagName`, { shadow: true })).toBe("LABEL");
expect(selectedEl.length).toBe(3);
expect(element).toEqualAttribute("value", "3");
});
Expand Down Expand Up @@ -663,11 +684,11 @@ describe("calcite-rating", () => {
expect(labels[2]).not.toHaveClass("selected");
expect(labels[3]).not.toHaveClass("selected");
expect(labels[4]).not.toHaveClass("selected");
expect(labels[0]).not.toHaveClass("focused");
expect(labels[1]).toHaveClass("focused");
expect(labels[2]).not.toHaveClass("focused");
expect(labels[3]).not.toHaveClass("focused");
expect(labels[4]).not.toHaveClass("focused");
expect(await isElementFocused(page, `[for=${labels[0].getAttribute("for")}]`, { shadowed: true })).toBe(false);
expect(await isElementFocused(page, `[for=${labels[1].getAttribute("for")}]`, { shadowed: true })).toBe(true);
expect(await isElementFocused(page, `[for=${labels[2].getAttribute("for")}]`, { shadowed: true })).toBe(false);
expect(await isElementFocused(page, `[for=${labels[3].getAttribute("for")}]`, { shadowed: true })).toBe(false);
expect(await isElementFocused(page, `[for=${labels[4].getAttribute("for")}]`, { shadowed: true })).toBe(false);
expect(element).toEqualAttribute("value", "2");
expect(changeEvent).toHaveReceivedEventTimes(3);
});
Expand Down
7 changes: 3 additions & 4 deletions packages/calcite-components/src/components/rating/rating.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,9 @@
flex-direction: column;
cursor: pointer;
color: theme("borderColor.color.input");
}

.focused {
@apply focus-outset;
&:focus {
@apply focus-outset;
}
}

.average,
Expand Down
12 changes: 12 additions & 0 deletions packages/calcite-components/src/components/rating/rating.stories.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,3 +41,15 @@ export const darkModeRTL_TestOnly = (): string => html`
darkModeRTL_TestOnly.parameters = { modes: modesDarkDefault };

export const disabled_TestOnly = (): string => html`<calcite-rating disabled value="3"></calcite-rating>`;

export const Focus_TestOnly = (): string => html` <calcite-rating value="4" required></calcite-rating>
<script>
(async () => {
await customElements.whenDefined("calcite-rating");
await document.querySelector("calcite-rating").setFocus();
})();
</script>`;

Focus_TestOnly.parameters = {
chromatic: { delay: 500 },
};
Loading