fix(extension): restrict fill to editable controls and editor roots

[kevin9327]

Problem

bsk fill rejected a <p> (or other inner block) inside a contenteditable editor with target_not_fillable.

DOM.describeNode only reports the target's own attributes. Snapshot refs for TipTap, ProseMirror, Notion-style editors point at the inner paragraph, which has no contenteditable attribute even though element.isContentEditable is true. Fill never reached the live editability check.

Change

Treat unknown tags as maybe-fillable so the live isContentEditable check can accept inherited editability. Keep rejecting controls that fill cannot drive (button, select, media, and similar).

Testing

• pnpm --filter @browser-skill/extension exec vitest run src/tools/__tests__/fill.test.ts -t contenteditable
• pnpm --filter @browser-skill/extension test

AI-assisted (Grok)

[iuyo5678]

Thanks for working on this. The issue still exists on main (8ca7911), so the fix is worth pursuing. However, testing the patch against that revision in real Chrome exposed a few issues that should be addressed before merging:

1. Ordinary editable descendants still fail. A<p> inside a contenteditable container passes the new check, but DOM.focus fails with Element is not focusable when the paragraph has no tabindex.

2. Filling a focusable descendant can modify the wrong paragraph. For example:

   <div contenteditable="true">
    <p id="target" tabindex="0">old</p>
    <p id="other">keep</p>
   </div>
   

   Filling #target with "hello" using the default clearing behavior leaves the target empty and changes the other paragraph to "hellokeep". The operation then returns fill_value_mismatch, but the content has already changed. On main, the same request is rejected without modifying either paragraph.

3. Non-editable targets can now cause focus side effects before rejection. A non-editable

   receives focus and triggers blur on the previously focused control before the live editability check rejects it.

Could you extend the implementation to distinguish the editing host from the requested target and explicitly constrain the selection to that target? Simply redirecting the operation to the parent editor could also expand the clearing scope unintentionally.

Please also add real-browser regression coverage for replacement, append, background tabs, and preserving sibling content. The new mocked test bypasses the focus and selection behavior responsible for these failures.

I’d recommend keeping this PR open and addressing these issues before merging, especially the unintended content modification.

17:49

[kevin9327]

@iuyo5678 Thanks for testing this in a real browser. All three failures reproduced with a real Chrome, and they are fixed in 3569319, rebased onto current main.

1. Paragraph without tabindex. The target is now kept separate from its editing host. For an element that is neither a native control nor carries its own contenteditable, a live check first resolves the outermost editable ancestor, and DOM.focus goes to that host.
2. Wrong paragraph modified. The target is no longer emptied before typing. After any background focus handling, the selection is set explicitly on the target: its contents are selected for a replacement, or the caret goes to the end of its contents for an append. Input.insertText then edits only that range. The readiness check also requires the selection to lie inside the target, so if a page handler moves it, the fill fails with fill_focus_lost before anything is typed. The editing host is never used as the clearing scope. An empty value clears only the target.
3. Focus side effects on non-editable targets. The live editability check now runs before DOM.focus, so <div tabindex="0"> is rejected with target_not_fillable and the previously focused control keeps focus with no blur event.

Real-browser coverage is in the new fill.browser.test.ts. It uses the existing isolated Chrome launcher and is added to the browser input CI job. There are 18 cases, each run in a foreground and in a background tab (document.hasFocus() false): replacement and append for a plain paragraph, a tabindex paragraph and a span inside a paragraph, all checking the sibling paragraphs; an empty value; the host itself as the target (unchanged behaviour); and the non-editable target. With the previous head, 16 of 18 fail, as you described. With this commit all 18 pass, headless and headed. The unit harness now hands out remote-object handles for the host, and a unit test checks that a non-editable target is rejected without DOM.focus. The extension suite, tsc --noEmit and biome pass.

[iuyo5678]

Thanks for the update! This is a substantial improvement. I re-tested 3569319: the ordinary paragraph, replacement, append, background-tab, and non-editable focus cases now behave correctly. All 18 new browser tests also passed locally.

Two remaining edge cases came up during additional testing:

1. Filling an empty target can still insert text into a sibling.
   With this markup:

   <div contenteditable="true">
     <p id="target"></p>
     <p id="other">keep</p>
   </div>
   

   Filling #target with "hello" leaves it empty and changes #other to "hellokeep", then returns fill_value_mismatch. This reproduces in both foreground and background tabs.
   It also occurs through a normal sequence: successfully clear a non-empty paragraph with fill(""), then fill that same paragraph with "hello". The selection containment check passes, but Chrome’s actual insertion position can still fall outside the empty element.
