Skip to content

Commit 9e9dc54

Browse files
authored
fix(emcn): stop the tab strip scrolling vertically by a pixel (#6113)
* fix(emcn): stop the tab strip scrolling vertically by a pixel The active tab extends one pixel past the strip so it covers the bottom border and reads as joined to the panel below. That pixel came from `-mb-px` on the tab itself, which meant it overflowed the row containing it — and that row is a scroll container, because `overflow-x: auto` computes a visible `overflow-y` to `auto` as well. The strip was therefore scrollable on the y axis by exactly one pixel, which is enough for a trackpad to nudge the tabs out of view. Moving the overlap onto the row keeps the tabs flush inside it, so the scroll container has nothing to scroll, while the row itself still hangs the pixel over the border. The strip is unchanged visually. Measured in a real renderer with the component's own classes: the row was scrollHeight 30 against clientHeight 29, and is now 30 against 30. A first attempt to reproduce it with hand-written inline styles showed no overflow at all — the real class output was needed to see it. Shared by the browser and terminal panels, so both stop scrolling. * test(emcn): pin the invariant, not the class placement Review pointed out the guard read source strings rather than rendered geometry, so it could pass while the overflow returned. Correct, and worth being precise about the compromise: this package's vitest runs in `node` with no browser mode, and the repo's only Playwright harness drives Electron against static HTML fixtures, so nothing here can lay out a React tree against the compiled CSS. Adding that capability is real infrastructure, not a line in this fix. What the guard can do is pin the invariant instead of two particular class strings: no negative bottom margin anywhere inside the scrolling row. That now catches the regression wherever a descendant reintroduces it, rather than only on the tab button it came from — verified against both shapes. The rendered behaviour stays measured rather than asserted, and the numbers are recorded next to the guard.
1 parent 7293b67 commit 9e9dc54

2 files changed

Lines changed: 38 additions & 2 deletions

File tree

packages/emcn/src/components/tab-strip/tab-strip.test.ts

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { readFileSync } from 'node:fs'
12
import { describe, expect, it } from 'vitest'
23
import { isTabTitleTruncated, type TabStripItem, tabDropIndex } from './tab-strip'
34

@@ -78,3 +79,30 @@ describe('tab tooltips', () => {
7879
expect(tooltipFor(tab, false)).toBe('GitHub')
7980
})
8081
})
82+
83+
describe('tab strip vertical overflow', () => {
84+
const source = readFileSync(new URL('./tab-strip.tsx', import.meta.url), 'utf8')
85+
/** Declarations only — the comments here discuss the very class they removed. */
86+
const markup = source.replace(/\/\*[\s\S]*?\*\//g, '').replace(/^\s*\/\/.*$/gm, '')
87+
88+
/**
89+
* A source-level guard, which is weaker than asserting rendered geometry and is a
90+
* deliberate compromise: this package's vitest runs in `node`, has no browser mode, and
91+
* the repo's only Playwright harness drives Electron against static HTML fixtures, so
92+
* nothing here can lay out a React tree against the compiled CSS. The behaviour was
93+
* instead measured directly in a renderer — the row went from scrollHeight 30 against
94+
* clientHeight 29 to 30 against 30 — and this guard pins the invariant that produced it.
95+
*/
96+
it('keeps every negative bottom margin outside the horizontally scrolling row', () => {
97+
// The active tab has to hang one pixel over the strip's bottom border. Anything inside
98+
// the row that does so overflows it, and `overflow-x: auto` computes the visible
99+
// `overflow-y` to `auto` as well — so the strip became scrollable by that one pixel.
100+
// Asserting the count, not just the row's own class, catches the regression wherever a
101+
// descendant reintroduces it rather than only on the tab it came from originally.
102+
const negativeBottomMargins = markup.match(/-mb-px/g) ?? []
103+
expect(negativeBottomMargins).toHaveLength(1)
104+
105+
const scrollRow = markup.match(/className='([^']*overflow-x-auto[^']*)'/)?.[1] ?? ''
106+
expect(scrollRow).toContain('-mb-px')
107+
})
108+
})

packages/emcn/src/components/tab-strip/tab-strip.tsx

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -180,7 +180,7 @@ function Tab({
180180
aria-current={tab.active ? 'page' : undefined}
181181
aria-label={tab.pinned ? tab.title : undefined}
182182
className={cn(
183-
'-mb-px h-[30px] w-full select-none rounded-b-none border border-transparent border-b-0 bg-transparent py-0 font-normal text-caption',
183+
'h-[30px] w-full select-none rounded-b-none border border-transparent border-b-0 bg-transparent py-0 font-normal text-caption',
184184
tab.pinned ? 'justify-center px-0' : 'justify-start gap-1.5 px-2',
185185
closeable && !tab.pinned && 'pr-7',
186186
tab.active &&
@@ -317,8 +317,16 @@ export function TabStrip({
317317
and scrolls horizontally instead of growing, which pins the button back
318318
at the right edge rather than pushing it out of view.
319319
*/}
320+
{/*
321+
`-mb-px` sits on this row rather than on the tabs inside it. The active tab has to
322+
extend one pixel past the strip to cover its bottom border, and while that pixel
323+
came from the tab it overflowed THIS element — which is a scroll container, since
324+
`overflow-x: auto` computes the visible `overflow-y` to `auto` as well. The result
325+
was a tab strip you could scroll vertically by exactly one pixel. Pulling the whole
326+
row down instead keeps the tabs flush inside it, so there is nothing to scroll.
327+
*/}
320328
<div
321-
className='flex min-w-0 shrink select-none items-end gap-0.5 overflow-x-auto [scrollbar-width:none] [&::-webkit-scrollbar]:hidden'
329+
className='-mb-px flex min-w-0 shrink select-none items-end gap-0.5 overflow-x-auto [scrollbar-width:none] [&::-webkit-scrollbar]:hidden'
322330
onDragOver={(event) => {
323331
if (draggedIdRef.current) event.preventDefault()
324332
}}

0 commit comments

Comments
 (0)