diff --git a/src/frontend/src/components/portal/lens-rail.test.tsx b/src/frontend/src/components/portal/lens-rail.test.tsx new file mode 100644 index 000000000..be41fa418 --- /dev/null +++ b/src/frontend/src/components/portal/lens-rail.test.tsx @@ -0,0 +1,183 @@ +// @vitest-environment jsdom +/** + * The rail's open state. + * + * Every case here is about the interaction rather than the look, because the + * look is the easy half. The one that matters is the click: a click navigates + * and leaves the pointer sitting on the rail, so without an explicit dismissal + * the rail reopens on top of the pane the click was aimed at. That failed + * silently once already when the state was expressed as CSS variants — the + * rules simply never matched and nothing said so. + */ +import { act, render, screen } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +const mocks = vi.hoisted(() => ({ + layout: "wide" as "phone" | "narrow" | "wide", + selected: [] as string[], +})); + +vi.mock("@/lib/portal/use-shell-layout", () => ({ + useShellLayout: () => mocks.layout, +})); +vi.mock("@/lib/portal/use-zone-nav", () => ({ + useZoneNav: () => ({ + zones: [ + { id: "overview", label: "Overview", icon: () => null }, + { id: "people", label: "People", icon: () => null }, + ], + activeZone: "overview", + selectZone: (z: { id: string }) => mocks.selected.push(z.id), + }), +})); +vi.mock("@/components/app-sidebar-footer", () => ({ + AppSidebarFooter: () => null, +})); + +import { SidebarProvider } from "@/components/ui/sidebar"; +import { LensRail } from "./lens-rail"; + +/** The rail opens on a timer, so a hover only counts once the wait is over. */ +const settle = () => act(() => { vi.advanceTimersByTime(400); }); + +const rail = () => + render( + + + , + ); + +/** The label is present either way; what changes is whether it can be seen. */ +const labelOf = (name: string) => + screen.getByRole("button", { name }).querySelector("span:not(.sr-only)"); + +beforeEach(() => { + vi.useFakeTimers({ shouldAdvanceTime: true }); + mocks.layout = "wide"; + mocks.selected = []; + window.matchMedia ??= ((query: string) => ({ + matches: false, + media: query, + onchange: null, + addEventListener: () => {}, + removeEventListener: () => {}, + addListener: () => {}, + removeListener: () => {}, + dispatchEvent: () => false, + })) as unknown as typeof window.matchMedia; +}); + +describe("LensRail", () => { + it("shows labels while the pointer is on it", async () => { + const user = userEvent.setup({ advanceTimers: vi.advanceTimersByTime }); + rail(); + expect(labelOf("Overview")).toHaveClass("opacity-0"); + + await user.hover(screen.getByTestId("lens-rail")); + settle(); + expect(labelOf("Overview")).toHaveClass("opacity-100"); + }); + + it("collapses on a click and stays collapsed under the pointer", async () => { + // The whole reason this state exists. The click navigates; the pointer has + // not moved; reopening here would cover the pane that was just asked for. + const user = userEvent.setup({ advanceTimers: vi.advanceTimersByTime }); + rail(); + await user.hover(screen.getByTestId("lens-rail")); + settle(); + expect(labelOf("People")).toHaveClass("opacity-100"); + + await user.click(screen.getByRole("button", { name: "People" })); + expect(mocks.selected).toEqual(["people"]); + expect(labelOf("People")).toHaveClass("opacity-0"); + }); + + it("expands again once the pointer has left and come back", async () => { + const user = userEvent.setup({ advanceTimers: vi.advanceTimersByTime }); + rail(); + const el = screen.getByTestId("lens-rail"); + + await user.hover(el); + settle(); + await user.click(screen.getByRole("button", { name: "People" })); + expect(labelOf("People")).toHaveClass("opacity-0"); + + await user.unhover(el); + await user.hover(el); + settle(); + expect(labelOf("People")).toHaveClass("opacity-100"); + }); + + it("renders nothing on a phone", () => { + // 56px of rail plus a 256px pane left a phone with almost no content; the + // zones live in the context pane's drawer there instead. + mocks.layout = "phone"; + rail(); + expect(screen.queryByTestId("lens-rail")).not.toBeInTheDocument(); + }); +}); + +afterEach(() => { + vi.useRealTimers(); +}); + +describe("LensRail state that only breaks in a particular order", () => { + it("does not strand itself when a zone is chosen from the keyboard", async () => { + // Enter on a focused button produces a click, and a click used to mean + // "the pointer is resting on me, stay shut until it leaves". There is no + // pointer in this story, so nothing would ever clear that — the rail was + // dead to the mouse from then on. + const user = userEvent.setup({ advanceTimers: vi.advanceTimersByTime }); + rail(); + await user.tab(); + await user.tab(); + await user.keyboard("{Enter}"); + expect(mocks.selected).toEqual(["people"]); + + await user.hover(screen.getByTestId("lens-rail")); + settle(); + expect(labelOf("People")).toHaveClass("opacity-100"); + }); + + it("shows the labels to a keyboard user at all", async () => { + // Eight identical icons and the text at zero opacity is not navigable by + // anyone who can see but is not using a pointer. + const user = userEvent.setup({ advanceTimers: vi.advanceTimersByTime }); + rail(); + await user.tab(); + await user.tab(); + expect(labelOf("Overview")).toHaveClass("opacity-100"); + }); + + it("comes back shut after the rail is unmounted under the pointer", async () => { + // A width change unmounts the rail without a pointer-leave, so the state + // it left behind used to survive: widen again and it was already open, + // with the pointer nowhere near it. + const user = userEvent.setup({ advanceTimers: vi.advanceTimersByTime }); + const { rerender } = rail(); + await user.hover(screen.getByTestId("lens-rail")); + settle(); + expect(labelOf("Overview")).toHaveClass("opacity-100"); + + mocks.layout = "phone"; + rerender(); + mocks.layout = "wide"; + rerender(); + expect(labelOf("Overview")).toHaveClass("opacity-0"); + }); + + it("stays shut for a pointer that is only passing through", async () => { + // The wait is the whole guard. Before it was a timer, the panel became + // clickable at once and merely being over it counted as staying, so a + // crossing pointer opened the rail anyway — over the row it was heading + // for, having swallowed any click on the way. + const user = userEvent.setup({ advanceTimers: vi.advanceTimersByTime }); + rail(); + const el = screen.getByTestId("lens-rail"); + await user.hover(el); + await user.unhover(el); + settle(); + expect(labelOf("Overview")).toHaveClass("opacity-0"); + }); +}); diff --git a/src/frontend/src/components/portal/lens-rail.tsx b/src/frontend/src/components/portal/lens-rail.tsx index 47c76be90..a94f835fd 100644 --- a/src/frontend/src/components/portal/lens-rail.tsx +++ b/src/frontend/src/components/portal/lens-rail.tsx @@ -1,4 +1,5 @@ import { Settings2 } from "lucide-react"; +import { useEffect, useRef, useState } from "react"; import { AppSidebarFooter } from "@/components/app-sidebar-footer"; import { @@ -14,93 +15,315 @@ import { SidebarMenu, SidebarMenuButton, SidebarMenuItem, + useSidebar, } from "@/components/ui/sidebar"; -import { type Zone } from "@/lib/portal/nav-model"; import { useShellLayout } from "@/lib/portal/use-shell-layout"; +import type { Zone } from "@/lib/portal/nav-model"; import { useZoneNav } from "@/lib/portal/use-zone-nav"; +import { cn } from "@/lib/utils"; /** - * Portal primary rail: a bounded set of zone icons. Entity zones (Person / - * People) link to the existing dashboard routes and clear the theme-zone - * selection; other zones set the active zone so the context pane switches. - * Zones the active role can't see are filtered out (permission layer — FE - * stub over the future role_section_visibility entity). Rendered as a - * `collapsible="none"` sidebar so it sits in normal flow beside the pane. + * The zone rail: one icon per zone, expanding to labels on hover. + * + * Zones that are dashboards in their own right (Person, People) link to the + * existing dashboard routes and clear the theme-zone selection; other zones set + * the active zone so the context pane switches. Zones the active role can't see + * are filtered out (permission layer — FE stub over the future + * role_section_visibility entity). * * Below 768px the rail renders nothing: 56px of icons plus a 256px pane left a * phone with ~60px of content. The same zones (labelled, not icon-only) live in * the context pane's drawer instead — see `ContextPane`. On a tablet the rail * stays: 56px is affordable, and it is the pane that collapses. + * + * ── The expansion, ported from the lite product's rail ────────────────────── + * + * Four things make it work, and each one is there because leaving it out broke + * something: + * + * 1. The rail keeps its 56px slot in the layout and the labels open OVER the + * pane. Widening the element itself would shove the pane sideways every time + * a pointer crossed the rail on its way somewhere else. + * + * 2. The buttons widen to the full open width, but ONLY while it is open. A + * label you can read but not click is a trap: the pointer leaves the 56px + * column on its way to the word and the rail shuts before it arrives. People + * aim at what they can read. Keeping the buttons narrow while shut is what + * stops that from costing anything — approaching the pane from the content + * side never opens the rail over the row being reached for. + * + * 3. The buttons are inside the hover target, so a pointer resting on one keeps + * the rail open rather than fighting the thing that opened it. + * + * 4. A click collapses it until the pointer leaves. A click navigates, and the + * pointer is still on the rail afterwards — without this the rail reopens + * immediately, on top of the pane the click was aimed at. The lite product + * needs `sessionStorage` for this because its click reloads the page; here + * the navigation is client-side, so plain state survives it and is dropped + * the moment the pointer leaves. + */ + +/** + * How far the open rail reaches — wide enough for the longest zone label and no + * wider. It deliberately does NOT cover the pane beside it: an overlay that + * swallows the whole second column hides where the reader just was, and the + * pane is what they are usually navigating towards. With an edge and a shadow + * it reads as a panel resting over the pane rather than as a half-covered one. */ +const OPEN_WIDTH = "12rem"; + +/** + * Where the context pane ends, when it is there at all. + * + * The fade only has work to do while the pane is beside the rail. It collapses + * off-canvas on the middle width tier, and a reader can shut it by hand on a + * wide one — painting a fixed 19.5rem of gradient in either case laid a dimmed + * strip over the content for no reason. + */ +const PANE_EDGE = "19.5rem"; + +/** + * How long a pointer has to stay before the labels appear. + * + * The wait is the whole guard against opening over something a pointer was + * only passing. An earlier version delayed the FADE and let the panel become + * clickable immediately, which achieved the opposite of that: the invisible + * panel is wide, a pointer crossing towards the pane landed on it, that + * counted as still being inside, and the rail opened after the delay anyway — + * over exactly the row being reached for, having swallowed any click made in + * the meantime. Opening on a timer keeps "open" and "visible" the same fact, + * so there is no window in which one is true and the other is not. + */ +const OPEN_AFTER_MS = 200; + export function LensRail() { const layout = useShellLayout(); const { zones, activeZone, selectZone } = useZoneNav(); + const { state: paneState } = useSidebar(); + const paneIsBeside = paneState === "expanded"; + const [open, setOpen] = useState(false); + // Suppresses the hover until the pointer leaves. Only a pointer-driven click + // sets it — see the note where it is set. + const [dismissed, setDismissed] = useState(false); + const timer = useRef | null>(null); + + const cancel = () => { + if (timer.current !== null) { + clearTimeout(timer.current); + timer.current = null; + } + }; + const close = () => { + cancel(); + setOpen(false); + }; + + // A pointer can leave without a leave event: the element can be unmounted + // under it by a width change, a dialog can take the pointer, the window can + // lose focus. Each of those used to strand the state — cross the rail, narrow + // the window to the phone tier and back, and it returned already open with + // the pointer nowhere near it. + // + // Adjusted during render rather than in an effect. An effect would set state + // after painting the stale frame, and React flags the cascade; this is the + // documented shape for "a fact this state depended on has changed". + const [layoutOfState, setLayoutOfState] = useState(layout); + if (layoutOfState !== layout) { + setLayoutOfState(layout); + setOpen(false); + setDismissed(false); + } + + // Refs may not be touched during render, so the pending timer is dropped + // here instead: without this a wait started before a width change would fire + // afterwards and open a rail no pointer is on. + useEffect(() => cancel, [layout]); if (layout === "phone") return null; return ( - - -
- I -
-
- - - {zones.map((z) => ( - { + // Touch fires enter at press and leave at release, so a tap would + // flash the labels for the length of the tap and nothing else. Leave + // it shut there; the same zones are labelled in the pane's drawer. + if (e.pointerType === "touch" || dismissed) return; + cancel(); + timer.current = setTimeout(() => setOpen(true), OPEN_AFTER_MS); + }} + onPointerLeave={() => { + close(); + setDismissed(false); + }} + // Keyboard gets the labels too, and immediately: a sighted keyboard user + // was tabbing through eight identical icons with the text at zero + // opacity, and `title` does not surface on focus in any browser. + onFocusCapture={() => { + cancel(); + setOpen(true); + }} + onBlurCapture={(e) => { + if (!e.currentTarget.contains(e.relatedTarget as Node | null)) { + close(); + setDismissed(false); + } + }} + > + + {/* A fade beside the panel, not a flat veil. + The pane's rows do not object to being dimmed — they object to + being CUT, and a hard edge through the middle of a word reads as a + rendering fault whatever its brightness. So the strip the panel + does not cover goes from fully hidden at the panel's edge to fully + visible at the pane's, and a row dissolves instead of stopping + mid-letter. Dimming it uniformly was tried first and did nothing: + the cut, not the contrast, was the problem. */} + {paneIsBeside ? ( +
+ ) : null} + {/* The panel the labels sit on. + It takes pointer events WHILE OPEN, and that is not a detail: with + it inert the gaps between buttons belong to whatever is underneath, + so a pointer moving from an icon towards its label crosses bare + panel, the rail counts that as having been left, and it slams shut + under the hand that was reaching for it. + Delayed both ways: crossing the rail on the way elsewhere should not + flash it open, and leaving briefly should not shut it. */} +
+ +
+ I +
+
+ {/* The zone list scrolls while shut and lets the labels out while + open. It cannot do both: a box that clips its overflow on one axis + clips it on the other too, whatever `overflow-x: visible` says, so + the widened buttons were being cut off at the rail's edge. + Scrolling is the half worth losing, and only for as long as the + labels are showing — a reader who needs to scroll can move the + pointer away, which is also how they stop reading the labels. An + earlier version escaped the clip with a blanket child selector, + which won on specificity against this box's own overflow and left + the list unable to scroll at all, open or shut. */} + + + {zones.map((z) => ( + { + if (viaPointer) { + setDismissed(true); + close(); + } + selectZone(zone); + }} + /> + ))} + + + + + + + Settings + + } /> - ))} - - - - - - - Settings - - } - /> - - - - - - + + + + + + +
); } function ZoneItem({ zone, active, + open, onSelect, }: { zone: Zone; active: boolean; - onSelect: (zone: Zone) => void; + open: boolean; + onSelect: (zone: Zone, viaPointer: boolean) => void; }) { const Icon = zone.icon; return ( - + onSelect(zone)} + // 40px shut, the full open width while open — see note 2 above. The + // icon does not move between the two: the button starts its content at + // the same offset either way. + className={cn( + "h-10 justify-start gap-2 overflow-hidden p-0 ps-[10px] transition-[width] duration-150", + open || "w-10" + )} + style={open ? { width: `calc(${OPEN_WIDTH} - 1rem)` } : undefined} + // `detail` counts pointer clicks: keyboard activation reports 0. + onClick={(e) => onSelect(zone, e.detail > 0)} > - - {zone.label} + + {/* Visible only while open, and never a pointer target of its own — + the button under it is what widens, so the word IS the hit area. */} + + {zone.label} + ); diff --git a/src/frontend/src/components/widgets/metric-views/metric-activity.tsx b/src/frontend/src/components/widgets/metric-views/metric-activity.tsx index 23f3bc93e..05e6d27a9 100644 --- a/src/frontend/src/components/widgets/metric-views/metric-activity.tsx +++ b/src/frontend/src/components/widgets/metric-views/metric-activity.tsx @@ -296,10 +296,17 @@ function DayStrip({ denominators.size === 1 ? [...denominators][0] : null; return ( -
- {/* Hover reads out in the caption below rather than in a tooltip per - day: a month is thirty-one triggers, and a floating card that covers - its neighbours is the wrong shape for asking "what was that bar". +
+ {/* The reading appears over the bar the pointer is on, not in the + caption below. Put anywhere else it is a change in the middle of a + chart, which is exactly where a reader looking at one bar does not + look — they hover, the number moves somewhere in their periphery, and + they never learn it was there. + + One positioned element rather than a tooltip per day: a month is + thirty-one triggers, and thirty-one floating cards is both heavy and + the wrong shape. It sits ABOVE the bars so it never covers the + neighbours being compared against. The strip carries its own description rather than making each day focusable. Thirty-one tab stops per strip, three strips to a section, @@ -307,11 +314,30 @@ function DayStrip({ the shape is — and the per-day figure is an enhancement over content the header and caption already state. */}
setHovered(null)} > + {hoveredDay && hovered != null ? ( +
+ {dayTitle(metric, hoveredDay)} +
+ ) : null} {days.map((day, index) => (
{period ? formatDate(period.from) : null} - - {hoveredDay - ? dayTitle(metric, hoveredDay) - : constantDenominator != null - ? `measured against ${constantDenominator} per day` - : silent > 0 - ? `${silent} ${silent === 1 ? "day" : "days"} with no reading` - : null} + + {constantDenominator != null + ? `measured against ${constantDenominator} per day` + : silent > 0 + ? `${silent} ${silent === 1 ? "day" : "days"} with no reading` + : null} {period ? formatDate(period.to) : null}