fix(web): decide the sidebar's pointer leave by geometry, and tighten the gate
Review findings from the first round: - ui-settings renders its full-viewport panel as a fixed-position DESCENDANT of the sidebar column, so `pointerleave` never fires when the pointer moves onto it and the bars stayed drawn after it closed. Leaving is now decided against the column's box from a document-level pointermove that exists only while the bars are drawn; the element's own leave stays for the pointer that leaves the window. - The rebind gate judges the rule rather than each declaration, so a pair that mixes `transparent` with an l2 hover no longer passes, and the elevation half compares whole values instead of token shape. - Hiding no longer exempts a sheet from the elevated-surface rebind check. - The e2e polls the reveal before reading a colour for the golden, and pins that a pointerless scroll draws no thumb.
This commit is contained in:
@@ -73,25 +73,58 @@ export function SidebarRoot({
|
||||
// away): drawn while it is inside, and for SCROLLBAR_LINGER_MS after it
|
||||
// leaves. A pointer that returns within that window cancels the pending
|
||||
// hide rather than restarting from a hidden bar.
|
||||
const column = useRef<HTMLDivElement>(null)
|
||||
const [pointerInside, setPointerInside] = useState(false)
|
||||
const lingerTimer = useRef<number | undefined>(undefined)
|
||||
useEffect(() => () => { window.clearTimeout(lingerTimer.current) }, [])
|
||||
const armLinger = (): void => {
|
||||
if (lingerTimer.current !== undefined) return
|
||||
lingerTimer.current = window.setTimeout(() => {
|
||||
lingerTimer.current = undefined
|
||||
setPointerInside(false)
|
||||
}, SCROLLBAR_LINGER_MS)
|
||||
}
|
||||
const cancelLinger = (): void => {
|
||||
window.clearTimeout(lingerTimer.current)
|
||||
lingerTimer.current = undefined
|
||||
}
|
||||
// Leaving is decided by the column's BOX, not by DOM containment, and only
|
||||
// while the bars are drawn. ui-settings renders its full-viewport panel as a
|
||||
// fixed-position DESCENDANT of this column, so a pointer moved onto that
|
||||
// panel — or onto the conversation once it closes — fires no `pointerleave`
|
||||
// here, and the bars would stay drawn over a column nobody is pointing at.
|
||||
// The element's own leave stays as the one signal geometry cannot give: a
|
||||
// pointer that leaves the window emits no further moves.
|
||||
useEffect(() => {
|
||||
if (!pointerInside) return
|
||||
const onMove = (event: PointerEvent): void => {
|
||||
const rect = column.current?.getBoundingClientRect()
|
||||
/* v8 ignore next -- the listener only exists while the column is mounted and revealed. */
|
||||
if (rect === undefined) return
|
||||
const inside = event.clientX >= rect.left && event.clientX < rect.right
|
||||
&& event.clientY >= rect.top && event.clientY < rect.bottom
|
||||
if (inside) cancelLinger()
|
||||
else armLinger()
|
||||
}
|
||||
document.addEventListener('pointermove', onMove)
|
||||
return () => {
|
||||
document.removeEventListener('pointermove', onMove)
|
||||
cancelLinger()
|
||||
}
|
||||
}, [pointerInside])
|
||||
|
||||
return (
|
||||
<div
|
||||
ref={column}
|
||||
className={clsx(
|
||||
css.root, !wide && css.collapsed, !wide && everWide.current && css.railIn,
|
||||
collapsed && wide && css.fading, !pointerInside && css.quietBars,
|
||||
)}
|
||||
style={wide ? { width: collapsed ? lastWideWidth.current : width } : undefined}
|
||||
onPointerEnter={() => {
|
||||
window.clearTimeout(lingerTimer.current)
|
||||
cancelLinger()
|
||||
setPointerInside(true)
|
||||
}}
|
||||
onPointerLeave={() => {
|
||||
window.clearTimeout(lingerTimer.current)
|
||||
lingerTimer.current = window.setTimeout(() => { setPointerInside(false) }, SCROLLBAR_LINGER_MS)
|
||||
}}
|
||||
onPointerLeave={() => { armLinger() }}
|
||||
>
|
||||
<div className={css.logoRow}>
|
||||
{/* Expanded, the wordmark doubles as a New Session shortcut; the
|
||||
|
||||
@@ -11,6 +11,10 @@ import type { SidebarRootComponentProps, SidebarSectionOwnerProps } from '../src
|
||||
import { SidebarRoot } from '../src/client/SidebarRoot.tsx'
|
||||
import { en } from '../src/client/locales.ts'
|
||||
|
||||
/** Pinned column box; the shell compares pointer coordinates against it. */
|
||||
const COLUMN_WIDTH = 280
|
||||
const COLUMN_HEIGHT = 600
|
||||
|
||||
const t: SidebarRootComponentProps['t'] = key => (en as Record<string, string>)[key] ?? key
|
||||
/** The shell never reads the global hooks; the props share carries them regardless. */
|
||||
const neverHook = (() => { throw new Error('shell must not read global hooks') }) as never
|
||||
@@ -36,6 +40,14 @@ function mountColumn(): { column: HTMLElement; quiet: () => boolean } {
|
||||
)
|
||||
const column = view.container.firstElementChild
|
||||
if (!(column instanceof HTMLElement)) throw new Error('sidebar column not rendered')
|
||||
// jsdom lays nothing out, and the leave decision is geometric: pin the box
|
||||
// the shell reads so a coordinate can be inside or outside it.
|
||||
Object.defineProperty(column, 'getBoundingClientRect', {
|
||||
value: () => ({
|
||||
left: 0, top: 0, right: COLUMN_WIDTH, bottom: COLUMN_HEIGHT,
|
||||
x: 0, y: 0, width: COLUMN_WIDTH, height: COLUMN_HEIGHT, toJSON: () => ({}),
|
||||
}),
|
||||
})
|
||||
// CSS-module locals are hashed in this bench, so the state is read as a
|
||||
// substring of the class list rather than as an exact local name.
|
||||
return { column, quiet: () => [...column.classList].some(name => name.includes('quietBars')) }
|
||||
@@ -54,6 +66,16 @@ function movePointer(column: HTMLElement, direction: 'in' | 'out'): void {
|
||||
else fireEvent.pointerOut(column, { relatedTarget: outside })
|
||||
}
|
||||
|
||||
/**
|
||||
* Move the pointer over the document, as a pointer crossing a fixed overlay
|
||||
* that is a DOM descendant of the column does.
|
||||
* @param x - client x coordinate.
|
||||
* @param y - client y coordinate.
|
||||
*/
|
||||
function movePointerOverDocument(x: number, y: number): void {
|
||||
fireEvent.pointerMove(document, { clientX: x, clientY: y })
|
||||
}
|
||||
|
||||
describe('SidebarRoot pointer-revealed scrollbars', () => {
|
||||
it('draws them only while the pointer is inside, and lingers on the way out', () => {
|
||||
vi.useFakeTimers()
|
||||
@@ -83,6 +105,45 @@ describe('SidebarRoot pointer-revealed scrollbars', () => {
|
||||
expect(quiet()).toBe(false)
|
||||
})
|
||||
|
||||
it('hides when the pointer moves outside the column box without leaving its subtree', () => {
|
||||
// ui-settings renders its full-viewport panel as a fixed-position
|
||||
// DESCENDANT of the column, so DOM containment reports the pointer as
|
||||
// still inside while it is visually somewhere else entirely.
|
||||
vi.useFakeTimers()
|
||||
const { column, quiet } = mountColumn()
|
||||
movePointer(column, 'in')
|
||||
expect(quiet()).toBe(false)
|
||||
movePointerOverDocument(COLUMN_WIDTH + 400, 300)
|
||||
act(() => { vi.advanceTimersByTime(2000) })
|
||||
expect(quiet()).toBe(true)
|
||||
})
|
||||
|
||||
it('does not restart the window when the pointer keeps moving outside', () => {
|
||||
vi.useFakeTimers()
|
||||
const { column, quiet } = mountColumn()
|
||||
movePointer(column, 'in')
|
||||
movePointer(column, 'out')
|
||||
act(() => { vi.advanceTimersByTime(1500) })
|
||||
// A pending hide is left alone rather than re-armed: otherwise a pointer
|
||||
// resting outside the column would keep pushing the bars' disappearance
|
||||
// out, one move at a time.
|
||||
movePointerOverDocument(COLUMN_WIDTH + 400, 300)
|
||||
act(() => { vi.advanceTimersByTime(600) })
|
||||
expect(quiet()).toBe(true)
|
||||
})
|
||||
|
||||
it('keeps them drawn while the pointer moves inside the column box', () => {
|
||||
vi.useFakeTimers()
|
||||
const { column, quiet } = mountColumn()
|
||||
movePointer(column, 'in')
|
||||
movePointer(column, 'out')
|
||||
// A move landing back inside the box cancels the pending hide, the same
|
||||
// way re-entering the element does.
|
||||
movePointerOverDocument(COLUMN_WIDTH - 10, 300)
|
||||
act(() => { vi.advanceTimersByTime(5000) })
|
||||
expect(quiet()).toBe(false)
|
||||
})
|
||||
|
||||
it('drops the pending hide when the column unmounts', () => {
|
||||
vi.useFakeTimers()
|
||||
const { column } = mountColumn()
|
||||
|
||||
@@ -33,6 +33,11 @@ const TOKEN_PREFIX = '--dsw-alias-scrollbar-'
|
||||
const INDIRECTION_PREFIX = '--dsh-scrollbar-'
|
||||
/** The one non-token rebind value: a surface that draws no thumb at all. */
|
||||
const HIDDEN_THUMB = 'transparent'
|
||||
/** The elevation rebind, spelled per property: value-wholeness, not token shape. */
|
||||
const ELEVATED_REBIND = new Map([
|
||||
['--dsh-scrollbar-thumb', '--dsw-alias-scrollbar-bg-l2'],
|
||||
['--dsh-scrollbar-thumb-hover', '--dsw-alias-scrollbar-hover-l2'],
|
||||
].map(([property, token]) => [property!, `var(${token!})`]))
|
||||
|
||||
/**
|
||||
* Flatten a stylesheet into rules. Whitespace, declaration order, and trailing
|
||||
@@ -181,8 +186,13 @@ interface SheetSurfaces {
|
||||
elevated: Set<string>
|
||||
/** True when some rule declares `overflow*: auto|scroll`. */
|
||||
scrolls: boolean
|
||||
/** True when some rule rebinds the indirection. */
|
||||
rebinds: boolean
|
||||
/**
|
||||
* True when some rule rebinds the indirection to an ELEVATION. A rule that
|
||||
* only hides the bar (`transparent`) does not count: it states no elevation,
|
||||
* so a sheet that hides its bars and also scrolls on an elevated surface
|
||||
* still owes the l2 pair for whatever draws a thumb there.
|
||||
*/
|
||||
rebindsElevation: boolean
|
||||
}
|
||||
const sheetSurfaces = new Map<string, SheetSurfaces>()
|
||||
|
||||
@@ -240,12 +250,16 @@ const elevatedSurfaces = elevatedRungs()
|
||||
|
||||
for (const file of packageStylesheets()) {
|
||||
const rules = parseRules(readFileSync(file, 'utf8'))
|
||||
const surfaces: SheetSurfaces = { elevated: new Set(), scrolls: false, rebinds: false }
|
||||
const surfaces: SheetSurfaces = { elevated: new Set(), scrolls: false, rebindsElevation: false }
|
||||
for (const rule of rules) {
|
||||
let rebinds = false
|
||||
let rebindsElevation = false
|
||||
const ruleSurfaces: string[] = []
|
||||
for (const [property, value] of rule.declarations) {
|
||||
if (property.startsWith(INDIRECTION_PREFIX) && file !== fileURLToPath(new URL('scrollbar.css', STYLES))) rebinds = true
|
||||
if (property.startsWith(INDIRECTION_PREFIX) && file !== fileURLToPath(new URL('scrollbar.css', STYLES))) {
|
||||
rebinds = true
|
||||
if (value !== HIDDEN_THUMB) rebindsElevation = true
|
||||
}
|
||||
if (OVERFLOW_PROPERTIES.includes(property) && /\b(?:auto|scroll)\b/.test(value)) surfaces.scrolls = true
|
||||
if (SURFACE_PROPERTIES.includes(property)) ruleSurfaces.push(...varReferences(value))
|
||||
for (const token of varReferences(value)) {
|
||||
@@ -256,10 +270,8 @@ for (const file of packageStylesheets()) {
|
||||
for (const token of ruleSurfaces) {
|
||||
if (elevatedSurfaces.has(token)) surfaces.elevated.add(token)
|
||||
}
|
||||
if (rebinds) {
|
||||
rebindRules.push({ file, rule })
|
||||
surfaces.rebinds = true
|
||||
}
|
||||
if (rebinds) rebindRules.push({ file, rule })
|
||||
if (rebindsElevation) surfaces.rebindsElevation = true
|
||||
}
|
||||
sheetSurfaces.set(file, surfaces)
|
||||
}
|
||||
@@ -454,21 +466,25 @@ describe('elevated surface rebinds', () => {
|
||||
}
|
||||
})
|
||||
|
||||
it('every rebind targets the l2 elevation pair or hides the bar outright', () => {
|
||||
// Two targets, and nothing else. An elevated surface moves the pair to l2;
|
||||
// a surface that draws no bar at all states `transparent` (ui-sidebar's
|
||||
// column, whose scrollbars follow the pointer). What this rejects is a
|
||||
// rebind to l1, which restates the base-surface default under a name that
|
||||
// reads as an elevation, and a literal colour, which leaves the palette.
|
||||
it('rebinds the pair to one target: the l2 elevation pair, or transparent', () => {
|
||||
// The rule as a whole, not each declaration on its own. Per-declaration
|
||||
// checking accepts a MIXED rule — `thumb: transparent` beside
|
||||
// `thumb-hover: var(--dsw-alias-scrollbar-hover-l2)` — which repaints the
|
||||
// bar the moment the pointer reaches it while passing a gate that claims
|
||||
// the two targets are exclusive.
|
||||
//
|
||||
// The elevation half compares the whole value against the pair's canonical
|
||||
// spelling rather than checking that every token it mentions ends in `-l2`.
|
||||
// A shape check admits `color-mix(…, var(--dsw-alias-scrollbar-bg-l2) 85%,
|
||||
// white)` and a crossed pair (the hover token bound to the resting
|
||||
// property); neither is what the contract says.
|
||||
for (const { file, rule } of rebindRules) {
|
||||
for (const [property, value] of rule.declarations) {
|
||||
if (!property.startsWith(INDIRECTION_PREFIX)) continue
|
||||
if (value === HIDDEN_THUMB) continue
|
||||
const tokens = varReferences(value)
|
||||
expect(tokens, `${file}: ${property}: ${value}`).not.toEqual([])
|
||||
for (const token of tokens) {
|
||||
expect(token, `${file}: ${property}`).toMatch(/-l2$/)
|
||||
}
|
||||
const rebinds = rule.declarations.filter(([property]) => property.startsWith(INDIRECTION_PREFIX))
|
||||
const where = `${file} ${rule.selectors.join(', ')}`
|
||||
if (rebinds.every(([, value]) => value === HIDDEN_THUMB)) continue
|
||||
expect(rebinds.some(([, value]) => value === HIDDEN_THUMB), `${where}: mixes ${HIDDEN_THUMB} with an elevation`).toBe(false)
|
||||
for (const [property, value] of rebinds) {
|
||||
expect(value, `${where}: ${property}`).toBe(ELEVATED_REBIND.get(property))
|
||||
}
|
||||
}
|
||||
})
|
||||
@@ -509,7 +525,7 @@ describe('elevated surface rebinds', () => {
|
||||
// (ChatView's `.toBottom`, CodeBlock's banner). Geometry cannot make that
|
||||
// call — a floating button carries a radius, a shadow, and a fixed size.
|
||||
for (const [file, surfaces] of sheetSurfaces) {
|
||||
if (!surfaces.scrolls || surfaces.rebinds) continue
|
||||
if (!surfaces.scrolls || surfaces.rebindsElevation) continue
|
||||
expect([...surfaces.elevated], `${file} scrolls on an elevated surface without rebinding`).toEqual([])
|
||||
}
|
||||
})
|
||||
|
||||
Reference in New Issue
Block a user