diff --git a/.changeset/ype-4494-non-palette-highlight-paint-clear.md b/.changeset/ype-4494-non-palette-highlight-paint-clear.md new file mode 100644 index 00000000..9e03bb35 --- /dev/null +++ b/.changeset/ype-4494-non-palette-highlight-paint-clear.md @@ -0,0 +1,7 @@ +--- +'@youversion/platform-react-ui': patch +--- + +Paint and clear non-palette highlight colors in the web Bible reader. + +Apply stays limited to the five SDK palette colors. Valid non-palette API hex now paints on the reader and appears in the remove tray at its exact color with the checkmark. Invalid hex is dropped from paint and the tray. Clear keeps the existing ANY rule for mixed selections. Dark fills (e.g. black) flip verse text and the remove checkmark to white in light mode so they stay legible. diff --git a/docs/adr/YPE-3705-controlled-mode.md b/docs/adr/YPE-3705-controlled-mode.md index 5c545916..332a40c2 100644 --- a/docs/adr/YPE-3705-controlled-mode.md +++ b/docs/adr/YPE-3705-controlled-mode.md @@ -82,6 +82,11 @@ path) via a `controlled` input, latched by `Root` at first mount: ## Out of scope -Notes, custom colors, >5 colors (epic fast-follow); self-contained notification -events; controlled *content* (the reader keeps fetching passages/books/versions); -offline/write-queue concerns (native-side, YPE-3717). +Notes; **creating** new custom apply colors (apply stays the five SDK palette +colors — YPE-4494); self-contained notification events; controlled *content* +(the reader keeps fetching passages/books/versions); offline/write-queue +concerns (native-side, YPE-3717). + +Valid non-palette hex that already exists on the account **does** paint and can +be cleared via the remove tray (YPE-4494) — that is not a custom-color apply +path. diff --git a/docs/highlight-flow-statechart.md b/docs/highlight-flow-statechart.md index 77264641..28ce748c 100644 --- a/docs/highlight-flow-statechart.md +++ b/docs/highlight-flow-statechart.md @@ -14,7 +14,9 @@ to explore it interactively. - **`booting`** → routes to `disabled` or `enabled` from the initial input. - **`disabled`** — the flag is off **or** no auth provider is mounted. Fully - inert: no fetch, no writes, no dialogs. A color tap resolves to `noop`. + inert: no fetch, no writes, no dialogs. A color tap resolves to `noop` + (including empty selection and non-palette apply colors — apply stays + palette-only; see YPE-4494). - **`enabled`** — a parallel state with two independent regions: - **`flow`** — the auth / dialog flow. - `resuming` consumes the data-exchange return exactly once, then routes on @@ -26,8 +28,10 @@ to explore it interactively. processing one queued operation at a time so a DELETE can never overtake an in-flight POST for the same verse. -`TAP_COLOR` forks in `flow`: authorized (`applied`) → optimistic write; signed -out → `signInDialog`; signed in without the permission → `permissionDialog`. +`TAP_COLOR` forks in `flow`: authorized palette color (`applied`) → optimistic +write; signed out → `signInDialog`; signed in without the permission → +`permissionDialog`. Non-palette colors, empty verse selection, and invalid hex +resolve to `noop` at the machine boundary (YPE-4494). Both dialog paths stash a pending highlight (10-min `sessionStorage` TTL) so the intent survives the full-page redirect and resumes on a granted return. @@ -50,6 +54,7 @@ stateDiagram-v2 state disabled { note right of disabled TAP_COLOR → outcome "noop" + (inert, non-palette, empty selection) no fetch / writes / dialogs end note } diff --git a/packages/ui/src/components/bible-reader-controlled.test.tsx b/packages/ui/src/components/bible-reader-controlled.test.tsx index 52bc501a..ffb2eef1 100644 --- a/packages/ui/src/components/bible-reader-controlled.test.tsx +++ b/packages/ui/src/components/bible-reader-controlled.test.tsx @@ -346,15 +346,17 @@ describe('BibleReader controlled mode - pure projection', () => { } }); - it('ignores entries with colors outside the built-in swatches (no un-removable paint)', () => { + it('paints valid non-palette colors and drops invalid hex', () => { const highlights: Highlight[] = [ { version_id: 111, passage_id: 'JHN.1.1', color: 'abcdef' }, - { version_id: 111, passage_id: 'JHN.1.2', color: YELLOW }, + { version_id: 111, passage_id: 'JHN.1.2', color: 'gggggg' }, + { version_id: 111, passage_id: 'JHN.1.3', color: YELLOW }, ]; const { container } = renderReader({ highlights }); - expect(getVerseEl(container, 1).style.backgroundColor).toBe(''); - expect(getVerseEl(container, 2).style.backgroundColor).toBe(fillFor(YELLOW)); + expect(getVerseEl(container, 1).style.backgroundColor).toBe(fillFor('abcdef')); + expect(getVerseEl(container, 2).style.backgroundColor).toBe(''); + expect(getVerseEl(container, 3).style.backgroundColor).toBe(fillFor(YELLOW)); }); it('re-projects when the highlights prop changes (host round-trip)', () => { diff --git a/packages/ui/src/components/bible-reader-highlights-machine.test.ts b/packages/ui/src/components/bible-reader-highlights-machine.test.ts index 61d016b9..c110f947 100644 --- a/packages/ui/src/components/bible-reader-highlights-machine.test.ts +++ b/packages/ui/src/components/bible-reader-highlights-machine.test.ts @@ -124,18 +124,18 @@ describe('bibleReaderHighlightsMachine — writeIntent lifecycle', () => { const { ref, refetch } = makeServices({ createHighlight }); const actor = startMachine(ref); - // Write A: apply red to verse 16 (goes in flight). - actor.send({ type: 'TAP_COLOR', color: 'FF0000', verses: [16] }); + // Write A: apply pink to verse 16 (goes in flight). + actor.send({ type: 'TAP_COLOR', color: 'ff95ef', verses: [16] }); await vi.waitFor(() => expect(createHighlight).toHaveBeenCalledTimes(1)); const claimA = actor.getSnapshot().context.writeIntent.get(16); expect(claimA).toBeDefined(); // Write B: re-claim verse 16 with green while A is still in flight (queued). - actor.send({ type: 'TAP_COLOR', color: '00FF00', verses: [16] }); + actor.send({ type: 'TAP_COLOR', color: '5dff79', verses: [16] }); const claimB = actor.getSnapshot().context.writeIntent.get(16); expect(claimB).toBeDefined(); expect(claimB).not.toBe(claimA); - expect(actor.getSnapshot().context.overlay).toEqual({ 16: '00ff00' }); + expect(actor.getSnapshot().context.overlay).toEqual({ 16: '5dff79' }); // A settles: it must not delete B's claim or reconcile verse 16. first.resolve(undefined); @@ -143,7 +143,7 @@ describe('bibleReaderHighlightsMachine — writeIntent lifecycle', () => { const afterA = actor.getSnapshot().context; expect(afterA.writeIntent.get(16)).toBe(claimB); expect(afterA.reconcile.has(16)).toBe(false); - expect(afterA.overlay).toEqual({ 16: '00ff00' }); + expect(afterA.overlay).toEqual({ 16: '5dff79' }); // B settles: it cleans up its own claim and registers its reconcile entry. await vi.waitFor(() => expect(createHighlight).toHaveBeenCalledTimes(2)); @@ -151,7 +151,7 @@ describe('bibleReaderHighlightsMachine — writeIntent lifecycle', () => { await vi.waitFor(() => expect(refetch).toHaveBeenCalledTimes(2)); const afterB = actor.getSnapshot().context; expect(afterB.writeIntent.has(16)).toBe(false); - expect(afterB.reconcile.get(16)).toEqual({ op: 'apply', color: '00ff00' }); + expect(afterB.reconcile.get(16)).toEqual({ op: 'apply', color: '5dff79' }); actor.stop(); }); }); @@ -167,16 +167,16 @@ describe('bibleReaderHighlightsMachine — pending stash on lost permission', () vi.spyOn(console, 'error').mockImplementation(vi.fn()); // Both taps issued before either write settles: A is writing, B is queued. - actor.send({ type: 'TAP_COLOR', color: 'AAAAAA', verses: [1, 2, 3] }); - actor.send({ type: 'TAP_COLOR', color: 'BBBBBB', verses: [4, 5, 6] }); + actor.send({ type: 'TAP_COLOR', color: 'fffe00', verses: [1, 2, 3] }); + actor.send({ type: 'TAP_COLOR', color: '5dff79', verses: [4, 5, 6] }); await vi.waitFor(() => expect(refetch).toHaveBeenCalledTimes(2)); const stash = peekPendingHighlights(); expect(stash).toHaveLength(2); // Verse-level ordering deterministic: first-queued (A) first. - expect(stash[0]).toMatchObject({ verses: [1, 2, 3], color: 'aaaaaa' }); - expect(stash[1]).toMatchObject({ verses: [4, 5, 6], color: 'bbbbbb' }); + expect(stash[0]).toMatchObject({ verses: [1, 2, 3], color: 'fffe00' }); + expect(stash[1]).toMatchObject({ verses: [4, 5, 6], color: '5dff79' }); actor.stop(); }); @@ -192,14 +192,14 @@ describe('bibleReaderHighlightsMachine — pending stash on lost permission', () const actor = startMachine(ref); vi.spyOn(console, 'error').mockImplementation(vi.fn()); - actor.send({ type: 'TAP_COLOR', color: 'AAAAAA', verses: [1, 2, 3] }); - actor.send({ type: 'TAP_COLOR', color: 'BBBBBB', verses: [4, 5, 6] }); + actor.send({ type: 'TAP_COLOR', color: 'fffe00', verses: [1, 2, 3] }); + actor.send({ type: 'TAP_COLOR', color: '5dff79', verses: [4, 5, 6] }); await vi.waitFor(() => expect(refetch).toHaveBeenCalledTimes(2)); const stash = peekPendingHighlights(); expect(stash).toHaveLength(1); - expect(stash[0]).toMatchObject({ verses: [1, 2, 3], color: 'aaaaaa' }); + expect(stash[0]).toMatchObject({ verses: [1, 2, 3], color: 'fffe00' }); // The 5xx write's verses were never stashed. expect(stash.some((entry) => entry.verses.includes(4))).toBe(false); actor.stop(); @@ -212,7 +212,7 @@ describe('bibleReaderHighlightsMachine — pending stash on lost permission', () appendPendingHighlight( { verses: [1, 2, 3], - color: 'aaaaaa', + color: 'fffe00', versionId: 111, book: 'JHN', chapter: '3', @@ -223,7 +223,7 @@ describe('bibleReaderHighlightsMachine — pending stash on lost permission', () appendPendingHighlight( { verses: [4, 5, 6], - color: 'bbbbbb', + color: '5dff79', versionId: 111, book: 'JHN', chapter: '3', @@ -243,12 +243,12 @@ describe('bibleReaderHighlightsMachine — pending stash on lost permission', () expect(passages).toEqual(['JHN.3.1-3', 'JHN.3.4-6']); // Both colors painted in the same-scope overlay. expect(actor.getSnapshot().context.overlay).toEqual({ - 1: 'aaaaaa', - 2: 'aaaaaa', - 3: 'aaaaaa', - 4: 'bbbbbb', - 5: 'bbbbbb', - 6: 'bbbbbb', + 1: 'fffe00', + 2: 'fffe00', + 3: 'fffe00', + 4: '5dff79', + 5: '5dff79', + 6: '5dff79', }); // Pending consumed once resumed. expect(readPendingHighlights()).toEqual([]); diff --git a/packages/ui/src/components/bible-reader-highlights-machine.ts b/packages/ui/src/components/bible-reader-highlights-machine.ts index 8b8143d5..115cfc7b 100644 --- a/packages/ui/src/components/bible-reader-highlights-machine.ts +++ b/packages/ui/src/components/bible-reader-highlights-machine.ts @@ -44,6 +44,7 @@ * leaving the tested apply-convergence behavior untouched. */ import { collapseVerseRuns, formatPassageId, type VerseRun } from '@/lib/usfm-ranges'; +import { isPaletteHighlightColor } from '@/lib/highlight-colors'; import { appendPendingHighlight, clearPendingHighlight, @@ -363,14 +364,20 @@ export const bibleReaderHighlightsMachine = setup({ signedOut: ({ context }) => !context.isAuthenticated, // ── TAP_COLOR fork ── - tapInert: ({ event }) => event.type === 'TAP_COLOR' && event.verses.length === 0, + tapInert: ({ event }) => + event.type === 'TAP_COLOR' && + (event.verses.length === 0 || !isPaletteHighlightColor(event.color)), tapCanWrite: ({ context, event }) => event.type === 'TAP_COLOR' && event.verses.length > 0 && + isPaletteHighlightColor(event.color) && context.isAuthenticated && context.services.current.hasHighlightsPermission(), tapNeedsSignIn: ({ context, event }) => - event.type === 'TAP_COLOR' && event.verses.length > 0 && !context.isAuthenticated, + event.type === 'TAP_COLOR' && + event.verses.length > 0 && + isPaletteHighlightColor(event.color) && + !context.isAuthenticated, // ── resume fork ── // Guards must be pure, so they PEEK (never clear expired/malformed entries); @@ -471,6 +478,7 @@ export const bibleReaderHighlightsMachine = setup({ /** Optimistically paint + claim + enqueue a user apply (TAP_COLOR authorized path). */ startApplyWrite: enqueueActions(({ enqueue, context, event }) => { if (event.type !== 'TAP_COLOR') return; + if (!isPaletteHighlightColor(event.color)) return; const color = event.color.toLowerCase(); const verses = event.verses; const token = {}; @@ -497,6 +505,7 @@ export const bibleReaderHighlightsMachine = setup({ */ stashPendingTap: enqueueActions(({ enqueue, context, event }) => { if (event.type !== 'TAP_COLOR') return; + if (!isPaletteHighlightColor(event.color)) return; stashPendingHighlight({ verses: event.verses, color: event.color.toLowerCase(), @@ -542,6 +551,7 @@ export const bibleReaderHighlightsMachine = setup({ if (pendings.length === 0) return; clearPendingHighlight(); for (const pending of pendings) { + if (!isPaletteHighlightColor(pending.color)) continue; const scope: HighlightScope = { versionId: pending.versionId, book: pending.book, diff --git a/packages/ui/src/components/bible-reader.tsx b/packages/ui/src/components/bible-reader.tsx index 289244a9..fc47c668 100644 --- a/packages/ui/src/components/bible-reader.tsx +++ b/packages/ui/src/components/bible-reader.tsx @@ -238,9 +238,9 @@ export type RootProps = { * "controlled, nothing highlighted"; leaving the prop off means * self-contained. * - * Entries whose color is outside the reader's five built-in swatches are - * ignored — the verse-action popover can only offer removal for its own - * palette, so an unmanageable color must not paint. + * Invalid API hex is dropped from paint. Valid non-palette colors paint and + * appear in the remove tray at their exact hex (YPE-4494). Apply stays + * palette-only. */ highlights?: Highlight[]; /** diff --git a/packages/ui/src/components/use-bible-reader-highlights.test.tsx b/packages/ui/src/components/use-bible-reader-highlights.test.tsx index b789d7e1..8ea70a7f 100644 --- a/packages/ui/src/components/use-bible-reader-highlights.test.tsx +++ b/packages/ui/src/components/use-bible-reader-highlights.test.tsx @@ -195,6 +195,25 @@ describe('useBibleReaderHighlights — fetched highlights', () => { 17: '5dff79', }); }); + + it('drops invalid hex from the fetched server path (parseServerColors)', () => { + mockUseHighlights({ + highlights: makeCollection([ + { version_id: 111, passage_id: 'JHN.3.16', color: 'fffe00' }, + { version_id: 111, passage_id: 'JHN.3.17', color: 'gggggg' }, + { version_id: 111, passage_id: 'JHN.3.18', color: 'aabbcc' }, + ]), + }); + + const { result } = renderHook(() => useBibleReaderHighlights(defaultOptions), { + wrapper: AuthWrapper, + }); + + expect(result.current.highlightedVerses).toEqual({ + 16: 'fffe00', + 18: 'aabbcc', + }); + }); }); describe('useBibleReaderHighlights — apply', () => { @@ -259,6 +278,21 @@ describe('useBibleReaderHighlights — apply', () => { ); consoleError.mockRestore(); }); + + it('rejects non-palette apply colors without writing', () => { + const mocked = mockUseHighlights(); + + const { result } = renderHook(() => useBibleReaderHighlights(defaultOptions), { + wrapper: AuthWrapper, + }); + + act(() => { + expect(result.current.apply('aabbcc', [16])).toBe('noop'); + }); + + expect(result.current.highlightedVerses).toEqual({}); + expect(mocked.createHighlight).not.toHaveBeenCalled(); + }); }); describe('useBibleReaderHighlights — overlay reconciliation (Fix 2)', () => { @@ -458,6 +492,33 @@ describe('useBibleReaderHighlights — remove', () => { expect(mocked.deleteHighlight).not.toHaveBeenCalled(); expect(result.current.highlightedVerses).toEqual({ 16: 'fffe00' }); }); + + it('clears valid non-palette highlights on the selected verses', async () => { + const custom = 'aabbcc'; + const mocked = mockUseHighlights({ + highlights: makeCollection([ + { version_id: 111, passage_id: 'JHN.3.16', color: custom }, + { version_id: 111, passage_id: 'JHN.3.17', color: 'fffe00' }, + ]), + }); + + const { result } = renderHook(() => useBibleReaderHighlights(defaultOptions), { + wrapper: AuthWrapper, + }); + + expect(result.current.highlightedVerses).toEqual({ 16: custom, 17: 'fffe00' }); + + act(() => { + result.current.remove(custom, [16, 17]); + }); + + expect(result.current.highlightedVerses).toEqual({ 17: 'fffe00' }); + + await waitFor(() => { + expect(mocked.deleteHighlight).toHaveBeenCalledTimes(1); + }); + expect(mocked.deleteHighlight).toHaveBeenCalledWith('JHN.3.16', { version_id: 111 }); + }); }); describe('useBibleReaderHighlights — scope changes', () => { @@ -623,6 +684,7 @@ describe('useBibleReaderHighlights — controlled mode (YPE-3705)', () => { ); expect(mocked.createHighlight).not.toHaveBeenCalled(); expect(result.current.highlightedVerses).toEqual({ + 1: 'abcdef', 16: 'fffe00', 17: '5dff79', 18: '5dff79', @@ -686,4 +748,48 @@ describe('useBibleReaderHighlights — controlled mode (YPE-3705)', () => { rerender({ highlights: [] }); expect(result.current.highlightedVerses).toEqual({}); }); + + it('rejects non-palette apply intents in controlled mode', () => { + const onApply = vi.fn(); + + const { result } = renderHook(() => + useBibleReaderHighlights({ + ...defaultOptions, + controlled: { highlights: [], onApply }, + }), + ); + + act(() => { + expect(result.current.apply('aabbcc', [16])).toBe('noop'); + }); + expect(onApply).not.toHaveBeenCalled(); + }); + + it('emits remove intents for valid non-palette colors', () => { + const custom = 'aabbcc'; + const onRemove = vi.fn(); + + const { result } = renderHook(() => + useBibleReaderHighlights({ + ...defaultOptions, + controlled: { + highlights: [{ version_id: 111, passage_id: 'JHN.3.16', color: custom }], + onRemove, + }, + }), + ); + + act(() => { + result.current.remove(custom, [16]); + }); + + expect(onRemove).toHaveBeenCalledWith({ + versionId: 111, + book: 'JHN', + chapter: '3', + verses: [16], + passageIds: ['JHN.3.16'], + color: custom, + }); + }); }); diff --git a/packages/ui/src/components/use-bible-reader-highlights.ts b/packages/ui/src/components/use-bible-reader-highlights.ts index 1542bf22..b50f494e 100644 --- a/packages/ui/src/components/use-bible-reader-highlights.ts +++ b/packages/ui/src/components/use-bible-reader-highlights.ts @@ -2,6 +2,7 @@ import { isHighlightsLive } from '@/lib/feature-flags'; import { deriveHighlightedVerses } from '@/lib/highlight-projection'; +import { isPaletteHighlightColor, normalizeHighlightHex } from '@/lib/highlight-colors'; import type { Highlight } from '@youversion/platform-core'; import { bibleReaderHighlightsMachine, @@ -18,7 +19,6 @@ import { } from '@youversion/platform-react-hooks'; import { useActorRef, useSelector } from '@xstate/react'; import { useContext, useEffect, useMemo, useRef } from 'react'; -import { HIGHLIGHT_COLORS } from './verse-action-popover'; /** * Bridge-safe highlight intent emitted in controlled mode. Structurally @@ -111,8 +111,10 @@ function parseServerColors( for (const highlight of highlights?.data ?? []) { if (highlight.version_id !== versionId) continue; if (!highlight.passage_id.startsWith(versePrefix)) continue; + const normalizedColor = normalizeHighlightHex(highlight.color); + if (normalizedColor === null) continue; const verse = parseInt(highlight.passage_id.slice(versePrefix.length), 10); - if (verse > 0) map[verse] = highlight.color.toLowerCase(); + if (verse > 0) map[verse] = normalizedColor; } return map; } @@ -282,16 +284,8 @@ export function useBibleReaderHighlights({ const machineScope = useSelector(actorRef, (state) => state.context.scope); const highlightedVerses = useMemo(() => { // Controlled: pure projection from the host prop — no overlay, no fetch. - // Colors outside the popover swatches are ignored so un-removable paint - // cannot appear (YPE-3705). if (isControlled) { - return deriveHighlightedVerses( - controlled?.highlights ?? [], - versionId, - book, - chapter, - HIGHLIGHT_COLORS, - ); + return deriveHighlightedVerses(controlled?.highlights ?? [], versionId, book, chapter); } // Gate on `live`: sign-out or flag-off must render nothing this very render, @@ -343,8 +337,9 @@ export function useBibleReaderHighlights({ () => ({ apply: (color, verses) => { const controlledInput = controlledRef.current; + if (verses.length === 0) return 'noop'; + if (!isPaletteHighlightColor(color)) return 'noop'; if (controlledInput !== undefined) { - if (verses.length === 0) return 'noop'; // Emit intent only — no optimistic paint (YPE-3705 ADR). controlledInput.onApply?.( buildHighlightIntent(scope.versionId, scope.book, scope.chapter, color, verses), diff --git a/packages/ui/src/components/verse-action-popover.test.tsx b/packages/ui/src/components/verse-action-popover.test.tsx index 619c03aa..78db0c39 100644 --- a/packages/ui/src/components/verse-action-popover.test.tsx +++ b/packages/ui/src/components/verse-action-popover.test.tsx @@ -7,6 +7,13 @@ import { type HighlightColor, } from './verse-action-popover'; +function fillFor(hex: string): string { + const r = parseInt(hex.slice(0, 2), 16); + const g = parseInt(hex.slice(2, 4), 16); + const b = parseInt(hex.slice(4, 6), 16); + return `rgb(${r}, ${g}, ${b})`; +} + describe('VerseActionPopover', () => { const defaultProps = { open: true, @@ -412,6 +419,22 @@ describe('VerseActionPopover', () => { }); describe('Edge cases', () => { + it('shows a remove swatch for a valid non-palette color at exact hex', () => { + const custom = 'aabbcc'; + render( + , + ); + + const removeButtons = clearButtons(); + expect(removeButtons).toHaveLength(1); + expect(removeButtons[0]!.style.backgroundColor).toBe(fillFor(custom)); + }); + it('should handle empty active highlights', () => { const activeHighlights = new Set(); @@ -591,6 +614,21 @@ describe('VerseActionPopover', () => { const check = screen.getByRole('button', { name: /Clear highlight/ }).querySelector('svg'); expect(check?.getAttribute('class')).toContain('yv:text-(--yv-gray-50)'); }); + + it('uses a white checkmark on a dark remove swatch in light mode', () => { + const custom = '000000'; + render( + , + ); + const check = screen.getByRole('button', { name: /Clear highlight/ }).querySelector('svg'); + expect(check?.getAttribute('class')).toContain('yv:text-white'); + }); }); describe('Viewport width cap + scrollable swatch row', () => { diff --git a/packages/ui/src/components/verse-action-popover.tsx b/packages/ui/src/components/verse-action-popover.tsx index 543de90f..95197a9e 100644 --- a/packages/ui/src/components/verse-action-popover.tsx +++ b/packages/ui/src/components/verse-action-popover.tsx @@ -6,19 +6,13 @@ import { cn } from '../lib/utils'; import { BoxStackIcon } from './icons/box-stack'; import { BoxArrowUpIcon } from './icons/box-arrow-up'; import { CheckIcon } from './icons/check'; -import { hexToRgba, HIGHLIGHT_FILL_OPACITY_DARK } from './verse'; +import { buildVerseActionSwatches } from '@/lib/highlight-colors'; +import { hexToRgba, HIGHLIGHT_FILL_OPACITY_DARK, isDarkHighlightHex } from './verse'; -type Measurable = { getBoundingClientRect: () => DOMRect }; - -/** - * Highlight colors, as 6-digit lowercase hex (no `#`) so they map 1:1 onto the - * API `highlight.color` field (/^[0-9a-f]{6}$/). Order is the canonical apply - * order: yellow, green, blue, orange, pink. Hardcoded to match the YouVersion - * iOS app exactly. - */ -export const HIGHLIGHT_COLORS = ['fffe00', '5dff79', '00d6ff', 'ffc66f', 'ff95ef'] as const; +/** Re-export for back-compat; prefer `@/lib/highlight-colors` for new code. */ +export { HIGHLIGHT_COLORS, type HighlightColor } from '@/lib/highlight-colors'; -export type HighlightColor = (typeof HIGHLIGHT_COLORS)[number]; +type Measurable = { getBoundingClientRect: () => DOMRect }; /** Width, in px, of the fade applied at each overflowing edge of the swatch row. */ const SCROLL_FADE_PX = 20; @@ -124,7 +118,10 @@ function ColorCircle({ color, showRemove, label, onClick, theme }: ColorCirclePr white to stay legible. Tapping it still removes the highlight. */} {showRemove && ( )} @@ -267,21 +264,11 @@ export const VerseActionPopover: FC = ({ // above it (side top). Both place the bar just inside the reader edge. const dockedSide = dockEdge === 'top' ? 'bottom' : 'top'; - const activeColors = HIGHLIGHT_COLORS.filter((c) => activeHighlights.has(c)); - const highlightedVerseCount = selectedVerses.filter((v) => highlightedVerses[v]).length; - const unHighlightedCount = selectedVerses.length - highlightedVerseCount; - const allColorsActive = activeHighlights.size === HIGHLIGHT_COLORS.length; - const showAllApplyColors = - !allColorsActive && (unHighlightedCount > 0 || activeHighlights.size > 1); - const colorsToApply = showAllApplyColors - ? HIGHLIGHT_COLORS - : HIGHLIGHT_COLORS.filter((c) => !activeHighlights.has(c)); - - // Remove (checkmark) circles come first, then apply circles in canonical order. - const colorCircles = [ - ...activeColors.map((color) => ({ color, showRemove: true, key: `${color}-clear` })), - ...colorsToApply.map((color) => ({ color, showRemove: false, key: `${color}-apply` })), - ]; + const colorCircles = buildVerseActionSwatches({ + activeHighlights, + selectedVerses, + highlightedVerses, + }); // Snapshot of everything the Content renders. While open we keep it fresh; the // moment `open` flips false (apply / outside-click) the parent clears the diff --git a/packages/ui/src/components/verse.test.tsx b/packages/ui/src/components/verse.test.tsx index 9ac1cb00..9a52819d 100644 --- a/packages/ui/src/components/verse.test.tsx +++ b/packages/ui/src/components/verse.test.tsx @@ -609,6 +609,39 @@ describe('Verse.Html - Highlight fill (theme-aware, Swift parity)', () => { }); }); + it('paints white verse text over a dark fill in light mode so the words stay legible', async () => { + const { container, rerender } = render( + , + ); + + await waitFor(() => { + const verse = container.querySelector('.yv-v[v="1"]'); + expect(verse).not.toBeNull(); + expect(verse!.style.backgroundColor).toBe('rgb(0, 0, 0)'); + expect(verse!.style.color).toBe('rgb(255, 255, 255)'); + }); + + rerender(); + + await waitFor(() => { + const verse = container.querySelector('.yv-v[v="1"]'); + expect(verse!.style.backgroundColor).toBe(''); + expect(verse!.style.color).toBe(''); + }); + }); + + it('keeps default verse text color over a palette fill in light mode', async () => { + const { container } = render( + , + ); + + await waitFor(() => { + const verse = container.querySelector('.yv-v[v="1"]'); + expect(verse).not.toBeNull(); + expect(verse!.style.color).toBe(''); + }); + }); + it('paints the fill at 0.3 alpha in dark mode', async () => { const { container } = render( , diff --git a/packages/ui/src/components/verse.tsx b/packages/ui/src/components/verse.tsx index dc555587..59551c45 100644 --- a/packages/ui/src/components/verse.tsx +++ b/packages/ui/src/components/verse.tsx @@ -157,6 +157,17 @@ export function hexToRgba(hex: string, alpha: number): string { return `rgba(${r}, ${g}, ${b}, ${alpha})`; } +/** + * Rec. 601 luma. Dark fills (e.g. `000000`) need light text in light mode; + * the five SDK palette colors all sit above this cutoff. + */ +export function isDarkHighlightHex(hex: string): boolean { + const r = parseInt(hex.slice(0, 2), 16); + const g = parseInt(hex.slice(2, 4), 16); + const b = parseInt(hex.slice(4, 6), 16); + return (0.299 * r + 0.587 * g + 0.114 * b) / 255 < 0.5; +} + /** * Extracts clean prose for a verse from the rendered DOM: concatenates every * `.yv-v[v="N"]` wrapper (a verse can span multiple, e.g. poetry) with verse @@ -347,7 +358,14 @@ function BibleTextHtml({ el.classList.toggle('yv-v-selected', selectedVerses.includes(verseNum)); const color = highlightedVerses[verseNum]; const isHighlighted = Boolean(color); - (el as HTMLElement).style.backgroundColor = color ? hexToRgba(color, fillOpacity) : ''; + const node = el as HTMLElement; + node.style.backgroundColor = color ? hexToRgba(color, fillOpacity) : ''; + // Light mode paints fills at full strength. A dark non-palette hex + // (YPE-4494) would sit under the reader's dark body text and go + // illegible, so flip the wrapper to white; labels and footnote icons + // inherit it. Palette fills stay on the default foreground. Dark mode + // already uses light body text, so leave color unset there. + node.style.color = color && !isDark && isDarkHighlightHex(color) ? '#ffffff' : ''; // Over a highlight fill the muted label color clashes with saturated fills, // so the label inherits the verse body text color instead — the reader's // main text color in light mode, white/near-white in dark mode. Setting diff --git a/packages/ui/src/lib/highlight-colors.test.ts b/packages/ui/src/lib/highlight-colors.test.ts new file mode 100644 index 00000000..730ae08b --- /dev/null +++ b/packages/ui/src/lib/highlight-colors.test.ts @@ -0,0 +1,131 @@ +import { describe, expect, it } from 'vitest'; +import { + HIGHLIGHT_COLORS, + buildVerseActionSwatches, + isPaletteHighlightColor, + isValidHighlightHex, + normalizeHighlightHex, +} from './highlight-colors'; + +describe('highlight-colors', () => { + it('isValidHighlightHex accepts 6-digit lowercase and uppercase hex', () => { + expect(isValidHighlightHex('abcdef')).toBe(true); + expect(isValidHighlightHex('ABCDEF')).toBe(true); + expect(isValidHighlightHex('#fffe00')).toBe(true); + }); + + it('isValidHighlightHex rejects invalid API colors', () => { + expect(isValidHighlightHex('gggggg')).toBe(false); + expect(isValidHighlightHex('abc')).toBe(false); + expect(isValidHighlightHex('1234567')).toBe(false); + expect(isValidHighlightHex('')).toBe(false); + }); + + it('normalizeHighlightHex returns lowercase hex for valid input', () => { + expect(normalizeHighlightHex('ABCDEF')).toBe('abcdef'); + expect(normalizeHighlightHex('#FFFE00')).toBe('fffe00'); + }); + + it('normalizeHighlightHex returns null for invalid input', () => { + expect(normalizeHighlightHex('not-a-color')).toBeNull(); + }); + + it('isPaletteHighlightColor recognizes the five SDK palette colors only', () => { + expect(isPaletteHighlightColor('fffe00')).toBe(true); + expect(isPaletteHighlightColor('FFFE00')).toBe(true); + expect(isPaletteHighlightColor('abcdef')).toBe(false); + expect(isPaletteHighlightColor('invalid')).toBe(false); + }); + + it('buildVerseActionSwatches includes remove swatches for valid non-palette colors at exact hex', () => { + const custom = 'aabbcc'; + const swatches = buildVerseActionSwatches({ + activeHighlights: new Set([custom]), + selectedVerses: [1], + highlightedVerses: { 1: custom }, + }); + + const remove = swatches.filter((swatch) => swatch.showRemove); + expect(remove).toEqual([{ color: custom, showRemove: true, key: `${custom}-clear` }]); + expect(swatches.filter((swatch) => !swatch.showRemove).map((swatch) => swatch.color)).toEqual( + HIGHLIGHT_COLORS, + ); + }); + + it('buildVerseActionSwatches keeps apply swatches palette-only', () => { + const swatches = buildVerseActionSwatches({ + activeHighlights: new Set(), + selectedVerses: [1], + highlightedVerses: {}, + }); + + expect(swatches.map((swatch) => swatch.color)).toEqual([...HIGHLIGHT_COLORS]); + expect(swatches.every((swatch) => !swatch.showRemove)).toBe(true); + }); + + it('buildVerseActionSwatches drops invalid hex from the remove row', () => { + const swatches = buildVerseActionSwatches({ + activeHighlights: new Set(['not-valid', HIGHLIGHT_COLORS[0]]), + selectedVerses: [1, 2], + highlightedVerses: { 1: 'not-valid', 2: HIGHLIGHT_COLORS[0] }, + }); + + expect(swatches.filter((swatch) => swatch.showRemove)).toEqual([ + { color: HIGHLIGHT_COLORS[0], showRemove: true, key: `${HIGHLIGHT_COLORS[0]}-clear` }, + ]); + }); + + it('buildVerseActionSwatches shows remove for every distinct valid color on a mixed selection (ANY rule)', () => { + const custom = '112233'; + const swatches = buildVerseActionSwatches({ + activeHighlights: new Set([HIGHLIGHT_COLORS[0], custom]), + selectedVerses: [1, 2], + highlightedVerses: { 1: HIGHLIGHT_COLORS[0], 2: custom }, + }); + + const remove = swatches.filter((swatch) => swatch.showRemove); + expect(remove.map((swatch) => swatch.color)).toEqual([HIGHLIGHT_COLORS[0], custom]); + expect(swatches.filter((swatch) => !swatch.showRemove).map((swatch) => swatch.color)).toEqual( + HIGHLIGHT_COLORS, + ); + }); + + it('shows a remove swatch when non-palette covers only part of the selection (Story 10)', () => { + const custom = 'aabbcc'; + const swatches = buildVerseActionSwatches({ + activeHighlights: new Set([custom]), + selectedVerses: [1, 2], + highlightedVerses: { 1: custom }, + }); + + expect(swatches.filter((swatch) => swatch.showRemove)).toEqual([ + { color: custom, showRemove: true, key: `${custom}-clear` }, + ]); + }); + + it('normalizes uppercase and #‑prefixed palette colors into the remove row', () => { + const swatches = buildVerseActionSwatches({ + activeHighlights: new Set(['#FFFE00', '5DFF79']), + selectedVerses: [1, 2], + highlightedVerses: { 1: 'fffe00', 2: '5dff79' }, + }); + + expect(swatches.filter((swatch) => swatch.showRemove).map((swatch) => swatch.color)).toEqual([ + HIGHLIGHT_COLORS[0], + HIGHLIGHT_COLORS[1], + ]); + }); + + it('dedupes the same hex when activeHighlights carries multiple casings', () => { + const custom = 'aabbcc'; + const swatches = buildVerseActionSwatches({ + activeHighlights: new Set([custom, custom.toUpperCase()]), + selectedVerses: [1], + highlightedVerses: { 1: custom }, + }); + + expect(swatches.filter((swatch) => swatch.showRemove)).toEqual([ + { color: custom, showRemove: true, key: `${custom}-clear` }, + ]); + }); +}); diff --git a/packages/ui/src/lib/highlight-colors.ts b/packages/ui/src/lib/highlight-colors.ts new file mode 100644 index 00000000..52c1947a --- /dev/null +++ b/packages/ui/src/lib/highlight-colors.ts @@ -0,0 +1,85 @@ +/** + * Highlight color rules (YPE-4494): apply stays palette-only; paint and clear + * accept any valid 6-digit API hex; invalid hex is dropped everywhere. + */ + +/** Canonical apply palette — yellow, green, blue, orange, pink (matches iOS). */ +export const HIGHLIGHT_COLORS = ['fffe00', '5dff79', '00d6ff', 'ffc66f', 'ff95ef'] as const; + +export type HighlightColor = (typeof HIGHLIGHT_COLORS)[number]; + +const HIGHLIGHT_HEX_REGEX = /^[0-9a-f]{6}$/i; + +function stripHighlightHexPrefix(color: string): string { + return color.startsWith('#') ? color.slice(1) : color; +} + +/** Whether `color` is a valid API highlight hex (6 digits, optional `#` stripped). */ +export function isValidHighlightHex(color: string): boolean { + return HIGHLIGHT_HEX_REGEX.test(stripHighlightHexPrefix(color)); +} + +/** Lowercases a valid highlight hex, or `null` when invalid. */ +export function normalizeHighlightHex(color: string): string | null { + if (!isValidHighlightHex(color)) return null; + return stripHighlightHexPrefix(color).toLowerCase(); +} + +/** Whether `color` is one of the five SDK apply swatches (valid hex required). */ +export function isPaletteHighlightColor(color: string): boolean { + const normalized = normalizeHighlightHex(color); + if (normalized === null) return false; + return (HIGHLIGHT_COLORS as readonly string[]).includes(normalized); +} + +export type VerseActionSwatch = { + color: string; + showRemove: boolean; + key: string; +}; + +export type BuildVerseActionSwatchesInput = { + /** Distinct valid colors on the current selection (ANY rule). */ + activeHighlights: ReadonlySet; + selectedVerses: readonly number[]; + highlightedVerses: Readonly>; +}; + +/** + * Builds the verse-action popover swatch row: remove circles (checkmark) for + * every distinct valid color on the selection — palette or not — then apply + * circles for palette colors only. Invalid hex never appears. + */ +export function buildVerseActionSwatches({ + activeHighlights, + selectedVerses, + highlightedVerses, +}: BuildVerseActionSwatchesInput): VerseActionSwatch[] { + const normalizedActive = new Set(); + for (const color of activeHighlights) { + const normalized = normalizeHighlightHex(color); + if (normalized !== null) normalizedActive.add(normalized); + } + + const activePalette = HIGHLIGHT_COLORS.filter((color) => normalizedActive.has(color)); + const activeNonPalette = [...normalizedActive] + .filter((color) => !(HIGHLIGHT_COLORS as readonly string[]).includes(color)) + // Deterministic tray order across renders; palette colors stay in canonical order above. + .sort(); + + const removeColors = [...activePalette, ...activeNonPalette]; + + const highlightedVerseCount = selectedVerses.filter((verse) => highlightedVerses[verse]).length; + const unHighlightedCount = selectedVerses.length - highlightedVerseCount; + const allPaletteColorsActive = HIGHLIGHT_COLORS.every((color) => normalizedActive.has(color)); + const showAllApplyColors = + !allPaletteColorsActive && (unHighlightedCount > 0 || normalizedActive.size > 1); + const colorsToApply = showAllApplyColors + ? HIGHLIGHT_COLORS + : HIGHLIGHT_COLORS.filter((color) => !normalizedActive.has(color)); + + return [ + ...removeColors.map((color) => ({ color, showRemove: true, key: `${color}-clear` })), + ...colorsToApply.map((color) => ({ color, showRemove: false, key: `${color}-apply` })), + ]; +} diff --git a/packages/ui/src/lib/highlight-projection.test.ts b/packages/ui/src/lib/highlight-projection.test.ts index 1608ac4f..24dbc343 100644 --- a/packages/ui/src/lib/highlight-projection.test.ts +++ b/packages/ui/src/lib/highlight-projection.test.ts @@ -75,18 +75,21 @@ describe('deriveHighlightedVerses', () => { expect(map).toEqual({ 16: 'fffe00', 18: '5dff79', 19: '5dff79' }); }); - it('ignores colors outside allowedColors, matching case-insensitively', () => { + it('drops invalid hex and paints valid non-palette colors', () => { const map = deriveHighlightedVerses( - [highlight(111, 'JHN.3.16', 'abcdef'), highlight(111, 'JHN.3.17', 'FFFE00')], + [ + highlight(111, 'JHN.3.16', 'gggggg'), + highlight(111, 'JHN.3.17', 'abcdef'), + highlight(111, 'JHN.3.18', 'FFFE00'), + ], 111, 'JHN', '3', - ['fffe00'], ); - expect(map).toEqual({ 17: 'fffe00' }); + expect(map).toEqual({ 17: 'abcdef', 18: 'fffe00' }); }); - it('accepts any color when allowedColors is omitted', () => { + it('accepts valid non-palette hex', () => { const map = deriveHighlightedVerses([highlight(111, 'JHN.3.16', 'abcdef')], 111, 'JHN', '3'); expect(map).toEqual({ 16: 'abcdef' }); }); diff --git a/packages/ui/src/lib/highlight-projection.ts b/packages/ui/src/lib/highlight-projection.ts index 640a93bf..259f8746 100644 --- a/packages/ui/src/lib/highlight-projection.ts +++ b/packages/ui/src/lib/highlight-projection.ts @@ -1,4 +1,5 @@ import type { Highlight } from '@youversion/platform-core'; +import { normalizeHighlightHex } from './highlight-colors'; /** * Defensive cap on how many verses a single range USFM may expand to. The @@ -50,24 +51,21 @@ export function expandPassageId(passageId: string): ExpandedPassageId | null { * Entries for other versions, books, or chapters are ignored — every entry * carries its full identity, so stale host data can never mispaint. Range * passage ids are expanded per verse. Colors are normalized to lowercase (the - * API accepts uppercase at the boundary). Later entries win on collisions. - * - * When `allowedColors` is given, entries with any other color are ignored too: - * the verse-action popover can only offer removal for its own swatches, so a - * color the reader can't manage must not paint (it would be un-removable). + * API accepts uppercase at the boundary). Invalid hex is dropped. Valid + * non-palette colors paint so the remove tray can clear them (YPE-4494). Later + * entries win on collisions. */ export function deriveHighlightedVerses( highlights: readonly Highlight[], versionId: number, book: string, chapter: string, - allowedColors?: readonly string[], ): Record { const map: Record = {}; for (const { version_id, passage_id, color } of highlights) { if (version_id !== versionId) continue; - const normalizedColor = color.toLowerCase(); - if (allowedColors && !allowedColors.includes(normalizedColor)) continue; + const normalizedColor = normalizeHighlightHex(color); + if (normalizedColor === null) continue; const expanded = expandPassageId(passage_id); if (!expanded || expanded.book !== book || expanded.chapter !== chapter) continue; for (const verse of expanded.verses) {