Skip to content

Commit 76e6992

Browse files
authored
Merge pull request #346 from quarto-dev/bugfix/bd-pvcnea83-edit-chrome-top-crop
Fix edit chrome cropped at top of viewport for first/top block (bd-pvcnea83)
2 parents b8fb38b + 7f6aebe commit 76e6992

9 files changed

Lines changed: 394 additions & 18 deletions

File tree

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
1+
# Fix: edit chrome cropped at the top of the viewport
2+
3+
**Strand:** bd-pvcnea83
4+
**Date:** 2026-06-25
5+
**Status:** in progress
6+
7+
## Problem (confirmed in the binary)
8+
9+
Editing the first block of a title-less document (or any block scrolled near the
10+
viewport top) crops the floating edit chrome. The rich-text toolbar is
11+
`position:absolute; bottom:100%; margin-bottom:4px` (floats *above* the edit
12+
box); for a block flush against the top of the scroll area it lands at negative
13+
`top` (above the iframe viewport, `scrollTop` already 0) and is clipped — only a
14+
sliver shows. Measured: editor `top:15`, toolbar `top:-15.4 → bottom:11`
15+
(height 26.4). The inline nesting breadcrumb lives in the toolbar, so it crops
16+
too. The standalone `BreadcrumbChip` (non-rich blocks at the top) has the same
17+
top-crop: its geometry sets `top = surfaceTop − chipH`, negative for a top
18+
surface.
19+
20+
## Approach: collision-aware flip (above → below)
21+
22+
Standard floating-toolbar behavior. When there isn't room above
23+
(`surfaceTop − chromeHeight − gap < 0`, viewport-relative), render the chrome
24+
**below** the block instead of above. While editing a top block the chrome then
25+
sits just under it (briefly over the next block), fully visible; the edited text
26+
is never covered. Parity-neutral (no change to the rendered/preview document
27+
spacing). Generalizes correctly to any near-top block, not just the literal
28+
first one.
29+
30+
Shared pure helper (mirrors the chip's existing `computeChipGeometry` pattern):
31+
32+
```ts
33+
// editChromeGeometry.ts
34+
export function shouldPlaceChromeBelow(surfaceTop: number, chromeHeight: number, gap: number): boolean {
35+
return surfaceTop - chromeHeight - gap < 0;
36+
}
37+
```
38+
39+
- **Toolbar** (`RichTextToolbar`): `useLayoutEffect` measures the
40+
`.q2-richtext-editor` box's viewport top + the toolbar's height; if it would
41+
clip above, set a `q2-rt-toolbar-below` class (`top:100%; bottom:auto;
42+
margin-top:4px`). Guard: skip when `offsetHeight <= 0` (degenerate jsdom rects)
43+
so the default `above` placement is kept there.
44+
- **Chip** (`BreadcrumbChip`): in the geometry effect, when
45+
`shouldPlaceChromeBelow(sRect.top, chipH, GAP)` (and real geometry —
46+
`sRect.height > 0`), set `top = (sRect.bottom − hostRect.top) + GAP` (below)
47+
instead of `surfaceTop − chipH` (above). Horizontal left-spill geometry is
48+
unchanged.
49+
50+
Both compute placement from the *surface's* stable top (not the chrome's flipped
51+
position), so there is no flip/re-measure loop.
52+
53+
## Tests (TDD)
54+
55+
1. **Unit (RED first):** `editChromeGeometry.test.ts``shouldPlaceChromeBelow`
56+
true for `(15, 26, 4)` (the measured case), false for `(100, 26, 4)`, and
57+
boundary cases.
58+
2. **Real-binary e2e:** title-less fixture; edit the first block →
59+
- rich Para: `.q2-rt-toolbar` has `q2-rt-toolbar-below` and its rect top ≥ 0
60+
(not clipped);
61+
- first block is a code block (non-rich): standalone chip rect top ≥ 0
62+
(placed below).
63+
Follow the established floating-chrome testing pattern (pure unit + real e2e;
64+
jsdom rects are degenerate, so vertical placement is not asserted in jsdom).
65+
66+
## Notes / known interactions
67+
68+
- The hub-client `q2-preview-breadcrumb-geometry` e2e asserts "chip above
69+
surface" for a TOP paragraph. That suite is already red (bd-fpys25b0,
70+
rich-text default-on) and will need its top-block expectation updated to the
71+
flipped (below) placement when bd-fpys25b0 is addressed. Out of scope here;
72+
noted on that strand.
73+
74+
## Work items
75+
76+
- [x] `editChromeGeometry.ts` + unit test (`shouldPlaceChromeBelow`, 5 cases)
77+
- [x] Toolbar: `useLayoutEffect` measure + `q2-rt-toolbar-below` class + CSS
78+
- [x] Chip: flip `top` below when clipped (guarded on `sRect.height > 0`)
79+
- [x] preview-renderer unit (505) + integration (515) suites green
80+
- [x] Real-binary e2e on title-less fixtures (toolbar + chip),
81+
`q2-preview-spa/e2e/edit-chrome-placement.spec.ts` (2 tests); full spa e2e
82+
suite green (39). Verified live in Chrome: toolbar flips below (top 48.5,
83+
uncropped) for the first paragraph; chip flips below (top 83.8, uncropped)
84+
for a first code block.
85+
- [x] hub-client build + unit (662) + integration (76) green;
86+
`cargo xtask verify --skip-hub-build` green (one flaky pampa-oracle spike
87+
failure on first run, passed on re-run — unrelated to this change)
88+
- [ ] hub-client/changelog.md (two-commit) — pending commit

