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.
Esri · Aug 3, 2023 · 2f37854 · 2f37854
1 parent 98c870e
commit 2f37854
Show file tree

Hide file tree

Showing 5 changed files with 157 additions and 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
@@ -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(`<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");
+        });
       });
     });
   });

diff --git 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
@@ -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
@@ -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
@@ -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;
+}