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(color-picker): draw slider thumbs within bounds #7398

Merged
merged 5 commits into from
Aug 3, 2023
+157 −89
Merged

fix(color-picker): draw slider thumbs within bounds #7398

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c728a02
fix(color-picker): draw slider thumbs within bounds
jcfranco Jul 31, 2023
913adea
merge main
jcfranco Aug 1, 2023
ef519e0
tidy up
jcfranco Aug 2, 2023
a926dc3
map range only if x is within the edge of the slider
jcfranco Aug 2, 2023
8fd0c57
fix test
jcfranco Aug 3, 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
157 changes: 81 additions & 76 deletions packages/calcite-components/src/components/color-picker/color-picker.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -519,12 +519,12 @@ describe("calcite-color-picker", () => {
const expectedColorSamples = [
"#ff0000",
"#ffd900",
"#48ff00",
"#00ff91",
"#4cff00",
"#00ff8c",
"#0095ff",
"#4800ff",
"#ff00dd",
"#ff0004",
"#4400ff",
"#ff00e1",
"#ff0008",
];

for (let i = 0; i < expectedColorSamples.length; i++) {
Expand Down Expand Up @@ -678,7 +678,7 @@ describe("calcite-color-picker", () => {
[hueScopeX, hueScopeY] = await getElementXY(page, "calcite-color-picker", `.${CSS.hueScope}`);
[hueScopeCenterX, hueScopeCenterY] = getScopeCenter(hueScopeX, hueScopeY);

expect(hueScopeCenterX).toBe(hueSliderX);
expect(hueScopeCenterX).toBe(hueSliderX + DIMENSIONS.m.thumb.radius);

await page.mouse.move(hueScopeCenterX, hueScopeCenterY);
await page.mouse.down();
Expand All @@ -689,7 +689,7 @@ describe("calcite-color-picker", () => {
[hueScopeX] = await getElementXY(page, "calcite-color-picker", `.${CSS.hueScope}`);
[hueScopeCenterX] = getScopeCenter(hueScopeX, hueScopeY);

expect(hueScopeCenterX).toBe(hueSliderX + DIMENSIONS.m.slider.width - 1);
expect(hueScopeCenterX).toBe(hueSliderX + DIMENSIONS.m.slider.width - DIMENSIONS.m.thumb.radius);
});

describe("unsupported value handling", () => {
Expand Down Expand Up @@ -2197,23 +2197,23 @@ describe("calcite-color-picker", () => {

const getScopeLeftOffset = async () => parseFloat((await scope.getComputedStyle()).left);

expect(await getScopeLeftOffset()).toBe(-0.5);
expect(await getScopeLeftOffset()).toBeCloseTo(DIMENSIONS.m.thumb.radius - 0.5, 0);

await nudgeAQuarterOfSlider();
expect(await getScopeLeftOffset()).toBe(67.5);
expect(await getScopeLeftOffset()).toBeCloseTo(71, 0);

await nudgeAQuarterOfSlider();
expect(await getScopeLeftOffset()).toBe(135.5);
expect(await getScopeLeftOffset()).toBeCloseTo(133, 0);

await nudgeAQuarterOfSlider();
// hue wraps around, so we nudge it back to assert position at the edge
await scope.press("ArrowLeft");
expect(await getScopeLeftOffset()).toBeLessThanOrEqual(203.5);
expect(await getScopeLeftOffset()).toBeGreaterThan(202.5);
expect(await getScopeLeftOffset()).toBeLessThanOrEqual(194);
expect(await getScopeLeftOffset()).toBeGreaterThan(193);

// nudge it to wrap around
await scope.press("ArrowRight");
expect(await getScopeLeftOffset()).toBe(-0.5);
expect(await getScopeLeftOffset()).toBeCloseTo(DIMENSIONS.m.thumb.radius - 0.5, 0);
});

it("allows editing hue slider via keyboard", async () => {
Expand Down Expand Up @@ -2245,84 +2245,89 @@ describe("calcite-color-picker", () => {

expect(await hueSliderScope.getComputedStyle()).toMatchObject({
top: "9.5px",
left: "-0.5px",
left: `${DIMENSIONS.m.thumb.radius - 0.5}px`,
});
});
});
describe("mouse", () => {
const scopeSizeOffset = 0.8;
it("should update value when color field scope is moved", async () => {
const page = await newE2EPage();
await page.setContent(`<calcite-color-picker ></calcite-color-picker>`);
const colorPicker = await page.find("calcite-color-picker");

const [colorFieldScopeX, colorFieldScopeY] = await getElementXY(
page,
"calcite-color-picker",
`.${CSS.colorFieldScope}`
);
const value = await colorPicker.getProperty("value");
describe("mouse", () => {
const scopeSizeOffset = 0.8;
it("should update value when color field scope is moved", async () => {
const page = await newE2EPage();
await page.setContent(`<calcite-color-picker ></calcite-color-picker>`);
const colorPicker = await page.find("calcite-color-picker");

const [colorFieldScopeX, colorFieldScopeY] = await getElementXY(
page,
"calcite-color-picker",
`.${CSS.colorFieldScope}`
);
const value = await colorPicker.getProperty("value");

await page.mouse.move(colorFieldScopeX, colorFieldScopeY + scopeSizeOffset);
await page.mouse.down();
await page.mouse.up();
await page.waitForChanges();
expect(await colorPicker.getProperty("value")).not.toBe(value);
});
await page.mouse.move(colorFieldScopeX, colorFieldScopeY + scopeSizeOffset);
await page.mouse.down();
await page.mouse.up();
await page.waitForChanges();
expect(await colorPicker.getProperty("value")).not.toBe(value);
});

it("should update value when hue scope is moved", async () => {
const page = await newE2EPage();
await page.setContent(`<calcite-color-picker ></calcite-color-picker>`);
const colorPicker = await page.find("calcite-color-picker");
it("should update value when hue scope is moved", async () => {
const page = await newE2EPage();
await page.setContent(`<calcite-color-picker></calcite-color-picker>`);
const colorPicker = await page.find("calcite-color-picker");

const [hueScopeX, hueScopeY] = await getElementXY(page, "calcite-color-picker", `.${CSS.hueScope}`);
const value = await colorPicker.getProperty("value");
const [hueScopeX, hueScopeY] = await getElementXY(page, "calcite-color-picker", `.${CSS.hueScope}`);
const value = await colorPicker.getProperty("value");

await page.mouse.move(hueScopeX + scopeSizeOffset, hueScopeY);
await page.mouse.down();
await page.mouse.up();
await page.waitForChanges();
expect(await colorPicker.getProperty("value")).not.toBe(value);
});
await page.mouse.move(hueScopeX + scopeSizeOffset, hueScopeY);
await page.mouse.down();
await page.mouse.up();
await page.waitForChanges();
expect(await colorPicker.getProperty("value")).not.toBe(value);
});

it("should update value when opacity scope is moved", async () => {
const page = await newE2EPage();
await page.setContent(`<calcite-color-picker alpha-channel></calcite-color-picker>`);
const [opacityScopeX, opacityScopeY] = await getElementXY(page, "calcite-color-picker", `.${CSS.opacityScope}`);
const colorPicker = await page.find("calcite-color-picker");
const value = await colorPicker.getProperty("value");

await page.mouse.move(opacityScopeX - 2, opacityScopeY);
await page.mouse.down();
await page.mouse.up();
await page.waitForChanges();
expect(await colorPicker.getProperty("value")).not.toBe(value);
it("should update value when opacity scope is moved", async () => {
const page = await newE2EPage();
await page.setContent(`<calcite-color-picker alpha-channel></calcite-color-picker>`);
const [opacityScopeX, opacityScopeY] = await getElementXY(
page,
"calcite-color-picker",
`.${CSS.opacityScope}`
);
const colorPicker = await page.find("calcite-color-picker");
const value = await colorPicker.getProperty("value");

await page.mouse.move(opacityScopeX - 2, opacityScopeY);
await page.mouse.down();
await page.mouse.up();
await page.waitForChanges();
expect(await colorPicker.getProperty("value")).not.toBe(value);
});
});
});

describe("alpha channel", () => {
it("allows editing alpha value via keyboard", async () => {
const page = await newE2EPage();
await page.setContent(`<calcite-color-picker alpha-channel value="#ffffffff"></calcite-color-picker>`);
describe("alpha channel", () => {
it("allows editing alpha value via keyboard", async () => {
const page = await newE2EPage();
await page.setContent(`<calcite-color-picker alpha-channel value="#ffffffff"></calcite-color-picker>`);

const picker = await page.find("calcite-color-picker");
const scope = await page.find(`calcite-color-picker >>> .${CSS.opacityScope}`);
const picker = await page.find("calcite-color-picker");
const scope = await page.find(`calcite-color-picker >>> .${CSS.opacityScope}`);

await scope.press("ArrowDown");
expect(await picker.getProperty("value")).toBe("#fffffffc");
await scope.press("ArrowDown");
expect(await picker.getProperty("value")).toBe("#fffffffa");
await scope.press("ArrowDown");
expect(await picker.getProperty("value")).toBe("#fffffff7");
await scope.press("ArrowDown");
expect(await picker.getProperty("value")).toBe("#fffffffc");
await scope.press("ArrowDown");
expect(await picker.getProperty("value")).toBe("#fffffffa");
await scope.press("ArrowDown");
expect(await picker.getProperty("value")).toBe("#fffffff7");

await scope.press("ArrowUp");
expect(await picker.getProperty("value")).toBe("#fffffffa");
await scope.press("ArrowUp");
expect(await picker.getProperty("value")).toBe("#fffffffa");

await scope.press("ArrowRight");
expect(await picker.getProperty("value")).toBe("#fffffffc");
await scope.press("ArrowRight");
expect(await picker.getProperty("value")).toBe("#fffffffc");

await scope.press("ArrowLeft");
expect(await picker.getProperty("value")).toBe("#fffffffa");
await scope.press("ArrowLeft");
expect(await picker.getProperty("value")).toBe("#fffffffa");
});
});
});
});
Expand Down
44 changes: 32 additions & 12 deletions packages/calcite-components/src/components/color-picker/color-picker.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {
DEFAULT_STORAGE_KEY_PREFIX,
DIMENSIONS,
HSV_LIMITS,
HUE_LIMIT_CONSTRAINED,
OPACITY_LIMITS,
RGB_LIMITS,
SCOPE_SIZE,
Expand Down Expand Up @@ -63,7 +64,7 @@ import {
LocalizedComponent,
NumberingSystem,
} from "../../utils/locale";
import { clamp } from "../../utils/math";
import { clamp, closeToRangeEdge, remap } from "../../utils/math";
import {
connectMessages,
disconnectMessages,
Expand Down Expand Up @@ -619,7 +620,7 @@ export class ColorPicker
} else if (clientX < bounds.x) {
samplingX = 0;
} else {
samplingX = bounds.width - 1;
samplingX = bounds.width;
}

if (clientY < bounds.y + bounds.height && clientY > bounds.y) {
Expand Down Expand Up @@ -1095,7 +1096,7 @@ export class ColorPicker
slider: { width },
},
} = this;
const hue = (360 / width) * x;
const hue = (HUE_LIMIT_CONSTRAINED / width) * x;

this.internalColorSet(this.baseColorFieldColor.hue(hue), false);
}
Expand Down Expand Up @@ -1385,7 +1386,6 @@ export class ColorPicker
const startAngle = 0;
const endAngle = 2 * Math.PI;
const outlineWidth = 1;
radius = radius - outlineWidth;

context.beginPath();
context.arc(x, y, radius, startAngle, endAngle);
Expand All @@ -1412,19 +1412,20 @@ export class ColorPicker

const {
dimensions: {
slider: { height, width },
slider: { width },
thumb: { radius },
},
} = this;

const x = hsvColor.hue() / (360 / width);
const y = radius - height / 2 + height / 2;
const x = hsvColor.hue() / (HUE_LIMIT_CONSTRAINED / width);
const y = radius;
const sliderBoundX = this.getSliderBoundX(x, width, radius);

requestAnimationFrame(() => {
this.hueScopeLeft = x;
this.hueScopeLeft = sliderBoundX;
});

this.drawThumb(this.hueSliderRenderingContext, radius, x, y, hsvColor);
this.drawThumb(this.hueSliderRenderingContext, radius, sliderBoundX, y, hsvColor);
}

private drawHueSlider(): void {
Expand All @@ -1441,7 +1442,15 @@ export class ColorPicker

const gradient = context.createLinearGradient(0, 0, width, 0);

const hueSliderColorStopKeywords = ["red", "yellow", "lime", "cyan", "blue", "magenta", "red"];
const hueSliderColorStopKeywords = [
"red",
"yellow",
"lime",
"cyan",
"blue",
"magenta",
"#ff0004" /* 1 unit less than #ff0 to avoid duplicate values within range */,
];

const offset = 1 / (hueSliderColorStopKeywords.length - 1);
let currentOffset = 0;
Expand Down Expand Up @@ -1565,12 +1574,23 @@ export class ColorPicker

const x = alphaToOpacity(hsvColor.alpha()) / (OPACITY_LIMITS.max / width);
const y = radius;
const sliderBoundX = this.getSliderBoundX(x, width, radius);

requestAnimationFrame(() => {
this.opacityScopeLeft = x;
this.opacityScopeLeft = sliderBoundX;
});

this.drawThumb(this.opacitySliderRenderingContext, radius, x, y, hsvColor);
this.drawThumb(this.opacitySliderRenderingContext, radius, sliderBoundX, y, hsvColor);
}

private getSliderBoundX(x: number, width: number, radius: number): number {
const closeToEdge = closeToRangeEdge(x, width, radius);

return closeToEdge === 0
? x
: closeToEdge === -1
? remap(x, 0, width, radius, radius * 2)
: remap(x, 0, width, width - radius * 2, width - radius);
}

private storeOpacityScope = (node: HTMLDivElement): void => {
Expand Down
3 changes: 3 additions & 0 deletions packages/calcite-components/src/components/color-picker/resources.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ export const HSV_LIMITS = {
v: 100,
};

// 0 and 360 represent the same value, so we limit the hue to 359
export const HUE_LIMIT_CONSTRAINED = HSV_LIMITS.h - 1;

export const OPACITY_LIMITS = {
min: 0,
max: 100,
Expand Down
26 changes: 25 additions & 1 deletion packages/calcite-components/src/utils/math.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { clamp, decimalPlaces } from "./math";
import { clamp, closeToRangeEdge, decimalPlaces, remap } from "./math";

describe("clamp", () => {
it("clamps numbers within min/max", () => {
Expand All @@ -14,3 +14,27 @@ describe("decimalPlaces", () => {
expect(decimalPlaces(123.123)).toBe(3);
});
});

describe("remap", () => {
it("remaps numbers", () => {
expect(remap(5, 0, 10, 0, 100)).toBe(50);
expect(remap(0, -100, 100, 0, 100)).toBe(50);
expect(remap(0.5, 0, 1, 0, 100)).toBe(50);
});
});

describe("closeToRangeEdge", () => {
it("returns -1 if close to lower edge", () => {
expect(closeToRangeEdge(0, 100, 10)).toBe(-1);
expect(closeToRangeEdge(9, 100, 10)).toBe(-1);
});

it("returns 1 if close to upper edge", () => {
expect(closeToRangeEdge(100, 100, 10)).toBe(1);
expect(closeToRangeEdge(91, 100, 10)).toBe(1);
});

it("returns 0 if not close to edge", () => {
expect(closeToRangeEdge(50, 100, 10)).toBe(0);
});
});
16 changes: 16 additions & 0 deletions packages/calcite-components/src/utils/math.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,19 @@ export const decimalPlaces = (value: number): number => {
(match[2] ? +match[2] : 0)
);
};

export function remap(value: number, fromMin: number, fromMax: number, toMin: number, toMax: number): number {
return ((value - fromMin) * (toMax - toMin)) / (fromMax - fromMin) + toMin;
}

/**
* Helper to determine if a value is close to the edge of a range within a threshold.
*
* @param value
* @param range
* @param threshold
* @returns -1 if close to lower edge, 1 if close to upper edge, 0 otherwise.
*/
export function closeToRangeEdge(value: number, range: number, threshold: number): number {
return value < threshold ? -1 : value > range - threshold ? 1 : 0;
}