hub-client/changelog.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ be in reverse chronological order (latest first).
1717

1818
### 2026-06-25
1919

20+
- [`d6066dc9`](https://github.com/quarto-dev/q2/commits/d6066dc9): The editing toolbar (and breadcrumb navigator) no longer gets cut off when you edit the very first block of a document with no title — it now flips below the block when there isn't room above.
2021
- [`15b9287c`](https://github.com/quarto-dev/q2/commits/15b9287c): The block-hierarchy navigator (◀ Dv ¶ ▶) now sits inline to the right of the rich-text formatting buttons instead of overlapping them, and `q2 preview --allow-edit` shows the navigator by default (matching the hub editor).
2122

2223
### 2026-06-24

hub-client/e2e/q2-preview-breadcrumb-geometry.spec.ts

Lines changed: 94 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -65,11 +65,14 @@ test.describe('Phase 4 — Breadcrumb chip geometry (real browser)', () => {
6565
});
6666

6767
// -------------------------------------------------------------------------
68-
// Existing vertical test (P3.5 tier i item 1) — kept byte-for-byte.
69-
// Guards: chip sits above the active edit surface, never occluding line 1.
68+
// Vertical placement at the top of the document (P3.5 tier i item 1).
69+
// The first block is flush against the viewport top, so there is no room
70+
// ABOVE it for the chip — it must flip BELOW the edit surface rather than be
71+
// clipped above the scroll area (bd-pvcnea83). Guards: chip flipped below,
72+
// anchored near the surface bottom, and never clipped above the viewport.
7073
// -------------------------------------------------------------------------
7174

72-
test('chip sits above the active edit surface, never occluding line 1', async ({ page }) => {
75+
test('chip flips below the edit surface at the document top (uncropped, never occluding the edited text)', async ({ page }) => {
7376
// Enable the nesting-cursor BEFORE any navigation — addInitScript re-applies
7477
// on every document load, including the iframe.
7578
await page.addInitScript(() => {
@@ -129,30 +132,33 @@ test.describe('Phase 4 — Breadcrumb chip geometry (real browser)', () => {
129132
// TypeScript narrowing (already asserted above).
130133
if (!chipBox || !taBox) throw new Error('impossible — asserted above');
131134

135+
const surfaceBottom = taBox.y + taBox.height;
132136
console.log(
133137
`Chip box: y=${chipBox.y.toFixed(2)}, bottom=${(chipBox.y + chipBox.height).toFixed(2)}, height=${chipBox.height.toFixed(2)}\n` +
134-
`Textarea box: y=${taBox.y.toFixed(2)}`,
138+
`Textarea box: y=${taBox.y.toFixed(2)}, bottom=${surfaceBottom.toFixed(2)}`,
135139
);
136140

137-
// Step 5: assertions.
141+
// Step 5: assertions (bd-pvcnea83 — flipped-below placement at the top).
138142
const TOL = 2;
139143

140-
// (a) Chip bottom is AT OR ABOVE surface top — never occludes line 1.
144+
// (a) Chip top is AT OR BELOW the surface bottom — the chip flipped below the
145+
// edit box (no room above at the document top), so it never overlaps the
146+
// edited text.
141147
expect(
142-
chipBox.y + chipBox.height,
143-
`chip bottom (${(chipBox.y + chipBox.height).toFixed(2)}) must be ≤ textarea top (${taBox.y.toFixed(2)}) + ${TOL}px tolerance`,
144-
).toBeLessThanOrEqual(taBox.y + TOL);
148+
chipBox.y,
149+
`chip top (${chipBox.y.toFixed(2)}) must be ≥ surface bottom (${surfaceBottom.toFixed(2)}) ${TOL}px — flipped below, not overlapping the surface`,
150+
).toBeGreaterThanOrEqual(surfaceBottom - TOL);
145151

146-
// (b) Chip bottom is anchored close to the surface top (not floating far above).
152+
// (b) Chip is anchored close to the surface bottom (not floating far below).
147153
// The gap must be less than ~one chip-gap (12 px) so the chip is still
148154
// visually attached and the useLayoutEffect positioning is doing real work.
149155
expect(
150-
taBox.y - (chipBox.y + chipBox.height),
151-
`chip must be anchored near the surface top — gap (${(taBox.y - (chipBox.y + chipBox.height)).toFixed(2)}px) must be < 12px`,
156+
chipBox.y - surfaceBottom,
157+
`chip must be anchored near the surface bottom — gap (${(chipBox.y - surfaceBottom).toFixed(2)}px) must be < 12px`,
152158
).toBeLessThan(12);
153159

154-
// (c) At the document top, the chip sits in the page margin (not clipped above
155-
// the viewport — `top` is clamped to ≥ 0 in real CSS).
160+
// (c) The chip is not clipped above the viewport top (the whole point of the
161+
// flip — `top` is ≥ 0 in real CSS).
156162
expect(
157163
chipBox.y,
158164
`chip top (${chipBox.y.toFixed(2)}) must be ≥ 0 — not clipped above the viewport`,
@@ -162,6 +168,80 @@ test.describe('Phase 4 — Breadcrumb chip geometry (real browser)', () => {
162168
await iframe.locator('textarea').first().press('Escape');
163169
});
164170

171+
// -------------------------------------------------------------------------
172+
// Companion to the above: the flip is CONDITIONAL. When a block has room
173+
// above it (not at the document top), the chip stays in its default ABOVE
174+
// placement — proving bd-pvcnea83 only flips when there isn't room, rather
175+
// than always rendering below.
176+
// -------------------------------------------------------------------------
177+
178+
test('chip stays above the edit surface when there is room above (non-top block)', async ({ page }) => {
179+
await page.addInitScript(() => {
180+
localStorage.setItem('quarto-hub:preferences', JSON.stringify({
181+
version: 1,
182+
scrollSyncEnabled: true,
183+
errorOverlayCollapsed: true,
184+
colorScheme: 'auto',
185+
unlockNestingCursor: true,
186+
}));
187+
});
188+
189+
const serverUrl = getServerUrl();
190+
// A title plus several paragraphs gives a later paragraph ample headroom
191+
// above it, so the chip is NOT clipped and keeps its default above placement.
192+
const QMD = [
193+
'---',
194+
'title: With a title',
195+
'format: q2-preview',
196+
'---',
197+
'',
198+
'Para one.',
199+
'',
200+
'Para two.',
201+
'',
202+
'Para three.',
203+
'',
204+
'Para four.',
205+
'',
206+
].join('\n');
207+
208+
const docId = await createProjectOnServer(serverUrl, [
209+
{ path: '_quarto.yml', content: 'project:\n type: default\n', contentType: 'text' },
210+
{ path: 'breadcrumb-above.qmd', content: QMD, contentType: 'text' },
211+
]);
212+
213+
const iframe = await openFile(page, serverUrl, docId, 'breadcrumb-above.qmd');
214+
215+
// Edit a LATER paragraph (index 2 → "Para three.") — well below the top.
216+
await iframe.locator('p[data-block-pool-id]').nth(2).click();
217+
await iframe.locator('textarea').first().waitFor({ timeout: 10_000 });
218+
const chip = iframe.locator('[data-testid="q2-breadcrumb-chip"]');
219+
await chip.waitFor({ timeout: 5000 });
220+
221+
const chipBox = await chip.boundingBox();
222+
const taBox = await iframe.locator('textarea').first().boundingBox();
223+
if (!chipBox || !taBox) throw new Error('chip/textarea bounding box must be non-null');
224+
225+
console.log(
226+
`Chip box: y=${chipBox.y.toFixed(2)}, bottom=${(chipBox.y + chipBox.height).toFixed(2)}\n` +
227+
`Textarea box: y=${taBox.y.toFixed(2)}`,
228+
);
229+
230+
const TOL = 2;
231+
// Default ABOVE placement: chip bottom sits at or above the surface top.
232+
expect(
233+
chipBox.y + chipBox.height,
234+
`chip bottom (${(chipBox.y + chipBox.height).toFixed(2)}) must be ≤ surface top (${taBox.y.toFixed(2)}) + ${TOL}px — default above placement`,
235+
).toBeLessThanOrEqual(taBox.y + TOL);
236+
// Anchored near the surface top.
237+
expect(
238+
taBox.y - (chipBox.y + chipBox.height),
239+
`chip must be anchored near the surface top — gap (${(taBox.y - (chipBox.y + chipBox.height)).toFixed(2)}px) must be < 12px`,
240+
).toBeLessThan(12);
241+
242+
await iframe.locator('textarea').first().press('Escape');
243+
});
244+
165245
// -------------------------------------------------------------------------
166246
// 4a — DESIGN GO/NO-GO GATE (not a regression guard).
167247
//
Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
/**
2+
* bd-pvcnea83 — floating edit chrome must not be cropped at the top of the
3+
* viewport. Editing the first block of a title-less document (flush against the
4+
* scroll-area top) previously clipped the chrome, which floats ABOVE the edit
5+
* box (`bottom:100%`), above the viewport top with no way to scroll up to it.
6+
* The fix flips the chrome BELOW the block when there's no room above.
7+
*
8+
* Real-binary e2e (drives target/debug/q2 via startPreviewServer). The pure
9+
* placement threshold is unit-tested in
10+
* ts-packages/preview-renderer/src/q2-preview/editChromeGeometry.test.ts; jsdom
11+
* rects are degenerate, so the actual flip is only observable here.
12+
*
13+
* Build chain prerequisite (the binary does NOT auto-rebuild the embedded SPA):
14+
* cargo xtask build-q2-preview-spa
15+
* cargo build -p quarto --bin q2
16+
*/
17+
18+
import { test, expect, type Page } from '@playwright/test';
19+
import { startPreviewServer, type PreviewServerHandle } from './helpers/previewServer';
20+
21+
let server: PreviewServerHandle;
22+
23+
/** Viewport-relative top of an element inside the preview iframe (0 = top of the
24+
* visible scroll area). A clipped-above-the-top element has a negative top. */
25+
async function iframeRectTop(page: Page, selector: string): Promise<number> {
26+
return page.frameLocator('iframe').locator(selector).first().evaluate(
27+
(el) => el.getBoundingClientRect().top,
28+
);
29+
}
30+
31+
test.describe('bd-pvcnea83 — edit chrome flips below at the top of the viewport', () => {
32+
test.setTimeout(120_000);
33+
34+
test.afterEach(async () => {
35+
await server?.stop();
36+
});
37+
38+
test('rich-text toolbar: editing the first (title-less) paragraph flips the toolbar below, uncropped', async ({ page }) => {
39+
server = await startPreviewServer({
40+
allowEdit: true,
41+
fixtureFiles: [{
42+
path: 'index.qmd',
43+
// No title front matter: the first paragraph is flush against the top.
44+
content: 'First paragraph at the very top.\n\nSecond paragraph.\n',
45+
}],
46+
});
47+
await page.goto(server.url);
48+
const iframe = page.frameLocator('iframe');
49+
await page.waitForFunction(() => {
50+
const inner = document.querySelector('iframe')?.contentDocument;
51+
return inner?.querySelector('p[data-block-pool-id]') != null;
52+
}, null, { timeout: 30_000 });
53+
54+
await iframe.locator('p[data-block-pool-id]').first().click();
55+
56+
const toolbar = iframe.locator('.q2-rt-toolbar').first();
57+
await toolbar.waitFor({ timeout: 10_000 });
58+
59+
// Flipped below (the collision-avoidance class) ...
60+
await expect(toolbar, 'toolbar must flip below at the top of the document')
61+
.toHaveClass(/q2-rt-toolbar-below/);
62+
// ... and consequently not cropped above the viewport top.
63+
await expect
64+
.poll(() => iframeRectTop(page, '.q2-rt-toolbar'), {
65+
timeout: 8000,
66+
message: 'toolbar top must be >= 0 (not clipped above the viewport)',
67+
})
68+
.toBeGreaterThanOrEqual(0);
69+
});
70+
71+
test('standalone breadcrumb chip: editing a first (title-less) code block flips the chip below, uncropped', async ({ page }) => {
72+
server = await startPreviewServer({
73+
allowEdit: true,
74+
fixtureFiles: [{
75+
path: 'index.qmd',
76+
// First block is a code block (non-rich) → textarea + standalone chip.
77+
content: '```python\nx = 1\ny = 2\n```\n\nA paragraph after.\n',
78+
}],
79+
});
80+
await page.goto(server.url);
81+
const iframe = page.frameLocator('iframe');
82+
await page.waitForFunction(() => {
83+
const inner = document.querySelector('iframe')?.contentDocument;
84+
return inner?.querySelector('[data-block-pool-id]') != null;
85+
}, null, { timeout: 30_000 });
86+
87+
await iframe.locator('[data-block-pool-id]').first().click();
88+
await iframe.locator('#q2-active-edit-region textarea').first().waitFor({ timeout: 10_000 });
89+
90+
const chip = iframe.locator('[data-testid="q2-breadcrumb-chip"]');
91+
await chip.waitFor({ timeout: 5000 });
92+
await expect
93+
.poll(() => iframeRectTop(page, '[data-testid="q2-breadcrumb-chip"]'), {
94+
timeout: 8000,
95+
message: 'standalone chip top must be >= 0 (not clipped above the viewport)',
96+
})
97+
.toBeGreaterThanOrEqual(0);
98+
});
99+
});

ts-packages/preview-renderer/src/q2-preview/BreadcrumbChip.tsx

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,10 @@ import { buildAncestorPath, currentSourceNodeType } from './nestingNav';
5757
import type { AncestorCrumb } from './nestingNav';
5858
import { BreadcrumbCrumbs, type CrumbDisplayItem } from './BreadcrumbCrumbs';
5959
import { richEditorActiveForType } from './richTextSupport';
60+
import { shouldPlaceChromeBelow } from './editChromeGeometry';
61+
62+
/** Gap (px) between the chip and the surface when flipped below it (bd-pvcnea83). */
63+
const CHIP_FLIP_GAP = 4;
6064

6165
// ── Constants ──────────────────────────────────────────────────────────────────
6266

@@ -255,9 +259,20 @@ export function BreadcrumbChip(): React.ReactElement | null {
255259
);
256260
const displayItems = selectDisplayItems(crumbs, slots);
257261

258-
// --- Chip top: bottom edge flush at surface top ---
262+
// --- Chip top: above the surface by default; flip below when clipped ---
263+
// Default: bottom edge flush at the surface top (top = surfaceTop − chipH).
264+
// But for a surface flush against the viewport top (e.g. the first block of
265+
// a title-less document) that lands above the scroll area and is cropped, so
266+
// flip BELOW the surface instead (bd-pvcnea83). Decide from the surface's
267+
// viewport-relative top (sRect.top), guarded on real geometry so jsdom's
268+
// zero-rects keep the default 'above' placement.
259269
const chipH = chipRef.current?.getBoundingClientRect().height ?? 0;
260-
const top = surfaceTop - chipH;
270+
const haveRealGeometry = sRect.height > 0;
271+
const flipBelow = haveRealGeometry
272+
&& shouldPlaceChromeBelow(sRect.top, chipH, CHIP_FLIP_GAP);
273+
const top = flipBelow
274+
? (sRect.bottom - hostRect.top) + CHIP_FLIP_GAP // below the surface
275+
: surfaceTop - chipH; // above (default)
261276

262277
setGeom({ top, chipLeft, bandWidth, displayItems });
263278
}, [active, et?.anchorR0, et?.anchorR1, ctx?.activeEditRegionRef, ctx?.sourceIndex]);

0 commit comments

Comments
 (0)