2. Multiline input can split the target before verification fails.
   Filling <p id="target">old</p> with "one\ntwo" produces:

   <p id="target">one</p>
   <p id="target">two</p>
   

   The operation then returns fill_value_mismatch, since verification reads only the original node. Append and trailing-newline cases show similar behavior.

Could you add browser regressions for empty targets, clear-then-refill, and multiline input? For any cases that cannot yet be supported safely, rejecting them before modifying the page would be a reasonable interim approach.

The remaining scope is much narrower now. I’d still address these before merging, particularly the empty-target case, because it can modify content outside the requested target. Thanks again for the implementation improvements and the real-browser coverage.

[kevin9327]

@iuyo5678 Thanks for the extra testing. Both cases reproduced in headless Chrome and are handled in e2fc4f7:

1. Empty target. Before inserting, Chrome moves a caret placed in an empty element out to a neighbouring text node, which is why the text landed in #other. An empty target now gets a zero-width placeholder character that the typed text replaces, so the insertion stays inside it. Chrome's own empty-line <br> is replaced the same way typing would replace it. If a fill stops early, its placeholder is removed. An element holding only that <br> now also reads as empty instead of \n, so clear-then-refill and appending to an empty paragraph verify correctly.
2. Multiline input. Typing a line break inside an editing host makes the editor split the target, so the text can't stay in, or be verified against, one element. For a target inside an editor, a value containing \n or \r is now rejected with fill_value_invalid before anything is focused or changed, as you suggested. Filling the editing host itself still accepts multiline values.

New browser regressions, each run in foreground and background tabs:

• empty <p>, <p><br></p> and empty <span> targets, for both replace and append, checking that the siblings are untouched and no placeholder is left
• clear-then-refill
• multiline replace, append, trailing newline and CRLF are rejected, and the editor's HTML is unchanged

All 42 tests in fill.browser.test.ts pass locally across 3 runs, and 20 of the new ones fail on 3569319. The extension unit suite, tsc --noEmit and biome also pass.

[iuyo5678]

@kevin9327 Thank you for sticking with this through several rounds. Your real-browser harness and regression cases made it possible to pin down exactly where the behavior breaks, and the final version builds directly on them.

While re-testing e2fc4f7, three more issues came up: a paragraph containing only ordinary spaces could still write into its neighbour, cancelling right after the placeholder was inserted could leave a hidden character behind, and the placeholder cleanup could in rare cases remove a character the user actually asked to fill. They share one root cause: Chrome inserts text at the canonical visible position within the editing host, not at the DOM range we set, so a fill cannot reliably be confined to a descendant. Rich-text editors also re-render their own paragraph nodes. Rather than ask for another round of edge-case fixes, we narrowed the contract and pushed two commits on top of your branch. Your commits are unchanged.

Contract

fill supports two kinds of targets:

• enabled, editable native input / textarea controls, keeping the existing input-type, read-only, disabled, and value-format checks;
• the editing host of a contenteditable region, i.e. the root of the editor, not an outer wrapper and not an inner paragraph or span.

A descendant is rejected with target_not_fillable before anything is scrolled, focused, or changed, and the error hint points to the editor root. A target is never promoted to its parent editor.

Changes

• 36f2a7a fix(extension): restrict fill to editable controls and editor roots
  • Rejects editor descendants, including focusable ones and ones that repeat contenteditable, during preflight.
  • Removes the zero-width placeholder entirely; fill no longer inserts or deletes temporary characters.
  • Places the caret only inside the editing host and verifies it before typing, in foreground and background tabs.
  • Observation exposes contenteditable="" roots as textbox refs.
  • Adds a shared eval fixture and a CLI smoke case for this boundary, and updates the CLI help, error hint, and skill reference.
• 977d88f refactor(extension): read editor text through one line model
  • Reads the layouts Chrome creates while typing (text, <br>, direct <div> lines) with one set of rules instead of three special cases. This also fixes appending to an editor that ends with <br>, which previously wrote the text and then reported fill_value_mismatch.

Behaviour change to note

An element that repeats contenteditable="true" inside an already editable parent was accepted on main and is now rejected. Nested editing hosts inside a contenteditable="false" island are still supported.

Verification (merged with current main, no conflicts)

• fill real-browser regression: 107 passed
• extension unit tests: 2422 passed; tsc and lint clean
• evals: 23 passed; Rust bsk tests passed

Follow-ups, out of scope here

Editing a single paragraph inside a long document is a real need, but it fits poorly with fill's replace/append semantics. A separate capability that places the caret or selection (for example via click) and then inserts text at the caret would work with the editor's own input handling. We'll track that separately.

I'd like to retitle this PR to fix(extension): restrict fill to editable controls and editor roots, since the earlier commit messages describe behaviour the final version no longer has. Please take a look, and let us know if you see any problem with the new boundary. Thanks again for the contribution!