From 2f378548dda9e91719b726a77ab6893e562a20ce Mon Sep 17 00:00:00 2001 From: JC Franco Date: Wed, 2 Aug 2023 19:34:53 -0700 Subject: [PATCH] fix(color-picker): draw slider thumbs within bounds (#7398) **Related Issue:** #7005 ## Summary This addresses slider thumbs appearing cut off. This also ensures the max hue slider value doesn't overlap with 0 and adds a util to map a value between ranges. --- .../color-picker/color-picker.e2e.ts | 157 +++++++++--------- .../components/color-picker/color-picker.tsx | 44 +++-- .../src/components/color-picker/resources.ts | 3 + .../calcite-components/src/utils/math.spec.ts | 26 ++- packages/calcite-components/src/utils/math.ts | 16 ++ 5 files changed, 157 insertions(+), 89 deletions(-) diff --git a/packages/calcite-components/src/components/color-picker/color-picker.e2e.ts b/packages/calcite-components/src/components/color-picker/color-picker.e2e.ts index 0e10d672b09..9bf305b8003 100644 --- a/packages/calcite-components/src/components/color-picker/color-picker.e2e.ts +++ b/packages/calcite-components/src/components/color-picker/color-picker.e2e.ts @@ -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++) { @@ -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(); @@ -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", () => { @@ -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(67.5, 0); await nudgeAQuarterOfSlider(); - expect(await getScopeLeftOffset()).toBe(135.5); + expect(await getScopeLeftOffset()).toBeCloseTo(135.5, 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(193.5); + expect(await getScopeLeftOffset()).toBeGreaterThan(189.5); // 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 () => { @@ -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(``); - 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(``); + 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(``); - 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(``); + 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(``); - 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(``); + 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(``); + describe("alpha channel", () => { + it("allows editing alpha value via keyboard", async () => { + const page = await newE2EPage(); + await page.setContent(``); - 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"); + }); }); }); }); diff --git a/packages/calcite-components/src/components/color-picker/color-picker.tsx b/packages/calcite-components/src/components/color-picker/color-picker.tsx index 1bcbbce0b24..f39f75607a9 100644 --- a/packages/calcite-components/src/components/color-picker/color-picker.tsx +++ b/packages/calcite-components/src/components/color-picker/color-picker.tsx @@ -23,6 +23,7 @@ import { DEFAULT_STORAGE_KEY_PREFIX, DIMENSIONS, HSV_LIMITS, + HUE_LIMIT_CONSTRAINED, OPACITY_LIMITS, RGB_LIMITS, SCOPE_SIZE, @@ -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, @@ -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) { @@ -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); } @@ -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); @@ -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 { @@ -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; @@ -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 => { diff --git a/packages/calcite-components/src/components/color-picker/resources.ts b/packages/calcite-components/src/components/color-picker/resources.ts index 51a4e7357be..7048ecbc433 100644 --- a/packages/calcite-components/src/components/color-picker/resources.ts +++ b/packages/calcite-components/src/components/color-picker/resources.ts @@ -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, diff --git a/packages/calcite-components/src/utils/math.spec.ts b/packages/calcite-components/src/utils/math.spec.ts index be7eee4b27a..2bbdd9feb28 100644 --- a/packages/calcite-components/src/utils/math.spec.ts +++ b/packages/calcite-components/src/utils/math.spec.ts @@ -1,4 +1,4 @@ -import { clamp, decimalPlaces } from "./math"; +import { clamp, closeToRangeEdge, decimalPlaces, remap } from "./math"; describe("clamp", () => { it("clamps numbers within min/max", () => { @@ -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); + }); +}); diff --git a/packages/calcite-components/src/utils/math.ts b/packages/calcite-components/src/utils/math.ts index 3bc251556bc..8698bb5da96 100644 --- a/packages/calcite-components/src/utils/math.ts +++ b/packages/calcite-components/src/utils/math.ts @@ -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; +}