fix(directory-picker-browse): resolve quiet-navigation review
This commit is contained in:
creatixchu
imccyu
+2
-2
@@ -2,5 +2,5 @@
|
||||
# side as of the last confirmed-consistent state. Both languages carry equal authority;
|
||||
# after editing either side, bring the other along and re-record with:
|
||||
# pnpm run verify-translation-pairing --write .agents/notes/implemented/bug-fix/2026-07-28-themed-scrollbars-and-reserved-gutter.md
|
||||
2026-07-28-themed-scrollbars-and-reserved-gutter.md: 76dcb6d9f3976faf3338a89f3ccab7182fe6d5ab
|
||||
2026-07-28-themed-scrollbars-and-reserved-gutter.zh.md: ff12c6884bec706dbbd9974684010cbcd9fa03bf
|
||||
2026-07-28-themed-scrollbars-and-reserved-gutter.md: b45f70b126d083916c756afb88a8b646a4e9bb85
|
||||
2026-07-28-themed-scrollbars-and-reserved-gutter.zh.md: 8afa36429ce7e6e061b014d63dcb20e5a642a84c
|
||||
+3
-3
@@ -20,13 +20,13 @@ The rules sit on `body`, not `html`. `design-platform.css` declares the `--dsw-a
|
||||
|
||||
The two renderings are mutually exclusive, and the exclusion is enforced rather than assumed. A non-`auto` `scrollbar-width` or `scrollbar-color` makes Chromium and Safari discard every `::-webkit-scrollbar*` rule for that element, `::-webkit-scrollbar-thumb:hover` included. Declaring both unconditionally therefore leaves the hover token rendering nowhere at all: the engines that implement the hover pseudo-element are exactly the ones the standard properties silence, and Firefox has no hover pseudo-element to fall back on. The standard properties consequently sit inside `@supports not selector(::-webkit-scrollbar)`, which is true only where the pseudo-element is unimplemented, so Firefox takes the standard path and WebKit-based engines take the pseudo-element path. The WebKit rules are not gated in turn: an engine without those pseudo-elements drops them as unknown selectors, so a gate would only restate what selector matching already does. An engine too old for the `selector()` function makes the condition invalid, which evaluates false and selects the pseudo-element path — the correct side for the pre-16.4 Safari that is the realistic case for that reading.
|
||||
|
||||
Both paths read one indirection pair, `--dsh-scrollbar-thumb` and `--dsh-scrollbar-thumb-hover`, bound on `body` to the l1 (base-surface) tokens. **This is the rebinding contract, and it is the part the CSS alone does not state**: an elevated surface sets `--dsh-scrollbar-thumb: var(--dsw-alias-scrollbar-bg-l2)` and `--dsh-scrollbar-thumb-hover: var(--dsw-alias-scrollbar-hover-l2)` on its own container, and that one rebind reaches the standard properties and the WebKit pseudo-elements together. The pair is rebound as a pair; rebinding the resting thumb alone leaves the hover state on the base-surface token. The set of rebinding surfaces is owned by the mechanical gate (`packages/client/ui-theme/tests/scrollbar-styles.spec.ts`): any sheet that scrolls and paints an elevated surface must rebind, so this note no longer enumerates them (an enumeration here drifted twice). Most declare it on the elevated card rather than on the scrolling descendant, because the elevation is a property of the surface and custom properties inherit down to whichever child actually scrolls.
|
||||
Both paths read one indirection pair, `--dsh-scrollbar-thumb` and `--dsh-scrollbar-thumb-hover`, bound on `body` to the l1 (base-surface) tokens. **This is the rebinding contract, and it is the part the CSS alone does not state**: an elevated surface sets `--dsh-scrollbar-thumb: var(--dsw-alias-scrollbar-bg-l2)` and `--dsh-scrollbar-thumb-hover: var(--dsw-alias-scrollbar-hover-l2)` on its own container, and that one rebind reaches the standard properties and the WebKit pseudo-elements together. The pair is rebound as a pair; rebinding the resting thumb alone leaves the hover state on the base-surface token. The mechanically discoverable subset is owned by `packages/client/ui-theme/tests/scrollbar-styles.spec.ts`: any sheet that both scrolls and paints an elevated surface must rebind, so this note no longer maintains a complete surface inventory. Most declare the pair on the elevated card rather than on the scrolling descendant, because elevation belongs to the surface and custom properties inherit to whichever child actually scrolls.
|
||||
|
||||
The last four were missed in the first implementation and found in review, which is why the rebinding contract is now checked mechanically rather than by inspection: a sheet that scrolls somewhere and paints an elevated surface somewhere must rebind.
|
||||
Four surfaces — `Menu`, `InputBar`, `QuestionComposer`, and `TodoPanel` — were missed in the first implementation and found in review, which is why the per-sheet rebinding contract is checked mechanically rather than by inspection.
|
||||
|
||||
The elevated set is resolved from the palette's own dark elevation ladder — the surface tokens whose dark value lands on `bg-layer-2` or `bg-layer-3`, which is the step the l1/l2 split encodes. Deriving it instead from the sheets that already rebind was the first attempt and is unsound: such a set can only confirm what someone already remembered, and a surface nobody has rebound yet — exactly the case the check exists for — defines itself as unelevated. `--dsw-specific-tip` proved it, resolving to the menu surface's rung while the todo panel scrolled on it unrebound and the derived check stayed green.
|
||||
|
||||
Scope is by token family, not by geometry: only `--dsw-alias-bg-*` and `--dsw-specific-*` name a surface. `--dsw-alias-button-*`, `--dsw-alias-interactive-*`, and `--dsw-alias-markdown-*` reach the same rungs while naming a control or an inline span that no scroll container renders its bar against. Shape cannot make that call, since a floating button legitimately carries a radius, a shadow, and a fixed size. The check is per sheet rather than per rule because the card and the descendant that scrolls are separate rules, and CSS text does not express which contains which.
|
||||
Scope is by token family, not by geometry: only `--dsw-alias-bg-*` and `--dsw-specific-*` name a surface. `--dsw-alias-button-*`, `--dsw-alias-interactive-*`, and `--dsw-alias-markdown-*` reach the same rungs while naming a control or an inline span that no scroll container renders its bar against. Shape cannot make that call, since a floating button legitimately carries a radius, a shadow, and a fixed size. The check is per sheet rather than per rule because the card and the descendant that scrolls are separate rules. That approximation cannot detect a scrolling component embedded in an elevated card painted by another package's stylesheet, as `DirectoryBrowser` inside `Modal` demonstrated; cross-sheet composition remains a review and assembled-UI responsibility.
|
||||
|
||||
The track and the corner stay transparent, so the thumb reads against whatever surface scrolls under it; only the thumb and its hover state carry a token color.
|
||||
|
||||
|
||||
+3
-3
@@ -20,13 +20,13 @@ Status: implemented
|
||||
|
||||
两种渲染互斥，而这种互斥是被强制的，不是假定的。`scrollbar-width` 或 `scrollbar-color` 只要取非 `auto` 值，Chromium 与 Safari 就会丢弃该元素上的全部 `::-webkit-scrollbar*` 规则，`::-webkit-scrollbar-thumb:hover` 也在其中。因此无条件地同时声明会让 hover token 在任何地方都得不到渲染：实现了 hover 伪元素的引擎，恰恰就是被标准属性静音的那些，而 Firefox 没有 hover 伪元素可作退路。于是标准属性写在 `@supports not selector(::-webkit-scrollbar)` 之内，该条件只在伪元素未被实现处为真，因此 Firefox 走标准属性路径，WebKit 系引擎走伪元素路径。WebKit 规则不再反向加门禁：不实现这些伪元素的引擎会把它们当作未知选择器丢弃，因此加门禁只是重述选择器匹配本身已经做的事。对于旧到不支持 `selector()` 函数的引擎，该条件无效，从而求值为假并选中伪元素路径——对于这条判断下现实存在的 16.4 之前的 Safari，这正是正确的一侧。
|
||||
|
||||
两条路径都读取同一组间接变量 `--dsh-scrollbar-thumb` 与 `--dsh-scrollbar-thumb-hover`，它们在 `body` 上绑定到 l1（基础表面）token。**这就是重新绑定契约，也是单看 CSS 无法得知的部分**：抬升表面在自己的容器上设置 `--dsh-scrollbar-thumb: var(--dsw-alias-scrollbar-bg-l2)` 与 `--dsh-scrollbar-thumb-hover: var(--dsw-alias-scrollbar-hover-l2)`，这一次重新绑定同时作用于标准属性和 WebKit 伪元素。这组变量必须成对重新绑定；只改静止态滑块会让 hover 状态仍留在基础表面的 token 上。重新绑定表面的集合归机械门禁（`packages/client/ui-theme/tests/scrollbar-styles.spec.ts`）所有：任何既滚动又绘制抬升表面的样式表都必须重新绑定，因此本 note 不再枚举它们（这里的枚举已经漂移过两次）。多数把声明写在抬升卡片上而非滚动的后代元素上，因为抬升层级是这个表面的属性，而自定义属性会继承到真正滚动的那个子元素。
|
||||
两条路径都读取同一组间接变量 `--dsh-scrollbar-thumb` 与 `--dsh-scrollbar-thumb-hover`，它们在 `body` 上绑定到 l1（基础表面）token。**这就是重新绑定契约，也是单看 CSS 无法得知的部分**：抬升表面在自己的容器上设置 `--dsh-scrollbar-thumb: var(--dsw-alias-scrollbar-bg-l2)` 与 `--dsh-scrollbar-thumb-hover: var(--dsw-alias-scrollbar-hover-l2)`，这一次重新绑定同时作用于标准属性和 WebKit 伪元素。这组变量必须成对重新绑定；只改静止态滑块会让 hover 状态仍留在基础表面的 token 上。可由机械检查发现的子集归 `packages/client/ui-theme/tests/scrollbar-styles.spec.ts` 所有：任何既滚动又绘制抬升表面的样式表都必须重新绑定，因此本 note 不再维护完整的表面清单。多数把这组变量声明在抬升卡片上而非滚动的后代元素上，因为抬升层级属于这个表面，而自定义属性会继承到真正滚动的那个子元素。
|
||||
|
||||
后四处在最初的实现里被漏掉、由评审发现，因此重新绑定契约现在由机械检查把关，而不再依赖人工审阅：一张样式表只要在某处滚动、又在某处绘制抬升表面，就必须重新绑定。
|
||||
`Menu`、`InputBar`、`QuestionComposer` 与 `TodoPanel` 这四个表面在最初的实现里被漏掉、由评审发现，因此逐样式表的重新绑定契约由机械检查而非人工审阅把关。
|
||||
|
||||
抬升表面集合是从调色板自身的暗色抬升阶梯解析出来的——暗色取值落在 `bg-layer-2` 或 `bg-layer-3` 上的那些表面 token，而这一档正是 l1/l2 之分所编码的层级差。最初的做法是从已经做了重新绑定的样式表反向推导，那是不成立的：这样得到的集合只能确认别人已经记得的部分，而尚无人重新绑定的表面——恰恰就是这项检查存在的理由——会把自己定义成「非抬升」。`--dsw-specific-tip` 证明了这一点：它解析到与菜单表面相同的那一档，待办面板在它上面滚动却没有重新绑定，而推导式的检查依然是绿的。
|
||||
|
||||
判定范围依据 token 家族而非几何形状：只有 `--dsw-alias-bg-*` 与 `--dsw-specific-*` 表述的是表面。`--dsw-alias-button-*`、`--dsw-alias-interactive-*` 与 `--dsw-alias-markdown-*` 会落到相同档位，但它们表述的是控件或行内片段，没有任何滚动容器会把滚动条画在它们之上。形状无法做这个判断，因为悬浮按钮本来就会带圆角、阴影和固定尺寸。这项检查以样式表为粒度而非以规则为粒度，因为卡片与真正滚动的后代元素是两条不同的规则，而 CSS 文本无法表达谁包含谁。
|
||||
判定范围依据 token 家族而非几何形状：只有 `--dsw-alias-bg-*` 与 `--dsw-specific-*` 表述的是表面。`--dsw-alias-button-*`、`--dsw-alias-interactive-*` 与 `--dsw-alias-markdown-*` 会落到相同档位，但它们表述的是控件或行内片段，没有任何滚动容器会把滚动条画在它们之上。形状无法做这个判断，因为悬浮按钮本来就会带圆角、阴影和固定尺寸。这项检查以样式表为粒度而非以规则为粒度，因为卡片与真正滚动的后代元素是两条不同的规则。这种近似检查无法检测嵌在由另一个包的样式表绘制的抬升卡片中的滚动组件，`Modal` 内的 `DirectoryBrowser` 就证明了这一点；跨样式表的组合仍需在评审和组装后 UI 层面把关。
|
||||
|
||||
轨道与两条滚动条相交的角落保持透明，因此滑块是以其下滚动的任何表面为背景被看到；只有滑块及其 hover 状态带 token 颜色。
|
||||
|
||||
|
||||
@@ -242,6 +242,10 @@
|
||||
.status,
|
||||
.error {
|
||||
padding: 4px;
|
||||
/* The loading pill occupies the opposite corner while a stale status stays
|
||||
* visible. Reserve its widest localized footprint so wrapped text cannot
|
||||
* run underneath it on a narrow card. */
|
||||
padding-right: 120px;
|
||||
font-size: 12px;
|
||||
line-height: 18px;
|
||||
}
|
||||
@@ -259,15 +263,15 @@
|
||||
* the columns' height, and the stale view keeps rendering beneath it (it
|
||||
* only appears at all once a scan outlives SLOW_SCAN_DELAY_MS). Right,
|
||||
* not left: the truncated/error status rows flow at the bottom LEFT and
|
||||
* stay on screen through a scan, so the opposite corner keeps both
|
||||
* legible. After .status in the cascade — the element carries both
|
||||
* classes and this padding must win the same-specificity race. */
|
||||
* stay on screen through a scan, with their reserved right padding keeping
|
||||
* both legible even on a narrow card. After .status in the cascade — the
|
||||
* element carries both classes and this padding must win the
|
||||
* same-specificity race. */
|
||||
.loadingFloat {
|
||||
position: absolute;
|
||||
right: 16px;
|
||||
bottom: 8px;
|
||||
padding: 2px 8px;
|
||||
border-radius: 6px;
|
||||
background: var(--dsw-alias-bg-layer-2);
|
||||
}
|
||||
|
||||
|
||||
@@ -186,10 +186,14 @@ export function DirectoryBrowser({ open, listDirectory, createDirectory, onOpen,
|
||||
const [selected, setSelected] = useState<DirectoryEntry | null>(null)
|
||||
const [child, setChild] = useState<DirectoryListing | null>(null)
|
||||
const [loading, setLoading] = useState(false)
|
||||
// Derived from `loading` by the slow-scan effect below: true only once a
|
||||
// scan has been in flight for SLOW_SCAN_DELAY_MS, so fast listings never
|
||||
// render the indicator at all.
|
||||
// Derived from `loading` and `scanWindow` by the slow-scan effect below:
|
||||
// true only once the current listing call has been in flight for
|
||||
// SLOW_SCAN_DELAY_MS, so fast listings never render the indicator at all.
|
||||
const [slowScan, setSlowScan] = useState(false)
|
||||
// Every listing call owns a fresh silence window. `loading` may stay true
|
||||
// across a superseding row pick or across a navigation's target and parent
|
||||
// legs, so its boolean edge cannot identify the start of each scan.
|
||||
const [scanWindow, setScanWindow] = useState(0)
|
||||
const [error, setError] = useState<string | null>(null)
|
||||
// Path-edit state: null = breadcrumb mode; a string = the draft being typed.
|
||||
const [pathDraft, setPathDraft] = useState<string | null>(null)
|
||||
@@ -233,13 +237,20 @@ export function DirectoryBrowser({ open, listDirectory, createDirectory, onOpen,
|
||||
return ++requestSeq.current
|
||||
}, [])
|
||||
|
||||
/** Hide any prior indicator and start a fresh silence window for one listing call. */
|
||||
const restartSlowScanWindow = useCallback((): void => {
|
||||
setSlowScan(false)
|
||||
setScanWindow(value => value + 1)
|
||||
}, [])
|
||||
|
||||
/** Launch one listing under a fresh controller so a later supersession can abort it. */
|
||||
const launchListing = useCallback((path: string | undefined): { seq: number; scan: Promise<DirectoryListing> } => {
|
||||
const seq = supersede()
|
||||
const controller = new AbortController()
|
||||
scanController.current = controller
|
||||
restartSlowScanWindow()
|
||||
return { seq, scan: listDirectory(path, controller.signal) }
|
||||
}, [supersede, listDirectory])
|
||||
}, [supersede, restartSlowScanWindow, listDirectory])
|
||||
|
||||
/**
|
||||
* Launch a follow-up listing under the CURRENT supersession seq: a newer
|
||||
@@ -248,8 +259,9 @@ export function DirectoryBrowser({ open, listDirectory, createDirectory, onOpen,
|
||||
const continueScan = useCallback((path: string): Promise<DirectoryListing> => {
|
||||
const controller = new AbortController()
|
||||
scanController.current = controller
|
||||
restartSlowScanWindow()
|
||||
return listDirectory(path, controller.signal)
|
||||
}, [listDirectory])
|
||||
}, [restartSlowScanWindow, listDirectory])
|
||||
|
||||
/**
|
||||
* Replace the whole view with a freshly navigated level. Away from the
|
||||
@@ -474,9 +486,10 @@ export function DirectoryBrowser({ open, listDirectory, createDirectory, onOpen,
|
||||
})
|
||||
}
|
||||
|
||||
// The slow-scan gate for the loading indicator: arm a timer when a scan
|
||||
// starts, retire it (and the indicator) the moment loading ends. A settle
|
||||
// inside the window means the swap happened with nothing shown.
|
||||
// The slow-scan gate for the loading indicator: each listing call restarts
|
||||
// the timer even when a superseding scan or a navigation's parent leg keeps
|
||||
// `loading` continuously true. A settle inside its own window means the swap
|
||||
// happened with nothing shown.
|
||||
useEffect(() => {
|
||||
if (!loading) {
|
||||
setSlowScan(false)
|
||||
@@ -484,7 +497,7 @@ export function DirectoryBrowser({ open, listDirectory, createDirectory, onOpen,
|
||||
}
|
||||
const timer = window.setTimeout(() => { setSlowScan(true) }, SLOW_SCAN_DELAY_MS)
|
||||
return () => { window.clearTimeout(timer) }
|
||||
}, [loading])
|
||||
}, [loading, scanWindow])
|
||||
|
||||
// After the hooks: a closed dialog renders nothing and evaluates no copy.
|
||||
const crumbSource = child ?? parent
|
||||
|
||||
@@ -111,6 +111,12 @@ function rowButton(item: HTMLElement): HTMLButtonElement {
|
||||
}
|
||||
|
||||
describe('DirectoryBrowser', () => {
|
||||
it('renders nothing and launches no listing while initially closed', () => {
|
||||
const b = mount({ open: false })
|
||||
expect(screen.queryByRole('dialog')).toBeNull()
|
||||
expect(b.listDirectory).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('opens at the Host home as one wide column, hides hidden entries, and roots the crumbs at Home', async () => {
|
||||
const b = mount()
|
||||
await waitFor(() => { expect(screen.getByRole('listitem')).toBeTruthy() })
|
||||
@@ -335,9 +341,16 @@ describe('DirectoryBrowser', () => {
|
||||
fireEvent.click(screen.getByRole('button', { name: 'browser.editPath' }))
|
||||
fireEvent.change(screen.getByLabelText<HTMLInputElement>('browser.editPath'), { target: { value: DOCS } })
|
||||
fireEvent.keyDown(screen.getByLabelText('browser.editPath'), { key: 'Enter' })
|
||||
// The target can consume most of the outer scan's silence window.
|
||||
await act(async () => { vi.advanceTimersByTime(250) })
|
||||
await act(async () => { settlers.get(DOCS)!(listingFor(DOCS)) })
|
||||
// Its parent leg gets a fresh silence window. Crossing the original
|
||||
// scan's 300ms deadline therefore cannot flash the indicator during the
|
||||
// bounded landing wait.
|
||||
await act(async () => { vi.advanceTimersByTime(199) })
|
||||
expect(screen.queryByText('browser.loading')).toBeNull()
|
||||
// The parent leg outlives PARENT_LEG_WAIT_MS: the target lands alone.
|
||||
await act(async () => { vi.advanceTimersByTime(200) })
|
||||
await act(async () => { vi.advanceTimersByTime(1) })
|
||||
expect(columns()).toHaveLength(1)
|
||||
expect(screen.getByRole('listitem').textContent).toBe('harness')
|
||||
expect(screen.queryByLabelText('browser.editPath', { selector: 'input' })).toBeNull()
|
||||
@@ -417,6 +430,34 @@ describe('DirectoryBrowser', () => {
|
||||
}
|
||||
})
|
||||
|
||||
it('restarts the silence window when a row pick supersedes a pending scan', async () => {
|
||||
vi.useFakeTimers()
|
||||
try {
|
||||
const pending: ((value: DirectoryListing) => void)[] = []
|
||||
const listDirectory = vi.fn((path?: string, _signal?: AbortSignal) => {
|
||||
if (path === undefined) return Promise.resolve(listingFor(path))
|
||||
return new Promise<DirectoryListing>((resolve) => { pending.push(resolve) })
|
||||
})
|
||||
mount({ listDirectory })
|
||||
await act(async () => {})
|
||||
const documents = rowButton(screen.getByRole('listitem'))
|
||||
fireEvent.click(documents)
|
||||
await act(async () => { vi.advanceTimersByTime(300) })
|
||||
expect(screen.getByText('browser.loading')).toBeTruthy()
|
||||
// The same row remains actionable while its preview is pending. A second
|
||||
// pick starts a new listing without a false `loading` edge.
|
||||
fireEvent.click(documents)
|
||||
expect(screen.queryByText('browser.loading')).toBeNull()
|
||||
await act(async () => { vi.advanceTimersByTime(299) })
|
||||
expect(screen.queryByText('browser.loading')).toBeNull()
|
||||
await act(async () => { vi.advanceTimersByTime(1) })
|
||||
expect(screen.getByText('browser.loading')).toBeTruthy()
|
||||
await act(async () => { pending.at(-1)!(listingFor(DOCS)) })
|
||||
} finally {
|
||||
vi.useRealTimers()
|
||||
}
|
||||
})
|
||||
|
||||
it('a close mid-scan resets the slow-scan gate: reopening waits a fresh silence window', async () => {
|
||||
vi.useFakeTimers()
|
||||
try {
|
||||
|
||||
Reference in New Issue
Block a user