Skip to content

Don't insert stray markers when formatting with no selection (CS-11697) #5673

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
FadhlanR wants to merge 2 commits into
base: main
Choose a base branch
from
+271 −53
Open

Don't insert stray markers when formatting with no selection (CS-11697) #5673

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
35602ae
Don't insert stray markers when formatting with no selection (CS-11697)
FadhlanR Aug 3, 2026
7d74454
Keep unlink reachable from a bare caret inside a link
FadhlanR Aug 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 35 additions & 4 deletions packages/base/codemirror-editor.gts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { on } from '@ember/modifier';
import { scheduleOnce } from '@ember/runloop';
import { htmlSafe } from '@ember/template';
import { Tooltip } from '@cardstack/boxel-ui/components';
import { eq, not } from '@cardstack/boxel-ui/helpers';
import { eq } from '@cardstack/boxel-ui/helpers';

import {
baseRRI,
Expand Down Expand Up @@ -199,6 +199,15 @@ interface ToolbarItem {
// binding in the CodeMirror keymap (bold/italic/code); absent items render a
// label-only tooltip.
shortcut?: string;
// Inline-format toggles (bold/italic/etc.) wrap the current selection, so they
// only make sense when text is highlighted. Set for those buttons so they
// disable when the selection is collapsed — unless the toggle is active
// (e.g. the caret sits inside a link), since untoggling works at a bare
// caret. Line-based buttons omit it.
requiresSelection?: boolean;
// Computed enablement for this button, folding in focus and (for
// selection-requiring buttons) whether text is highlighted.
disabled?: boolean;
}

const EMPTY_FORMATS: SelectionFormats = Object.freeze({
Expand All @@ -212,6 +221,9 @@ const EMPTY_FORMATS: SelectionFormats = Object.freeze({
function sameToolbarState(a: SelectionInfo, b: SelectionInfo): boolean {
return (
a.hasFocus === b.hasFocus &&
// Selection presence gates the inline-format buttons' enablement, so a
// collapse/expand must refresh the toolbar even when nothing else changed.
a.hasSelection === b.hasSelection &&
a.formats.bold === b.formats.bold &&
a.formats.italic === b.formats.italic &&
a.formats.code === b.formats.code &&
Expand Down Expand Up @@ -467,7 +479,15 @@ export default class CodeMirrorEditor extends GlimmerComponent<CodeMirrorEditorS
get toolbarButtons(): ToolbarItem[] {
let f = this.toolbarFormats;
let pressed = (active: boolean) => (active ? 'true' : 'false');
return [
let enabled = this.toolbarEnabled;
let hasSelection = this._selectionInfo?.hasSelection ?? false;
// Inline-format toggles additionally require a highlighted selection,
// except when already active — an active toggle can always be untoggled
// (unlink works from a bare caret inside the link). Line-based buttons
// only require focus.
let disabledFor = (item: ToolbarItem) =>
!enabled || (!!item.requiresSelection && !hasSelection && !item.active);
let items: ToolbarItem[] = [
{
testId: 'bold',
label: 'Bold',
Expand All @@ -476,6 +496,7 @@ export default class CodeMirrorEditor extends GlimmerComponent<CodeMirrorEditorS
active: f.bold,
ariaPressed: pressed(f.bold),
shortcut: `${modKey}B`,
requiresSelection: true,
},
{
testId: 'italic',
Expand All @@ -485,6 +506,7 @@ export default class CodeMirrorEditor extends GlimmerComponent<CodeMirrorEditorS
active: f.italic,
ariaPressed: pressed(f.italic),
shortcut: `${modKey}I`,
requiresSelection: true,
},
{
testId: 'strikethrough',
Expand All @@ -493,6 +515,7 @@ export default class CodeMirrorEditor extends GlimmerComponent<CodeMirrorEditorS
action: this._wrapStrikethrough,
active: f.strikethrough,
ariaPressed: pressed(f.strikethrough),
requiresSelection: true,
},
{
testId: 'code',
Expand All @@ -502,6 +525,7 @@ export default class CodeMirrorEditor extends GlimmerComponent<CodeMirrorEditorS
active: f.code,
ariaPressed: pressed(f.code),
shortcut: `${modKey}\``,
requiresSelection: true,
},
{
testId: 'link',
Expand All @@ -510,6 +534,7 @@ export default class CodeMirrorEditor extends GlimmerComponent<CodeMirrorEditorS
action: this._toggleLink,
active: f.link,
ariaPressed: pressed(f.link),
requiresSelection: true,

@FadhlanR FadhlanR Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Regression (minor, non-blocking): unlinking with a collapsed cursor is no longer reachable from the UI.

The mechanism. toggleLink in packages/host/app/lib/codemirror-context.ts resolves the syntax tree and checks for an enclosing Link node before its collapsed-selection check, and a collapsed cursor inside [text](url) satisfies from >= link.from && to <= link.to — so previously, placing the caret in a link and clicking the Link button unlinked it. This requiresSelection: true disables the button whenever the selection is collapsed, and the toolbar button is toggleLink's only caller (the markdown keymap binds only Mod-B / Mod-I / Mod-`), so the collapsed-cursor entry into the unlink branch is now dead in practice.

Verified. Traced the base-branch toggleLink: a collapsed caret inside a link takes the unlink branch (it precedes if (from === to)) and dispatches the text replacement. Grepped all non-test callers: toggleLink is invoked only via _toggleLink in this file.

What still works. Selecting any range within the link enables the button, and that selection still satisfies the unlink guard — unlink remains reachable, it just now demands a selection. That's why this is minor.

The way out — two options, either fine:

  1. Accept it (select-then-click still unlinks, and it's consistent with the disabled-button story). If so, consider tightening the no-op comment inside toggleLink — "Linking applies to selected text only" isn't quite true of the function itself, whose earlier branch still unlinks at a collapsed caret; it's only true of what the UI now lets through.
  2. Restore it: enable the Link button when the collapsed caret sits inside a link. That needs a link-at-caret signal in SelectionInfo — note the update listener in createEditorState hardcodes formats to all-false for collapsed selections, so formats.link can't serve as-is.

@FadhlanR FadhlanR Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Resolved by restoring the affordance (option 2). What changed:

  • findEnclosingLink is extracted from toggleLink's unlink branch and shared with the selection-info emitter in createEditorState, so the toolbar's Link-button state is computed by the exact same syntax-tree check the command runs — the two can't drift.
  • At a collapsed caret the emitter now reports formats.link from that helper (the other formats stay false, matching wrapWith's no-op at a caret), and the toolbar's enablement rule gains an escape: an active toggle stays clickable without a selection, since untoggling works at a bare caret.
  • The stale wording in toggleLink's no-op comment is fixed — that branch is now correctly described as "caret outside any link".

Coverage added: a toggleLink unlink-from-bare-caret command test, a selection-info test pinning formats.link at a caret inside vs. outside a link, and a component test asserting the Link button's aria-pressed flips as the caret enters and leaves a link (drivable headless — only enablement needs OS focus). The pre-existing selection-based unlink test also now calls the real toggleLink instead of simulating it with a string scan. Both touched modules pass: codemirror-context 55/55, RichMarkdownField 30/30.

},
{ divider: true },
{
Expand Down Expand Up @@ -550,6 +575,12 @@ export default class CodeMirrorEditor extends GlimmerComponent<CodeMirrorEditorS
action: this._toggleBlockquote,
},
];
for (let item of items) {
if (!item.divider) {
item.disabled = disabledFor(item);
}
}
return items;
}

/** Prevent mousedown on toolbar/popup buttons from stealing editor focus/selection */
Expand Down Expand Up @@ -1212,7 +1243,7 @@ export default class CodeMirrorEditor extends GlimmerComponent<CodeMirrorEditorS
is suppressed while the control is disabled. }}
<Tooltip
@placement='top'
@disabled={{not this.toolbarEnabled}}
@disabled={{btn.disabled}}
data-test-toolbar-tooltip={{btn.testId}}
>
<:trigger>
Expand All @@ -1222,7 +1253,7 @@ export default class CodeMirrorEditor extends GlimmerComponent<CodeMirrorEditorS
type='button'
aria-label={{btn.label}}
aria-pressed={{btn.ariaPressed}}
disabled={{not this.toolbarEnabled}}
disabled={{btn.disabled}}
{{on 'mousedown' this._preventFocusLoss}}
{{on 'click' btn.action}}
>{{#let btn.icon as |Icon|}}<Icon
Expand Down
60 changes: 35 additions & 25 deletions packages/host/app/lib/codemirror-context.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1011,13 +1011,11 @@ function wrapWith(marker: string) {
let { from, to } = view.state.selection.main;
let len = marker.length;

// No selection: insert an empty pair of markers and drop the cursor
// between them so the user can type the content (e.g. **|**).
// No selection: do nothing. Inserting an empty marker pair here leaves
// stray markers (e.g. **) visible in source and preview. Inline formatting
// applies to selected text only — the toolbar disables these buttons and
// the keyboard shortcut is a no-op until the user highlights something.
if (from === to) {

@FadhlanR FadhlanR Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Confirmation: returning true on a collapsed selection is the right call.

Returning false would let the key event fall through CodeMirror's keymap to the browser default, and these chords have browser-level bindings in some contexts (e.g. Ctrl-B toggling a bookmarks sidebar). Consuming the shortcut while editing nothing is the correct no-op, and the rewritten test pins both halves — returns true, document and cursor unchanged.

When this would stop being safe: if a lower-priority keymap ever wants to handle Mod-B/Mod-I/Mod-on a collapsed cursor, thistrue` will shadow it silently. Nothing binds them today.

view.dispatch({
changes: { from, insert: marker + marker },
selection: { anchor: from + len },
});
return true;
}

Expand Down Expand Up @@ -1066,21 +1064,32 @@ function wrapWith(marker: string) {
};
}

// Toggle a markdown link around the selection. Uses the syntax tree to detect
// an enclosing [text](url) — a string scan can match across unrelated brackets
// and delete text the user never selected.
function toggleLink(view: EditorView): boolean {
let { from, to } = view.state.selection.main;

let node: any = syntaxTree(view.state).resolveInner(from, 1);
let link: any = null;
// Find the markdown Link syntax node enclosing the given range, if any. Uses
// the syntax tree — a string scan can match across unrelated brackets and
// claim text the user never selected. Shared by toggleLink's unlink branch and
// the selection-info emitter, so the toolbar's Link-button state can't drift
// from what the command actually supports.
function findEnclosingLink(
state: EditorState,
from: number,
to: number,
): any | null {
let node: any = syntaxTree(state).resolveInner(from, 1);
for (let n: any = node; n; n = n.parent) {
if (n.name === 'Link') {
link = n;
break;
if (n.name === 'Link' && from >= n.from && to <= n.to) {
return n;
}
}
if (link && from >= link.from && to <= link.to) {
return null;
}

// Toggle a markdown link around the selection. A caret or selection inside an
// existing [text](url) unlinks it; a selection elsewhere wraps as a link.
function toggleLink(view: EditorView): boolean {
let { from, to } = view.state.selection.main;

let link = findEnclosingLink(view.state, from, to);
if (link) {
// Unlink: replace the whole node with just its text (between [ and ]).
let marks: { from: number; to: number }[] = [];
let c = link.cursor();
Expand All @@ -1099,12 +1108,9 @@ function toggleLink(view: EditorView): boolean {
}

if (from === to) {
// No selection: insert empty link syntax with the cursor inside the
// brackets so the user can type the link text — [|](url).
view.dispatch({
changes: { from, insert: '[](url)' },
selection: { anchor: from + 1 },
});
// Caret outside any link: nothing to unlink, and inserting empty link
// syntax would leave a stray [](url) in the document. The wrap direction
// needs a selection.
return true;
}

Expand Down Expand Up @@ -1258,11 +1264,15 @@ function createEditorState(options: CreateEditorStateOptions): EditorState {
formats: hasSelection
? detectFormats(update.state, from, to)
: {
// The wrap toggles no-op at a caret, so they read inactive.
// Link is the exception: toggleLink can still unlink an
// enclosing [text](url) from a bare caret, so report it
// active — the toolbar keeps an active toggle clickable.
bold: false,
italic: false,
code: false,
strikethrough: false,
link: false,
link: !!findEnclosingLink(update.state, from, to),
},
currentRef,
});
Expand Down
Loading
Loading