Преглед изворни кода

fix(web): keep sidebar scrollbar inside inset

_Kerman пре 2 месеци
родитељ
комит
c1ac85c15f

+ 45 - 4
apps/web/tests/sidebar-scrollbar.e2e.ts

@@ -106,6 +106,8 @@ interface ListMetrics {
   overflows: boolean
   /** Border-box width minus client width: the space the scrollbar takes out of the content area. */
   band: number
+  /** Distance from the first row background's right edge to the list border box. */
+  rowEdgeInset: number
   /** Client-area right edge in viewport coordinates (`clientWidth` excludes the scrollbar band). */
   clientRight: number
   /** Border-box right edge in viewport coordinates. */
@@ -133,6 +135,8 @@ function measureList(page: Page): Promise<ListMetrics> {
     if (list === null) throw new Error('sidebar session list not in the DOM')
     const time = list.querySelector<HTMLElement>('[class*="time"]')
     if (time === null) throw new Error('no row relative-time element in the sidebar list')
+    const row = list.querySelector<HTMLElement>('[role="treeitem"]')
+    if (row === null) throw new Error('no row in the sidebar list')
     // Each indirection variable is resolved through its own throwaway probe
     // appended to the list: `var()` substitution then happens where the list
     // sits in the cascade, which is the claim, and `color` normalizes whatever
@@ -167,6 +171,7 @@ function measureList(page: Page): Promise<ListMetrics> {
     const style = getComputedStyle(list)
     const pseudoWidth = getComputedStyle(list, '::-webkit-scrollbar').width
     const barWidth = pseudoWidth === 'auto' ? 15 : Number.parseFloat(pseudoWidth)
+    const listRect = list.getBoundingClientRect()
     return {
       gutter: style.scrollbarGutter,
       width: pseudoWidth,
@@ -177,9 +182,10 @@ function measureList(page: Page): Promise<ListMetrics> {
       token: resolve('--dsh-scrollbar-thumb'),
       hoverToken: resolve('--dsh-scrollbar-thumb-hover'),
       overflows: list.scrollHeight > list.clientHeight,
-      band: list.getBoundingClientRect().width - list.clientWidth,
-      clientRight: list.getBoundingClientRect().left + list.clientWidth,
-      borderRight: list.getBoundingClientRect().right,
+      band: listRect.width - list.clientWidth,
+      rowEdgeInset: listRect.right - row.getBoundingClientRect().right,
+      clientRight: listRect.left + list.clientWidth,
+      borderRight: listRect.right,
       timeRight: time.getBoundingClientRect().right,
       // The bar is drawn in the rightmost `barWidth` of the border box, whether
       // or not that space was reserved. Its width comes from the sheet where the
@@ -188,7 +194,26 @@ function measureList(page: Page): Promise<ListMetrics> {
       // absent. Taking the UA width as the fallback is what keeps the assertion
       // honest: assuming 0 there would report no occlusion precisely in the
       // state that has it.
-      timeCoveredBy: Math.max(0, time.getBoundingClientRect().right - (list.getBoundingClientRect().right - barWidth)),
+      timeCoveredBy: Math.max(0, time.getBoundingClientRect().right - (listRect.right - barWidth)),
+    }
+  })
+}
+
+/**
+ * Measure only overflow and row inset, which remain observable when every
+ * session is hidden under a collapsed workspace group.
+ * @param page - the page under test.
+ * @returns the list overflow state and first row's trailing inset.
+ */
+function measureRowInset(page: Page): Promise<Pick<ListMetrics, 'overflows' | 'rowEdgeInset'>> {
+  return page.evaluate(() => {
+    const list = document.querySelector<HTMLElement>('[role="tree"][aria-label="Sessions"]')
+    if (list === null) throw new Error('sidebar session list not in the DOM')
+    const row = list.querySelector<HTMLElement>('[role="treeitem"]')
+    if (row === null) throw new Error('no row in the sidebar list')
+    return {
+      overflows: list.scrollHeight > list.clientHeight,
+      rowEdgeInset: list.getBoundingClientRect().right - row.getBoundingClientRect().right,
     }
   })
 }
@@ -222,6 +247,7 @@ function renderGeometry(light: ListMetrics, dark: ListMetrics): string {
     `- --dsh-scrollbar-thumb-hover: ${metrics.hoverToken}`,
     `- list overflows: ${String(metrics.overflows)}`,
     `- reserved band: ${String(metrics.band)}px`,
+    `- row background inset from the sidebar edge: ${String(metrics.rowEdgeInset)}px`,
     `- relative time covered by the bar: ${String(metrics.timeCoveredBy)}px`,
     `- relative time ends inside the content area: ${String(metrics.timeRight <= metrics.clientRight)}`,
     `- content area ends before the border box: ${String(metrics.clientRight < metrics.borderRight)}`,
@@ -299,6 +325,7 @@ describe('web e2e: sidebar session list scrollbar (reserved gutter / themed thum
     // drawn over it. Removing the declaration makes it exactly 0. The value
     // itself is not pinned — it tracks `scrollbar-width` and the platform.
     expect(metrics.band).toBeGreaterThan(0)
+    expect(metrics.rowEdgeInset).toBe(12)
     // The reported symptom, stated directly: no part of the row's relative time
     // lies under the bar. Measures 7 on clean master — the `h` of `1h` is the
     // covered part. Unlike the client-edge comparison below it does not go
@@ -317,6 +344,20 @@ describe('web e2e: sidebar session list scrollbar (reserved gutter / themed thum
     expect(tripwire.pageErrors).toEqual([])
   }, 60_000)
 
+  it('keeps the row background inset when overflow disappears', async () => {
+    onTestFailed(() => saveFailureShot(page, 'web-e2e-sidebar-scrollbar-stable-inset'))
+    expect(await measureRowInset(page)).toEqual({ overflows: true, rowEdgeInset: 12 })
+    const bucket = page.getByText('Ungrouped', { exact: true }).locator('..').locator('..')
+    await bucket.click()
+    try {
+      await expect.poll(async () => (await measureRowInset(page)).overflows, { timeout: 10_000 }).toBe(false)
+      expect(await measureRowInset(page)).toEqual({ overflows: false, rowEdgeInset: 12 })
+    } finally {
+      await expandSeededSessions(page)
+    }
+    expect(tripwire.pageErrors).toEqual([])
+  }, 60_000)
+
   it('renders the themed thumb through the WebKit path in both palettes', async () => {
     onTestFailed(() => saveFailureShot(page, 'web-e2e-sidebar-scrollbar-theme'))
     const light = await measureList(page)

+ 2 - 0
apps/web/tests/snapshots/sidebar-scrollbar/geometry.expected.md

@@ -12,6 +12,7 @@
 - --dsh-scrollbar-thumb-hover: rgb(212, 212, 212)
 - list overflows: true
 - reserved band: 8px
+- row background inset from the sidebar edge: 12px
 - relative time covered by the bar: 0px
 - relative time ends inside the content area: true
 - content area ends before the border box: true
@@ -28,6 +29,7 @@
 - --dsh-scrollbar-thumb-hover: rgb(84, 85, 87)
 - list overflows: true
 - reserved band: 8px
+- row background inset from the sidebar edge: 12px
 - relative time covered by the bar: 0px
 - relative time ends inside the content area: true
 - content area ends before the border box: true

+ 6 - 4
packages/client/ui-sidebar/src/client/SidebarRoot.module.css

@@ -7,10 +7,11 @@
    mid-slide. */
 
 .root {
+  --dsh-sidebar-inline-padding: 12px;
   display: flex;
   flex-direction: column;
   height: 100%;
-  padding: 6px 12px;
+  padding: 6px var(--dsh-sidebar-inline-padding);
   box-sizing: border-box;
   background: var(--dsw-specific-sidebar-fill);
   color: var(--dsw-alias-label-primary);
@@ -189,14 +190,15 @@
   max-width: 0;
 }
 
-/* Region seat: always mounted so the foot never moves; the browser inside
-   handles its own wide/rail content. */
+/* Region seat: always mounted so the foot never moves. Its trailing margin
+   cancels the wide shell inset so the nested scrollbar can sit at the sidebar
+   edge; the browser restores that inset inside its own rows. */
 .regionArea {
   flex: 1;
   min-height: 0;
   display: flex;
   flex-direction: column;
-  margin-right: -12px;
+  margin-right: calc(-1 * var(--dsh-sidebar-inline-padding));
   overflow: hidden;
 }
 

+ 38 - 0
packages/client/ui-sidebar/tests/sidebar-styles.spec.ts

@@ -0,0 +1,38 @@
+/** Sidebar shell inset contract shared with the nested workspace browser. */
+import { readFileSync } from 'node:fs'
+import { fileURLToPath } from 'node:url'
+import { describe, expect, it } from 'vitest'
+
+const css = readFileSync(fileURLToPath(new URL('../src/client/SidebarRoot.module.css', import.meta.url)), 'utf8')
+
+/**
+ * Declarations of one exact selector, keyed by property.
+ * @param selector - exact selector text.
+ * @returns the normalized declarations, or undefined when absent.
+ */
+function declarations(selector: string): Map<string, string> | undefined {
+  const withoutComments = css.replace(/\/\*[\s\S]*?\*\//g, ' ')
+  for (const [, selectorList = '', body = ''] of withoutComments.matchAll(/([^{}]+)\{([^{}]*)\}/g)) {
+    if (!selectorList.split(',').map(value => value.trim()).includes(selector)) continue
+    const found = new Map<string, string>()
+    for (const part of body.split(';')) {
+      const colon = part.indexOf(':')
+      if (colon === -1) continue
+      found.set(part.slice(0, colon).trim(), part.slice(colon + 1).trim().replace(/\s+/g, ' '))
+    }
+    return found
+  }
+  return undefined
+}
+
+describe('SidebarRoot.module.css inset', () => {
+  it('shares and cancels the wide shell trailing padding structurally', () => {
+    const root = declarations('.root')
+    expect(root?.get('--dsh-sidebar-inline-padding')).toBe('12px')
+    expect(root?.get('padding')).toBe('6px var(--dsh-sidebar-inline-padding)')
+    expect(declarations('.regionArea')?.get('margin-right')).toBe(
+      'calc(-1 * var(--dsh-sidebar-inline-padding))',
+    )
+    expect(declarations('.collapsed .regionArea')?.get('margin-right')).toBe('0')
+  })
+})

+ 10 - 16
packages/client/ui-workspace/src/client/WorkspaceBrowser.module.css

@@ -4,12 +4,14 @@
    rail state renders only the two 36x36 icon controls. */
 
 .root {
+  --dsh-session-list-edge-inset: var(--dsh-sidebar-inline-padding);
+  --dsh-session-list-scrollbar-width: 8px;
   flex: 1;
   min-height: 0;
   display: flex;
   flex-direction: column;
   box-sizing: border-box;
-  padding-right: 12px;
+  padding-right: var(--dsh-session-list-edge-inset);
 }
 
 .root.rail {
@@ -173,7 +175,7 @@
   min-height: 0;
   display: flex;
   flex-direction: column;
-  margin-right: -12px;
+  margin-right: calc(-1 * var(--dsh-session-list-edge-inset));
   overflow: hidden;
 }
 
@@ -195,7 +197,7 @@
 .fade {
   position: absolute;
   left: 0;
-  right: 0;
+  right: var(--dsh-session-list-edge-inset);
   bottom: 0;
   height: 72px;
   background: linear-gradient(to bottom, transparent, var(--dsw-specific-sidebar-fill));
@@ -211,27 +213,19 @@
   from { opacity: 0; }
 }
 
-/* List: the only scrolling region. Block, not a flex column: as flex items
-   the 54/34 rows would shrink under content overflow; block children keep
-   their design heights do not shrink under content overflow. Its right
-   padding is the sole gap between row backgrounds and the sidebar edge; the
-   outer seats move their clip boundary out so they add no hidden inset. */
+/* List: the only scrolling region. Block children keep their design heights
+   under content overflow. The stable 8px themed scrollbar and the remaining
+   padding together equal the shell's right inset, with or without overflow. */
 .list {
-  --dsh-session-list-edge-inset: 12px;
-  --dsh-session-list-scrollbar-width: 8px;
   flex: 1;
   min-height: 0;
   overflow-y: auto;
-  padding-right: var(--dsh-session-list-edge-inset);
+  padding-right: calc(var(--dsh-session-list-edge-inset) - var(--dsh-session-list-scrollbar-width));
   padding-bottom: 12px;
   scrollbar-gutter: stable;
 }
 
-.list::-webkit-scrollbar {
-  width: var(--dsh-session-list-scrollbar-width);
-}
-
-.list > [role='treeitem'] + [role='treeitem'],
+.flatList > * + *,
 .searchTree > [role='treeitem'] + [role='treeitem'],
 .groupSection > * + * {
   margin-top: 2px;

+ 1 - 1
packages/client/ui-workspace/src/client/WorkspaceBrowser.tsx

@@ -238,7 +238,7 @@ function FlatList({ useSessions, open, forkSession, onSessionRename, onSessionAr
   const now = Date.now()
   return (
     <div className={clsx(css.treeBody, css.wide)}>
-      <div className={css.list} role="tree" aria-label={t('section.sessions')}>
+      <div className={clsx(css.list, css.flatList)} role="tree" aria-label={t('section.sessions')}>
         {rows.length === 0 && (
           <div className={css.empty}>{t('empty.none')}</div>
         )}

+ 37 - 27
packages/client/ui-workspace/tests/browser-styles.spec.ts

@@ -1,7 +1,7 @@
 /**
- * WorkspaceBrowser scroll-region style contract, asserted against the CSS text
- * on disk: the session list keeps one stable right inset for row hover fills,
- * with or without overflow, while outer clip seats add no hidden second inset.
+ * WorkspaceBrowser spacing contract, asserted against the CSS text on disk:
+ * row fills share the shell's trailing inset, the stable scrollbar counts
+ * inside it, and flat, grouped, and search views keep their intended rhythm.
  */
 import { readFileSync } from 'node:fs'
 import { fileURLToPath } from 'node:url'
@@ -10,46 +10,56 @@ import { describe, expect, it } from 'vitest'
 const css = readFileSync(fileURLToPath(new URL('../src/client/WorkspaceBrowser.module.css', import.meta.url)), 'utf8')
 
 /**
- * Declarations of one class rule, keyed by property with whitespace collapsed.
+ * Declarations of one selector rule, keyed by property with whitespace collapsed.
  * Declaration order and trailing semicolons are normalized away.
- * @param className - local class name, without the leading dot.
+ * @param selector - one exact selector, including a leading dot for local classes.
  * @returns the rule's declarations, or undefined when no such rule exists.
  */
-function declarations(className: string): Map<string, string> | undefined {
+function declarations(selector: string): Map<string, string> | undefined {
   const withoutComments = css.replace(/\/\*[\s\S]*?\*\//g, ' ')
-  const match = new RegExp(String.raw`(^|[\s,}])\.${className}\s*\{([^{}]*)\}`).exec(withoutComments)
-  if (match === null) return undefined
-  const found = new Map<string, string>()
-  // The body group is unconditional in the pattern; the fallback only satisfies
-  // noUncheckedIndexedAccess.
-  for (const part of (match[2] ?? '').split(';')) {
-    const colon = part.indexOf(':')
-    if (colon === -1) continue
-    found.set(part.slice(0, colon).trim(), part.slice(colon + 1).trim().replace(/\s+/g, ' '))
+  for (const [, selectorList = '', body = ''] of withoutComments.matchAll(/([^{}]+)\{([^{}]*)\}/g)) {
+    if (!selectorList.split(',').map(value => value.trim()).includes(selector)) continue
+    const found = new Map<string, string>()
+    for (const part of body.split(';')) {
+      const colon = part.indexOf(':')
+      if (colon === -1) continue
+      found.set(part.slice(0, colon).trim(), part.slice(colon + 1).trim().replace(/\s+/g, ' '))
+    }
+    return found
   }
-  return found
+  return undefined
 }
 
 describe('WorkspaceBrowser.module.css list', () => {
-  const root = declarations('root')
-  const listArea = declarations('listArea')
-  const list = declarations('list')
-  const treeBody = declarations('treeBody')
+  const root = declarations('.root')
+  const listArea = declarations('.listArea')
+  const list = declarations('.list')
 
   it('is the scrolling region', () => {
     expect(list).toBeDefined()
     expect(list!.get('overflow-y')).toBe('auto')
   })
 
-  it('keeps row backgrounds edge-flush with the scrolling region', () => {
-    expect(root?.get('padding-right')).toBe('12px')
-    expect(listArea?.get('margin-right')).toBe('-12px')
-    expect(treeBody?.get('margin-right')).toBeUndefined()
-    expect(list?.get('margin-right')).toBeUndefined()
-    expect(list?.get('padding-right')).toBe('var(--dsh-session-list-edge-inset)')
+  it('counts the themed scrollbar inside the shell trailing inset', () => {
+    expect(root?.get('--dsh-session-list-edge-inset')).toBe('var(--dsh-sidebar-inline-padding)')
+    expect(root?.get('--dsh-session-list-scrollbar-width')).toBe('8px')
+    expect(root?.get('padding-right')).toBe('var(--dsh-session-list-edge-inset)')
+    expect(listArea?.get('margin-right')).toBe('calc(-1 * var(--dsh-session-list-edge-inset))')
+    expect(declarations('.fade')?.get('right')).toBe('var(--dsh-session-list-edge-inset)')
+    expect(list?.get('padding-right')).toBe(
+      'calc(var(--dsh-session-list-edge-inset) - var(--dsh-session-list-scrollbar-width))',
+    )
+    expect(declarations('.list::-webkit-scrollbar')).toBeUndefined()
   })
 
-  it('reserves the scrollbar inside the stable visual inset', () => {
+  it('reserves the scrollbar whether or not the list overflows', () => {
     expect(list!.get('scrollbar-gutter')).toBe('stable')
   })
+
+  it('keeps 2px between rows and 4px between workspace groups', () => {
+    expect(declarations('.flatList > * + *')?.get('margin-top')).toBe('2px')
+    expect(declarations(".searchTree > [role='treeitem'] + [role='treeitem']")?.get('margin-top')).toBe('2px')
+    expect(declarations('.groupSection > * + *')?.get('margin-top')).toBe('2px')
+    expect(declarations('.groupSection + .groupSection')?.get('margin-top')).toBe('4px')
+  })
 })