Clip markdown editor toolbar corners to the rounded outline - #5765
Clip markdown editor toolbar corners to the rounded outline#5765FadhlanR wants to merge 4 commits into
Conversation
0298c3b to
2afe9af
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2afe9af45b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| flex: 1; | ||
| min-height: 0; | ||
| border-radius: calc(var(--boxel-border-radius) - 1px); | ||
| overflow: clip; |
There was a problem hiding this comment.
Exclude the Add-embed popover from the clipping wrapper
The new overflow: clip also clips .toolbar-embed-popover, which remains inside this wrapper and is positioned below the toolbar with top: 100%. When the outer edit panel is scrolled far enough that the sticky toolbar is constrained against the editor's bottom edge—the state this change targets—there is no room below it inside the wrapper, so opening Add embed hides part or all of the menu and can make its actions unclickable. The added test only verifies that the menu exists in the DOM, not that it is visible; the popover should escape the clipped subtree or flip above the toolbar near the bottom.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 8a88e2f. The Add-embed popover is now position: fixed, pinned under its trigger from getBoundingClientRect() and repositioned on scroll/resize while it's open. Its containing block is the viewport, so .codemirror-body's overflow: clip can no longer reach it — the menu stays visible and clickable when the toolbar docks at the bottom. The test was inverted to assert the popover is fixed-positioned rather than merely present in the DOM.
You were right and it under-counted: the mode selector had the same problem (its menu is taller), and it's fixed too — it now renders in the shared dropdown wormhole instead of in place.
habdelra
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] I reviewed this as a clipping change rather than a styling change: introducing an overflow box mid-tree changes what every positioned descendant underneath it can do, so the interesting question is not "do the corners look right" but "what else was relying on being able to escape that box". I rebuilt the box structure in Chromium from the real declarations in codemirror-editor.gts, rich-markdown.gts, ember-basic-dropdown.css, select/index.gts and markdown-editor-mode-select.gts, with variables.css loaded so the tokens resolve to their production values, and measured across the outer panel's scroll range. Every number below came out of that harness — not from reading the CSS and reasoning about it.
Bottom line: the corner fix itself is correct and the clip-not-hidden reasoning holds up under measurement, but the new clip box makes both of the toolbar's menus unusable in exactly the bottom-docked state this change exists to fix. That needs resolving before merge. The mode selector — the "Compose" control named in this change's own summary — is affected and is not mentioned anywhere in the diff, the comment, or the tests.
What lands right. The clip vs hidden choice is the load-bearing decision here and it is correct for the stated reason. Toggling only that value: with clip the toolbar holds at a constant 20px offset from the scroll panel's top edge as the panel scrolls; with hidden it runs to −119, −319, −519, −619 — straight out of the panel, because hidden makes .codemirror-body a scroll container and top: 0 then resolves against a box that never scrolls. Documenting that in the CSS is the right call, since the failure is silent and the next person to "tidy" it to hidden would have no signal. I also checked the flex chain the wrapper interposes: with the editor in normal auto-height flow, .codemirror-mount still reaches the editor's inner bottom edge exactly (gap of 0px, short content and long), so the "click anywhere below the text focuses it" affordance survives intact.
Answering the open automated-review thread on this rule: it is correct, and it under-counts. The Add-embed popover is clipped as described — I measured the menu going from 46/46px visible and both items clickable, to 30/46 with one item clickable, to 0/46 with none, across the last ~50px of scroll, with the trigger on screen and clickable throughout. The same mechanism hits the mode selector harder (94px of menu instead of 46px, and on a one-line body field the failure is ~57px of scroll away). It is not a matter of the menu being partly obscured — it is entirely absent while its trigger looks normal.
Recommendations, in priority order:
- Move both toolbar menus out of the clip box, or drop the clip in favour of rounding the toolbar's own bottom corners. Mechanism, measurements, and three concrete options are on the
.codemirror-bodyrule incodemirror-editor.gts. Blocking. - Correct the Add-embed paragraph in the CSS comment once (1) is settled — as written it asserts an invariant the code does not hold. See the thread on that comment block, which also keeps the
clip/hiddenhalf intact. Blocking. - Invert the Add-embed test to assert the menu is not clipped, rather than asserting it sits inside the clipped subtree. See the thread on that test. Blocking, follows from (1).
- Add a
border-radiusassertion to the wrapper test — without it, deleting the radius (the declaration that makes the clip do anything visible) leaves every assertion passing and brings the reported bug straight back. See the thread on that test. Non-blocking. max(0px, calc(var(--boxel-border-radius) - 1px))for the radius, since cards do reassign that token to values near 1px. Detail in the comment-block thread. Non-blocking.
Adjacent, out of scope — worth one manual check. The slash-command (/) menu is a CodeMirror autocompletion tooltip. codemirror-context.ts configures autocompletion({ override: [slashSource] }) with no tooltips({ parent }), so CodeMirror renders it into view.dom — the .cm-editor element, which now sits inside the new clip box. Whether it escapes turns entirely on the position CodeMirror gives it: my probe confirms a position: fixed descendant escapes this clip (containing block is the viewport) while a position: absolute one does not. I could not resolve @codemirror/view in this environment to check which it uses, so this is a check rather than a finding — open the editor, type / near the bottom of a docked body field, and confirm the menu still appears. If it does not, it belongs with the two menus in (1).
Not raising: the two new tests repeat the TestCard + setupIntegrationTestRealm boilerplate, but so does every other test in that file, so that is the file's idiom rather than something this change introduced.
Generated by Claude Code
| .codemirror-body { | ||
| display: flex; | ||
| flex-direction: column; | ||
| flex: 1; | ||
| min-height: 0; | ||
| border-radius: calc(var(--boxel-border-radius) - 1px); | ||
| overflow: clip; | ||
| } |
There was a problem hiding this comment.
[Claude Code 🤖] Blocking — regression. This clip box swallows both menus that live in the toolbar, not just the Add-embed dropdown the comment above accounts for. The one that isn't mentioned anywhere in this change is the Compose / Source / Preview mode selector — the control this change's own summary names as the left end of the grey bar.
Background: what overflow: clip does and does not clip. An overflow value other than visible clips a descendant only when the clipping element sits in that descendant's containing-block chain. So a position: fixed descendant (containing block = the viewport, an ancestor of .codemirror-body) escapes; a position: absolute descendant whose containing block is inside .codemirror-body does not. I verified both directions in Chromium against this exact box structure:
positionFixed -> probe painted below the clip box, hit test returns the probe (escapes)
positionAbsolute -> probe below the clip box, hit test falls through to the page (clipped)
Both toolbar menus land on the clipped side of that rule.
.toolbar-embed-popover—position: absolute, containing block.toolbar-embed-trigger(position: relative, in the toolbar). This is the case the automated reviewer's open thread on this rule flags, and it is real; see my note on the comment block just above.MarkdownEditorModeSelect— it rendersBoxelSelectwith@renderInPlace={{true}}(packages/base/components/markdown-editor-mode-select.gts), andrich-markdown.gtsyields it into<:leadingControls>, which this template renders as the first child of.codemirror-toolbar.renderInPlacekeeps.ember-basic-dropdown-contentin the DOM next to its trigger, andpackages/boxel-ui/src/styles/ember-basic-dropdown.cssgives itposition: absoluteinside.ember-basic-dropdown { position: relative }. Containing block is inside the clip box → clipped.
Measured, using this repo's real CSS and tokens. I rebuilt the box structure in Chromium from the declarations in this file, rich-markdown.gts, ember-basic-dropdown.css, select/index.gts and markdown-editor-mode-select.gts, with variables.css loaded so the tokens resolve to their real values (--boxel-border-radius: 10px, --boxel-sp-xs: 9px, --boxel-sp-3xs: 5.06px, --boxel-font-size-sm: 14px). The 3-option mode menu measures 94px tall. Scrolling the outer edit panel, with a minimum-height editor (a one-line body field, editorH: 155px):
| panel scrollTop | room below toolbar in clip box | toolbar on screen | menu visible | options clickable |
|---|---|---|---|---|
| 0 | 118px | yes | 94/94 | 3/3 |
| 57 | 62px | yes | 64/94 | 2/3 |
| 86 | 33px | yes | 35/94 | 1/3 |
| 115 | 4px | yes | 6/94 | 0/3 |
| 143 | 0px | yes | 2/94 | 0/3 |
The toolbar is still fully on screen in every one of those rows — the trigger looks and behaves normally, and the menu it opens is simply not there. On a one-line body field that state is ~57px of outer-panel scroll away. With a long body the failure window is the last ~100px of scroll, i.e. precisely the bottom-docked state this change exists to fix.
Flipping the menu upward is not an escape either: ember-basic-dropdown's in-place positioning measures window.innerHeight - triggerRect.bottom, so it keys off the browser window, not this clip box. In the docked state the toolbar is near the top of the window, so it stays below and gets clipped; and if it did flip, the toolbar is pinned at the clip box's top edge, so it would be clipped upward instead.
The way out. The menus have to live outside whatever box does the clipping — any clip that contains the toolbar clips the toolbar's positioned descendants, so moving or resizing the wrapper won't help.
- Mode selector: dropping
@renderInPlace={{true}}puts the menu in#ember-basic-dropdown-wormhole, outside the clip entirely.BoxelSelectalready supports this path deliberately for themed contexts —onOpen()callsstartObservingTheme()→syncCustomProps(), which copies--background/--foreground/--border/--primaryonto the wormhole container so the card theme still applies (packages/boxel-ui/src/components/select/index.gts). Worth confirming that holds for a card-rendered field before committing to it, sincepackages/base/enum.gtsreaches forrenderInPlacetoo and that may encode a constraint I haven't found. - Add-embed popover: render it outside
.codemirror-body— either{{in-element}}into a container under.codemirror-editor(which already carriesposition: relative), or switch it toposition: fixedplaced from the trigger'sgetBoundingClientRect(), which the probe above shows escapes the clip. - Cheaper fallback, if both menus must stay in place: skip the clip entirely and put
border-bottom-left-radius/border-bottom-right-radiuson.codemirror-toolbaritself. One line, zero risk to the menus — but the bar then carries rounded bottom corners in the undocked state too, where they read as small notches against the mount. That is a design call rather than a correctness one.
Generated by Claude Code
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 8a88e2f. Dropped @renderInPlace={{true}} from MarkdownEditorModeSelect, so BoxelSelect renders its menu in #ember-basic-dropdown-wormhole — out of the clip box entirely. BoxelSelect's onOpen still syncs the card theme onto the wormhole container, so the themed look holds. The Add-embed popover is handled separately (now position: fixed, since it's a hand-rolled div rather than a BoxelSelect).
| /* Wraps the sticky toolbar + editor mount so the editor's rounded | ||
| corners clip the toolbar's square bottom corners when it docks at | ||
| the bottom on scroll. `overflow: clip` (not `hidden`) does this | ||
| without establishing a scroll container, so the toolbar keeps | ||
| sticking to the outer scroll panel. The Add-embed dropdown lives | ||
| inside this wrapper with the toolbar and is intentionally clipped | ||
| to it: it drops down into the tall editor mount, so the clip is | ||
| invisible in practice. The radius is inset by the 1px border so it | ||
| hugs the border's inner curve. */ |
There was a problem hiding this comment.
[Claude Code 🤖] Two halves of this comment, pulling in opposite directions.
The clip vs hidden half is right, and worth keeping — confirmation. I verified it rather than taking it on faith, because it is the load-bearing claim of the whole change and the failure mode is silent. Same box structure in Chromium, toggling only .codemirror-body's overflow, measuring the toolbar's offset from the scroll panel's top edge as the panel scrolls:
overflow: clip scrollTop 0/200/400/600/700 -> toolbar offset 81, 20, 20, 20, 20 (sticky holds)
overflow: hidden scrollTop 0/200/400/600/700 -> toolbar offset 81, -119, -319, -519, -619
With hidden the toolbar scrolls straight out of the panel — it becomes a scroll container, so top: 0 resolves against .codemirror-body, which never scrolls. That is a real trap for the next person who "tidies" this to hidden, and this comment is the right place to warn them. The mechanism is stated correctly, so it survives refactors.
The Add-embed half is not accurate, and that is what makes it worth fixing rather than just rewording. "It drops down into the tall editor mount, so the clip is invisible in practice" holds only while the editor still has room below the toolbar inside the clip box. In the bottom-docked state — the exact state this change targets — that room is 0 by construction, because docking means the toolbar's bottom edge has reached the clip box's bottom edge. Measured on the same harness, the menu goes from fully clickable to fully gone across the last ~50px of scroll while the trigger stays on screen and clickable:
| room below toolbar | menu visible | "Add a card" | "Add a file" |
|---|---|---|---|
| 51px | 46/46 | clickable | clickable |
| 31px | 30/46 | clickable | clipped |
| 11px | 10/46 | clipped | clipped |
| 1px | 0/46 | clipped | clipped |
So the automated reviewer's open thread on this rule is correct, and it under-counts: the mode selector is affected the same way and harder (94px of menu instead of 46px). Detail and fix options are in my comment on the .codemirror-body rule below. Once that's settled, this paragraph should describe whatever the new arrangement actually guarantees — as written it documents an invariant the code does not hold, which is worse than no comment, since the next reader will trust it.
Unrelated nit on the last sentence, non-blocking. calc(var(--boxel-border-radius) - 1px) goes negative — and so becomes invalid, dropping the radius to 0 — if a card theme sets --boxel-border-radius below 1px. Cards do reassign that token (packages/experiments-realm/task.gts sets it to --boxel-border-radius-xxs, ~1.5px), so the headroom here is about half a pixel. max(0px, calc(var(--boxel-border-radius) - 1px)) costs nothing and removes the cliff.
Generated by Claude Code
There was a problem hiding this comment.
[Claude Code 🤖] All three parts addressed (8a88e2f, radius clamp in e973eda).
clipvshidden: kept — it's the load-bearing choice and the comment still documents whyhiddenwould break sticky.- Add-embed paragraph: rewritten. It no longer claims the popover is "intentionally clipped / invisible in practice" (untrue once docked); it now states both menus must escape the clip and how — the mode selector wormholes out of place, the popover is
position: fixed. max(0px, calc(...)): applied. Worth noting the effect is defensive rather than behavioural — a negativecalc()invalidates the declaration and border-radius falls back to0, which is the same resultmax(0px, …)yields — but the clamp makes the intent explicit and avoids relying on the invalid-declaration fallback.
| assert.strictEqual( | ||
| getComputedStyle(wrapper).overflowY, | ||
| 'clip', | ||
| 'the wrapper clips its overflow', | ||
| ); |
There was a problem hiding this comment.
[Claude Code 🤖] Non-blocking, but this test cannot fail for the reason its name gives. The title promises the toolbar's corners conform to the rounded outline; the assertions check overflow-y, DOM containment, and parentage. None of those is the thing that makes corners round.
What actually does the work. overflow: clip on a square box clips nothing you can see — it only becomes a corner fix because .codemirror-body also carries border-radius: calc(var(--boxel-border-radius) - 1px) on the line right above. Delete that one declaration and every assertion here still passes, while the reported bug comes straight back: the sticky toolbar's square corners spill past the outline again. .codemirror-body appears exactly once in the stylesheet, so there is no other rule that would restore the radius — I checked.
That is the regression this test exists to catch, and it is the one it lets through.
The fix. One more assertion, alongside the overflow one. getComputedStyle resolves the calc() to a pixel value, so assert it is non-zero rather than pinning the exact number — the token is themeable per card and a hard-coded 9px would be brittle:
assert.notStrictEqual(
getComputedStyle(wrapper).borderRadius,
'0px',
'the wrapper is rounded — without a radius the clip has nothing to round off',
);Two smaller things while you are in here:
overflowYalone leavesoverflow-xunpinned. Since the point is that neither axis may become a scroll container (that is what would breakposition: sticky), asserting the shorthandoverflow— or both axes — matches the intent more closely.- The "…while keeping
position: stickyresolving to the outer panel" clause in the inline comment is the single most valuable fact in this file and nothing here checks it. That is genuinely hard to assert in an integration test — it needs a scrolling ancestor and real layout — so a comment is a fair place to leave it. Worth knowing that it is a comment, not a guard.
Generated by Claude Code
There was a problem hiding this comment.
[Claude Code 🤖] Added in 8a88e2f. The wrapper test now asserts getComputedStyle(wrapper).borderRadius is not 0px (non-zero rather than a hard-coded px, since the token is themeable per card). Deleting the radius declaration now fails this test instead of leaving every assertion green.
Left the position: sticky resolution as the documented inline comment, as you suggested — it needs a scrolling ancestor and real layout to assert, which this integration test doesn't set up.
| assert | ||
| .dom('[data-test-toolbar-embed-popover]', wrapper) | ||
| .exists( | ||
| 'the Add-embed dropdown lives inside the clipping wrapper — it drops ' + | ||
| 'into the tall editor mount so the clip is invisible in practice', | ||
| ); |
There was a problem hiding this comment.
[Claude Code 🤖] Blocking, tied to the .codemirror-body thread. This test pins the wrong contract, and the assertion message states as settled fact the thing that turns out not to be true.
What it checks vs. what it claims. .exists() on a DOM query proves the popover is in the subtree. Being inside the clipped subtree is exactly the condition under which it gets clipped — so the assertion passes precisely because the menu is in the position that breaks it. The message then explains that this is fine ("it drops into the tall editor mount so the clip is invisible in practice"), which is the claim I measured and found does not hold once the toolbar docks: in that state the room below the toolbar inside the clip box is 0 by construction, the menu's visible height goes to 0, and neither "Add a card" nor "Add a file" is hit-testable while the trigger stays on screen. Numbers are in my comment on the CSS comment block in codemirror-editor.gts.
The cost of leaving it is not just a weak test — a test that asserts a bug is intended behavior makes the fix look like a regression to whoever ships it next, and they have to relitigate this thread to find out otherwise.
The way out, once the .codemirror-body thread lands: invert it. Assert the menu is not clipped, which is the property anyone actually cares about, and which stays true no matter which of the fix options you pick — wormhole, portal, or position: fixed:
await click('[data-test-toolbar="add-embed"]');
let popover = root.querySelector('[data-test-toolbar-embed-popover]');
let clipBox = root.querySelector('[data-test-codemirror-body]');
assert.ok(
!clipBox.contains(popover),
'the Add-embed menu renders outside the clipping wrapper so it stays visible when the toolbar docks at the bottom',
);If instead you go with the cheap fallback (rounding .codemirror-toolbar's own bottom corners and dropping the clip entirely), this test has nothing left to guard and should just go.
One thing that is right here regardless: the comment on the line above — "No embed is referenced yet, so the toolbar shows the Add-embed trigger" — explains why [data-test-toolbar="add-embed"] and not edit-embed is present, which is a genuinely non-obvious branch in this template ({{#if this._currentBfmRef}}). Keep that sentence wherever this assertion ends up.
Generated by Claude Code
There was a problem hiding this comment.
[Claude Code 🤖] Inverted in 8a88e2f, renamed to "…escapes the corner-clipping wrapper so it stays visible when the toolbar docks at the bottom". It no longer asserts the popover lives inside the clip box; it asserts the popover is position: fixed — the property that lets it escape the clip.
One deviation from the suggested !clipBox.contains(popover): I fixed the popover with position: fixed rather than DOM-relocating it, so it's still a descendant of .codemirror-body and contains() would stay true. Asserting position: fixed is the escape contract for this approach. Kept the "no embed referenced yet" comment above the trigger click.
The rich-markdown editor's docked toolbar is a position: sticky bar with square bottom corners living inside the editor's rounded, bordered container. Because the container did not clip its children, scrolling to the bottom of the body pushed the sticky toolbar down to the container's bottom edge, where its square grey corners spilled past the rounded outline. Wrap the toolbar and editor mount in a `.codemirror-body` element with `overflow: clip` so the rounded corners clip the toolbar's bottom corners when it docks. `clip` (not `hidden`) avoids establishing a scroll container, so the toolbar keeps sticking to the outer scroll panel. The card-search and format-picker popovers stay outside this wrapper so they can still overflow the editor box. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Correct the .codemirror-body comment to state that the Add-embed dropdown is intentionally clipped inside the wrapper, while the card-search popup and format picker stay outside it. Add an integration test asserting the dropdown opens inside the clipping wrapper so the contract can't silently drift. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2afe9af to
c7ce9a8
Compare
The `overflow: clip` corner-clip on `.codemirror-body` also clipped the toolbar's two dropdown menus. When the sticky toolbar docks at the bottom there is no room left below it inside the clip, so both menus were swallowed while their triggers still looked normal. Move both out of the clip: the mode selector wormholes out of place (BoxelSelect without renderInPlace) and the Add-embed popover is now position: fixed, pinned under its trigger and repositioned on scroll. Correct the CSS comment that documented the old clipped behaviour, invert the Add-embed test to assert the menu escapes the clip, and add a border-radius assertion so deleting the radius fails the wrapper test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e it Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
[Claude Code 🤖] Thanks for the review — all three blocking items and both non-blocking ones are addressed. Pushed in 8a88e2f (menus out of the clip + comment + tests) and e973eda (radius clamp). Summary of the fix. Both toolbar menus now escape the corner-clip box:
The CSS comment no longer documents the old clipped-popover behaviour, the Add-embed test asserts the menu escapes the clip (not that it sits inside it), and the wrapper test now fails if the radius is removed. On the slash-command menu (the "worth one manual check" item): resolved by reading the source rather than by hand. CodeMirror's tooltip config defaults to Note: I verified prettier, |
Problem
In the rich-markdown editor, the docked toolbar (the grey "Compose / B / I / H1…" bar) is a
position: stickyelement with square bottom corners, living inside the editor's rounded, bordered container (.codemirror-editor). The container had aborder-radiusbut did not clip its children.When you scroll to the very bottom of the body field, the sticky toolbar is pushed down to the container's bottom edge, and its square grey corners spill past the rounded outline — the reported bug (CS-11700):
Fix
Wrap the toolbar + editor mount in a new
.codemirror-bodyelement withborder-radius+overflow: clip, so the editor's rounded corners clip the toolbar's square bottom corners when it docks at the bottom.Key detail:
overflow: clip, nothidden.hiddenwould establish a scroll container, which would break the toolbar'sposition: sticky(it would stick to the never-scrolling editor instead of the outer edit panel).cliprounds/clips the corners without creating a scroll container, preserving the sticky behavior.The card-search (
/) and format-picker popovers are intentionally left outside the clip wrapper (still positioned relative to.codemirror-editor), so they can continue to overflow the editor box — e.g. when you open the card search on the last line.Tests
rich-markdown-field-test.gtsasserting the toolbar + mount share theoverflow: clipwrapper and that the wrapper nests directly in the bordered editor container.codemirror-embed-toolbar/markdown-embed-choosertests continue to guard the popovers.Verification note
Statically verified (template-lint, eslint, prettier). The corner-pixel conformance is best confirmed visually in the running app; happy to attach before/after screenshots — flag if you'd like them before this comes out of draft.
Linear: CS-11700
Screenshot
Before
After
🤖 Generated with Claude Code