Add opt-in lvt-mod:skip-when-typing guard for window keyboard bindings

[adnaan]

What

Adds an opt-in lvt-mod:skip-when-typing modifier for lvt-on:window:keydown/keyup bindings. When present, the binding does not fire if document.activeElement is (or is within) an editable element — a text-like <input>, <textarea>, <select>, or [contenteditable]. Button/checkbox/radio inputs are not treated as editable, so shortcuts still fire when one is focused.

Why

Global single-key shortcuts (e.g. j/k to navigate) previously fired even while the user was typing in a text field, so a shortcut letter couldn't coexist with text entry. The guard is opt-in so that bindings which must fire while typing (e.g. Escape to cancel) simply omit the attribute and keep working.

Consumed downstream by prereview's keyboard-support feature (livetemplate/prereview#28).

Tests

tests/event-delegation.test.ts covers: opted-in binding suppressed in textarea/input/contenteditable; fires on non-editable focus; and a binding without the opt-in still fires while a textarea is focused. Full suite green.

🤖 Generated with Claude Code

[claude]

Review: opt-in lvt-mod:skip-when-typing guard

Overall clean — logic is correct, tests cover the key cases, and the opt-in design is the right call. A few things worth knowing:

Shadow DOM gap (minor)
document.activeElement returns the shadow host when focus is inside a shadow root, not the actual inner element. closest() won't pierce the shadow boundary, so isEditableTarget will return false for a focused <input> that lives inside a custom element's shadow DOM — and shortcuts will fire unexpectedly while the user types. If the app uses web components with internal text inputs anywhere, this is worth addressing (e.g. check shadowRoot.activeElement recursively). If not, this is moot.

<select> in EDITABLE_SELECTOR
Reasonable call — selects do typeahead — but worth flagging as a deliberate choice. A user who opens a <select> dropdown and presses j will have that keystroke suppressed for any skip-when-typing binding. That's probably what you want, just make sure callsites are aware.

No functional bugs found. Tests are thorough. Good to merge once the shadow DOM question is answered for your use case.

[claude]

Review

Good feature overall — the opt-in design is the right call, shadow-DOM traversal is handled correctly, and the test coverage is solid. A few things worth addressing:

Test DOM cleanup (no cleanup → test pollution)

setupWindowKeydown appends nodes to document.body but there's no afterEach (or cleanup return value) to remove them. DOM elements and attached window listeners accumulate across tests. The shadow DOM test is particularly risky — the shadow root persists and could interfere with later tests that inspect document.activeElement or shadowRoot. Suggest adding:

afterEach(() => {
  document.querySelectorAll("[data-lvt-id]").forEach((el) => el.remove());
});

input[type="image"] is missing from the exclusion list

The PR description says button/submit/reset/checkbox/radio are deliberately excluded because they accept activation keys, not text. <input type="image"> is functionally identical to a submit button but isn't in the negation list — so shortcuts would be suppressed while it's focused. Low-frequency edge case, but easy to add:

':not([type="image"])'

Does the guard apply to keyup too?

The PR description mentions lvt-on:window:keydown/keyup but the guard is added in one location and tests only cover keydown. Worth confirming (or adding a test for) that keyup bindings with lvt-mod:skip-when-typing are also guarded.

---

The core logic (deepActiveElement, closest for ancestor-editable check, opt-in via attribute) is sound. The issues above are all straightforward fixes.

[claude]

Clean implementation with thorough test coverage. A few observations:

Logic is correct. The opt-in design (guard only fires when lvt-mod:skip-when-typing is present) is the right call — Escape-style bindings keep working without any changes. Shadow DOM traversal in deepActiveElement is necessary and handled correctly.

Minor type imprecision — isEditableTarget accepts EventTarget | null but the only call site passes Element | null (the return type of deepActiveElement). The instanceof Element guard makes it safe, but tightening the parameter type to Element | null would make the intent clearer.

