Skip to content

Enter submits repeatable field-item dialog + autofocus (#127) #322

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
wakqasahmed wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

Enter submits repeatable field-item dialog + autofocus (#127) #322

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a1a16b9
fix: enter submits repeatable field-item dialog, autofocus input (#127)
Aug 16, 2026
1fa69d3
fix: address review findings on enter/autofocus dialog fix (#127)
Aug 16, 2026
eb2b084
Drop deprecated keyCode fallback, isComposing covers IME detection
wakqasahmed Aug 19, 2026
95b783f
fix: defer autofocus until dialog mounts in fieldItem watcher
wakqasahmed Aug 22, 2026
95f3775
fix: restore keyCode 229 IME fallback in dialog keydown guard (#127)
wakqasahmed Aug 23, 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
35 changes: 34 additions & 1 deletion modules/System/assets/vue-components/fields/renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,11 @@ export let FieldRenderer = {
modelValue() {
this.val = this.modelValue;
this.update();
},
fieldItem(val) {
if (val) {
this.$nextTick(() => this.focusFieldItem());

@wakqasahmed wakqasahmed Aug 16, 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.

Blocking (correctness): this autofocus is almost certainly a no-op — the dialog has no input yet when focusFieldItem() runs.

The dialog body renders the field via <fields-renderer> → field-renderer, and the inner FieldRenderer template is gated on v-if="fieldTypes". fieldTypes is only populated in its own mounted() hook, from FieldTypes.get() (modules/System/assets/js/settings.js), which is declared async get() — so even on the cached this._fields path it resolves through extra microtask ticks, and assigning this.fieldTypes then requires a further render flush.

Microtask ordering when fieldItem is set:

  1. flush: dialog mounts, inner field-renderer mounts with fieldTypes === null → renders nothing; FieldTypes.get().then(...) queued.
  2. $nextTick callback (registered on the flush promise) → focusFieldItem() runs.
  3. only later does fieldTypes get assigned and a second flush render the actual <input>.

At step 2 dialog.querySelector("input, textarea, select, [contenteditable]") returns null, if (input) short-circuits, and nothing is focused — silently. So the second half of #127 ("input field should be auto focused") is not actually delivered, and the failure mode is invisible because of the null guard.

Please verify in a browser. If confirmed, focus needs to be driven by something that waits for the field to exist — e.g. focus from the inner component once it has rendered, or a bounded MutationObserver / retry on the dialog element — not a single $nextTick.

@wakqasahmed wakqasahmed Aug 16, 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.

Confirmed and fixed in 1fa69d3. focusFieldItem() now polls via a bounded requestAnimationFrame loop (~60 frames) until a genuinely focusable control exists in the dialog, instead of relying on a single $nextTick that fires before the nested field-renderer's async fieldTypes resolves.

}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
},

Expand Down Expand Up @@ -171,6 +176,34 @@ export let FieldRenderer = {
this.fieldItem = null;
},

focusFieldItem() {

const dialog = document.querySelector(`kiss-dialog[data-field-render-uid="${this.uid}"]`);

if (!dialog) return;

const input = dialog.querySelector('input, textarea, select, [contenteditable]');

@wakqasahmed wakqasahmed Aug 16, 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.

Two problems with this selector, beyond the CodeRabbit note:

  1. [contenteditable] matches contenteditable="false" as well. Use [contenteditable=""], [contenteditable="true"], or filter on el.isContentEditable (which is what onFieldItemKeyup correctly uses one method below — worth being consistent).
  2. It takes the first matching element, not the first focusable one. Concretely in this codebase: field-code (and therefore field-object, which wraps it) initialises CodeMirror 5 asynchronously and its real input is an offscreen <textarea> inside a 3px wrapper; field-boolean renders <input type="checkbox" class="app-switch">, which is visually replaced by CSS. Neither is a sensible "first field to focus", and disabled/readonly inputs are not excluded either.

Minor style point that also matters for CI (see summary): if (!dialog) return; / if (input) input.focus(); are brace-less single-line ifs; every other conditional in this file uses braces.

@wakqasahmed wakqasahmed Aug 16, 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.

Fixed in 1fa69d3 — added a getFocusableFieldItemInput() helper that skips disabled, contenteditable="false", and offscreen/near-zero-size controls (covers CodeMirror's hidden measuring textarea in field-code/field-object, and disabled/readonly inputs). Also added braces to both single-line conditionals.


if (input) input.focus();
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
},

onFieldItemKeyup(evt) {

const tag = evt.target.tagName;

// buttons/links already trigger their own action on enter, don't also save
if (tag === 'BUTTON' || tag === 'A') {
return;
}

// let textareas / contenteditable areas keep their own newline behaviour
if (tag === 'TEXTAREA' || evt.target.isContentEditable) {

@wakqasahmed wakqasahmed Aug 16, 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.

Blocking (correctness): the TEXTAREA / contenteditable / BUTTON / A allowlist does not cover the field types that actually consume Enter, and keyup is the wrong event for this.

Concrete counter-example in this repo — the tags field. field-tags.js renders <app-tags>, whose app-tags.js handleKeydown() does:

case "Enter":
    e.preventDefault();
    ... this.addTag(inputValue);

Its editable control is a plain <input type="text" class="app-tags-input">, so tagName is INPUT and isContentEditable is false — neither guard fires. And preventDefault() on keydown does not suppress keyup, nor does app-tags call stopPropagation(). Net result for a tags field with multiple: true: pressing Enter to commit a tag also saves and closes the item dialog. The user loses the dialog mid-entry. Same class of bug for any third-party field type that handles Enter itself.

Two further gaps that keyup makes unavoidable:

  • IME composition. With a CJK IME, the Enter that commits a candidate produces a keyup with key === "Enter" and isComposing === false (the composition flag only survives on keydown/keypress). So CJK users get the dialog submitted while they are still typing the option label. This is the standard argument for handling Enter on keydown and checking evt.isComposing || evt.keyCode === 229.
  • Focus moving between keydown and keyup. Keyboard events are delivered to whatever is focused at the time of the event. Once the autofocus above is fixed (it is currently inert — see my comment on line 71), activating the "Add item" <button> with Enter will move focus into the dialog before keyup fires, so the keyup lands on the newly focused input, bubbles to kiss-content, and immediately re-saves and closes the dialog the user just opened. The BUTTON/A guard does not help, because by then the target is no longer the button. Today this is masked only because autofocus does not work — the two fixes are coupled.

Suggested shape: move to @keydown.enter, bail on evt.isComposing || evt.keyCode === 229, bail on evt.defaultPrevented (which cleanly covers app-tags and any other control that already calls preventDefault() on Enter), keep the TEXTAREA/isContentEditable guard, and evt.preventDefault() before saving.

@wakqasahmed wakqasahmed Aug 16, 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.

Fixed in 1fa69d3, taking the suggested shape: switched to @keydown.enter, bail on evt.isComposing || evt.keyCode === 229, bail on evt.defaultPrevented (confirmed this correctly excludes app-tags, since its own keydown listener sits on the input itself and runs before our ancestor listener during bubble, so its preventDefault() is already visible to us by the time we check), kept the TEXTAREA/isContentEditable guard, and call evt.preventDefault() before saving. Moving fully to keydown also removes the focus-race you flagged, since there's only one keydown event per physical keypress and it's dispatched/bubbled through the DOM as it existed at press time, before any dialog/focus change from that same press could occur.

return;
}

this.saveFieldItem();
},

removeFieldItem(list, index) {
list.splice(index, 1);
},
Expand Down Expand Up @@ -241,7 +274,7 @@ export let FieldRenderer = {

<teleport to="body">
<kiss-dialog open="true" size="large" :data-field-render-uid="uid" v-if="fieldItem">
<kiss-content class="animated fadeInUp faster">
<kiss-content class="animated fadeInUp faster" @keyup.enter="onFieldItemKeyup">

<div class="kiss-flex kiss-flex-middle">
<div>
Expand Down