ReviewOS

stacks/ts-medium-editor

Toolbar.handleButtonClick early-returns on collapsed selection — Bold/Italic/Underline/Strikethrough/Justify buttons silently no-op when caret is positioned without a highlight

#1889
Closed glennmichael123 opened this 9 days ago ·
9 days ago

Bug

Clicking any toolbar button — Bold, Italic, Underline, Strikethrough, justifyLeft/Center/Right, anything routed through Toolbar.handleButtonClick — does nothing when the user's selection is collapsed (just a caret, no highlighted range). The button visually does not toggle, the formatting doesn't change, no exec command fires.

The most common user-facing repro is the "stuck Bold" case:

  1. Type a sentence
  2. Select the sentence, click Bold → text becomes bold, button lights up ✅
  3. Press → arrow (deselects, caret now after the bold text but still INSIDE <b>)
  4. Type more characters → they're bold (correctly inheriting from the surrounding <b>)
  5. Caret is still inside <b>. User wants to turn bold off. Clicks Bold button.
  6. Nothing happens. Button stays lit, caret stays inside <b>, next typed char is still bold.

Same for Italic with <i>/<em>, Underline with <u>, Strikethrough with <s>. Same for the alignment buttons — justifyLeft/justifyCenter/justifyRight need to work on a collapsed caret because alignment is block-level, but they don't because of the same gate.

Root cause

Toolbar.handleButtonClick, currently at dist/index.js:4361-4371 (and equivalent source location):

const selection2 = window.getSelection()
if (!selection2 || selection2.rangeCount === 0) {
  console.warn('No selection available')
  return
}
const range = selection2.getRangeAt(0)
const selectedText = range.toString().trim()
if (!selectedText) {
  console.warn('No text selected')   // ← bug
  return                              // ← bug
}
if (!this.isSelectionInEditor(selection2)) {
  console.warn('Selection not in editor')
  return
}

The range.toString().trim() check rejects all collapsed selections. But collapsed-selection clicks ARE meaningful:

  • Inline formats (bold/italic/underline/strikethrough): browser's document.execCommand('bold') toggles the "pending format" bit — sets whether the next typed character will be wrapped/unwrapped. Every modern WYSIWYG editor (Google Docs, Notion, MediumEditor's original JS, ProseMirror) supports this. Without it, you can't toggle off a format mid-flight.
  • Block-level commands (justifyLeft/Center/Right, formatBlock, insertorderedlist, insertunorderedlist, indent, outdent): these apply to the entire paragraph containing the caret. A non-collapsed selection is not required and never has been — most users never highlight an entire paragraph before centering it.

Reproducer

import { MediumEditor } from 'ts-medium-editor'

const el = document.getElementById('editor')!
const editor = new MediumEditor(el, {
  toolbar: { buttons: ['bold', 'italic', 'justifyCenter'], static: true },
})

// Type 'hello', then programmatically:
const range = document.createRange()
range.selectNodeContents(el.firstChild!)
window.getSelection()!.removeAllRanges()
window.getSelection()!.addRange(range)
document.querySelector<HTMLButtonElement>('button[data-action="bold"]')!.click()
// Works — selection bold ✅

// Collapse the selection (deselect, caret now inside the resulting <b>):
const r2 = document.createRange()
r2.setStart(el.querySelector('b')!.firstChild!, 5)
r2.collapse(true)
window.getSelection()!.removeAllRanges()
window.getSelection()!.addRange(r2)
document.querySelector<HTMLButtonElement>('button[data-action="bold"]')!.click()
// FAILS — no console.warn surfaces but no exec either. Button stays active.
// Console output: "No text selected"

Proposed fix

Drop the if (!selectedText) return gate. Keep the others — rangeCount === 0 and !isSelectionInEditor(selection2) are legitimately defensive — but range.toString().trim() is not a precondition for any of the actions the toolbar wires up.

 const range = selection2.getRangeAt(0)
-const selectedText = range.toString().trim()
-if (!selectedText) {
-  console.warn('No text selected')
-  return
-}
 if (!this.isSelectionInEditor(selection2)) {
   console.warn('Selection not in editor')
   return
 }
+const selectedText = range.toString().trim()

selectedText is referenced further down (for logging and for restoreSelectionOnFormattedText); keep the local but move it past the guards. The downstream paths already handle empty strings correctly — hasExistingFormatting(range) works on collapsed ranges, document.execCommand(action) handles collapsed selections natively, and the wasActuallyFormatted verification path naturally short-circuits for collapsed cases.

Impact

This is the kind of bug that turns a WYSIWYG editor from "feels professional" to "feels broken" the moment a user does anything beyond the most basic select-and-click pattern. Every Google-Docs-style mid-flight format toggle hits it. Every alignment button is unusable for the common case of "click in a paragraph, click center."

Workaround (for adopters until this lands)

Capture-phase click listener on each toolbar button that handles collapsed selections directly:

toolbarEl.querySelectorAll('button[data-action]').forEach((btn) => {
  btn.addEventListener('click', (e) => {
    const sel = window.getSelection()
    if (!sel || sel.rangeCount === 0) return
    const range = sel.getRangeAt(0)
    if (!range.collapsed) return  // library handles non-collapsed correctly
    if (!editable.contains(range.startContainer)) return
    e.preventDefault()
    e.stopImmediatePropagation()
    editable.focus()
    sel.removeAllRanges()
    sel.addRange(range)
    document.execCommand(btn.getAttribute('data-action'), false, undefined)
    // sync button state via queryCommandState, fire 'input' on editable
  }, true)  // capture phase — beats the library's bubble listener
})

Currently running in production on a bench-review project; can extract a cleaner shared util if useful for the docs.

Stretch: console.warn → silent

While we're here — the console.warn('No text selected') and console.warn('No selection available') paths log to the user's console every time someone clicks a button without first highlighting text. That's noisy in dev tools for a behavior that should just work. Once the fix lands, the warns become unreachable for valid cases; if they stay around for the genuinely-detached paths (rangeCount === 0, not in editor), log.debug rather than log.warn seems more appropriate.

Happy to open a PR — the fix is the 4-line block above plus moving the selectedText variable; tests would cover collapsed-selection bold toggle (was bug), collapsed-selection alignment apply (was bug), and the existing non-collapsed paths (should remain unchanged).

Sign in to comment on this issue.