input[type="range"] is included as "editable" (it's not excluded by the :not() chain). Range inputs respond to arrow keys, so this is probably fine — but worth being explicit that it's intentional if it surfaces unexpectedly.

No bugs or security issues. LGTM.

[claude]

Code review — PR #137 (skip-when-typing guard)

Overall this is well-implemented and the test coverage is solid. Two actionable findings:

1. range input behavior is asserted in comments but not tested

The comment explicitly states range inputs are intentionally left in scope (so arrow-key shortcuts are suppressed while a slider is focused). That is a non-obvious policy decision worth pinning with a test. Without one, a future reader who finds the omission surprising may "fix" it and break the intended UX.

2. Test DOM leaks between cases

setupWindowKeydown appends to document.body but never cleans up. Wrappers accumulate across the test suite. This is fine as long as each wrapper ID stays unique (they are here), but an afterEach(() => wrapper.remove()) in the describe block would prevent subtle failures if a future test shares an ID or relies on a clean body.

No bugs or security issues found. The deepActiveElement shadow-DOM traversal is correct, the EDITABLE_SELECTOR exclusions are well-reasoned, and the opt-in design (attribute required to suppress) is the right default.

[claude]

Review

Logic: guard is silently skipped when lvt-key is absent

The skip-when-typing check lives inside the hasAttribute("lvt-key") block (lines 750–770 in the merged file), so this markup silently ignores the guard:

<!-- No lvt-key → guard never runs, shortcut fires while typing -->
<div lvt-on:window:keydown="nextFile" lvt-mod:skip-when-typing></div>

That combination is admittedly unusual (a binding that fires on every keydown), but the dependency is invisible to consumers. Two options:

1. Move the guard outside the lvt-key block so it always applies when the attribute is present.
2. Log a warning (dev-mode only) if skip-when-typing is set without lvt-key.

Minor gap: contenteditable="plaintext-only"

Chrome supports contenteditable="plaintext-only" as a distinct editable mode. It isn't matched by [contenteditable=""] or [contenteditable="true"], so a focused plaintext-only region won't suppress the shortcut. Probably fine to leave as a follow-up given how rare it is, but worth a note.

Everything else looks good — the shadow-DOM traversal is correct, the opt-in design is the right call, tests cover the key cases, and the selector exclusions for button/checkbox/radio inputs are well-reasoned. The range-input intentional inclusion is clearly documented.

[claude]

Overall: looks good — the opt-in design is correct and tests are thorough. Two things worth addressing before merge:

---

1. input[type="file"] is incorrectly treated as editable (dom/event-delegation.ts)

EDITABLE_SELECTOR excludes button/submit/reset/checkbox/radio/image but not file. So when a file-picker input is focused, all lvt-mod:skip-when-typing shortcuts are suppressed — but file inputs don't accept typed text. The exclusion list should include type="file":

'input:not([type="button"]):not([type="submit"]):not([type="reset"])'
  + ':not([type="checkbox"]):not([type="radio"]):not([type="image"])'
  + ':not([type="file"])',

---

2. input[type="number"] inconsistency with input[type="range"]

The comment explicitly calls out that range is kept in the editable set because arrow keys adjust the slider. The same is true of number inputs (↑/↓ increment the value). If range inputs suppress arrow shortcuts, number inputs probably should too. Worth a deliberate decision either way — right now number is included by default but the rationale comment only mentions range.

---

No other issues. The deepActiveElement() shadow-DOM traversal is correct, the closest() approach for contenteditable children is right, and the check order (key filter before typing guard) is efficient.

[claude]

Review

Clean implementation with solid test coverage. A few things worth noting:

Minor bug: color input unintentionally treated as editable

The exclusion list covers button/submit/reset/checkbox/radio/image/file, but not type="color". A focused color picker would suppress shortcuts with lvt-mod:skip-when-typing even though no text entry happens. Low-impact but worth adding :not([type="color"]) to the exclusion chain alongside file.

Redundant instanceof check in isEditableTarget

The parameter is already typed as Element | null, so node instanceof Element only guards null — node !== null would be clearer. Not a bug, just noisy.

Everything else looks good:

• Shadow DOM traversal in deepActiveElement is correct (iterates through nested shadow roots)
• closest() on the deepest active element correctly walks up within the same shadow tree
• The opt-in design (attribute absent means always fire) is the right default, so Escape-style bindings work without changes
• select inclusion is intentional and documented
• Test coverage is thorough: textarea, text input, contenteditable, shadow DOM, button/non-editable, range, keyup, no-lvt-key filter, and no-opt-in cases all covered

The color input exclusion is the only real issue worth fixing before merge.