Improve mobile input navigation #4647 - #4648
Conversation
There was a problem hiding this comment.
Pull request overview
Improves mobile form navigation in form2 by introducing a consistent “mobile completion” callback for leaf inputs, preserving IME composition behavior, and moving focus forward (including across occurrences) without implicitly adding/removing occurrences.
Changes:
- Added mobile accessibility utilities to detect the next appropriate focus target and to handle Enter/Next key completion while respecting IME composition.
- Extended input-type props with
onMobileCompleteand updated multiple input components to opt into mobile “Next” behavior (enterKeyHint,onKeyDown, anddata-mobile-focus-target). - Updated
InputField/OccurrenceListto wire mobile completion through occurrences and added new/updated Vitest coverage for the navigation behavior.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/resources/assets/admin/common/js/form2/utils/accessibility.ts | Adds mobile focus target discovery + Enter/IME-safe completion helper. |
| src/main/resources/assets/admin/common/js/form2/utils/accessibility.test.ts | Unit tests for new mobile accessibility helpers. |
| src/main/resources/assets/admin/common/js/form2/types.ts | Adds onMobileComplete to shared input component props. |
| src/main/resources/assets/admin/common/js/form2/components/time-input/TimeInput.tsx | Wires mobile completion to Enter key + picker confirmation. |
| src/main/resources/assets/admin/common/js/form2/components/text-line-input/TextLineInput.tsx | Adds mobile “Next” behavior and marks explicit focus target. |
| src/main/resources/assets/admin/common/js/form2/components/text-line-input/TextLineInput.test.ts | Tests enterKeyHint and Enter completion behavior. |
| src/main/resources/assets/admin/common/js/form2/components/text-area-input/TextAreaInput.tsx | Marks textarea as a mobile focus target. |
| src/main/resources/assets/admin/common/js/form2/components/radio-button-input/RadioButtonInput.tsx | Ensures a primary radio is the focus target/ref for mobile navigation. |
| src/main/resources/assets/admin/common/js/form2/components/occurrence-list/OccurrenceList.tsx | Threads onMobileComplete through occurrence rendering. |
| src/main/resources/assets/admin/common/js/form2/components/MobileInputBehavior.test.tsx | Adds integration-style tests for picker completion + mobile native controls. |
| src/main/resources/assets/admin/common/js/form2/components/long-input/LongInput.tsx | Adds mobile “Next” behavior and explicit focus target. |
| src/main/resources/assets/admin/common/js/form2/components/instant-input/InstantInput.tsx | Wires mobile completion to Enter key + picker confirmation. |
| src/main/resources/assets/admin/common/js/form2/components/input-field/InputField.tsx | Detects mobile via external store and moves focus on mobile completion (incl. within occurrences). |
| src/main/resources/assets/admin/common/js/form2/components/input-field/InputField.test.ts | Adds tests for mobile completion focus movement and non-occurrence changes. |
| src/main/resources/assets/admin/common/js/form2/components/geo-point-input/GeoPointInput.tsx | Adds mobile “Next” behavior and explicit focus target. |
| src/main/resources/assets/admin/common/js/form2/components/double-input/DoubleInput.tsx | Adds mobile “Next” behavior and explicit focus target. |
| src/main/resources/assets/admin/common/js/form2/components/date-time-input/DateTimeInput.tsx | Renders native datetime-local input on mobile and forces non-native picker on desktop. |
| src/main/resources/assets/admin/common/js/form2/components/date-input/DateInput.tsx | Renders native date input on mobile and forces non-native picker on desktop. |
| src/main/resources/assets/admin/common/js/form2/components/checkbox-input/CheckboxInput.tsx | Marks checkbox as a mobile focus target and forwards ref. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
edloidas
left a comment
There was a problem hiding this comment.
I ran this against the issue rather than reading it — Storybook at mobile viewports, reading rendered DOM and tabIndex values rather than JSX, plus the resolved @enonic/ui 1.1.1 bundle for what the pickers actually do with native. pnpm check is green on the branch: types, Biome, and 1105 tests.
The core mechanism holds up, and it is the right mechanism. data-mobile-focus-target genuinely beats the sortable list's tabIndex = -1 — I confirmed the explicit branch finds an input the fallback rejects, which is the whole point of adding it. The IME guard is correct and placed where every wired input goes through it. Completion never touches add or remove. And getInputRef already populates inputRefsRef for list mode, so the next-occurrence hop works wherever a leaf forwards its ref.
Three findings need a change before this ships and one is a scope question. Two of them are the same shape: the mechanism is right, but two of the input types it was built for do not participate — DateInput and DateTimeInput never get the marker or the completion callback, and everything added to InstantInput is inside a subtree the picker discards on mobile. The third is the mobile detection itself.
One withdrawal, because it shaped my first read: I had a reviewer report that TimeInput loses its value on mobile the same way, and that is wrong. TimePicker.Root returns composed children early and never consults native, so TimeInput keeps its own input and confirm button — measured at 375px, data-mobile-focus-target="true" present. DatePicker.Root has no such escape hatch, which is why only its consumers are affected.
Deliberately excluded: Copilot's open thread about [contenteditable="true"] — the selector is inconsistent with TagInput and SortableGridList, but nothing in this repo renders a bare contenteditable, so I have no failing case for it. The HH:MM truncation on the mobile DateTime path is pre-existing and shared with the desktop picker. TagInput and ComboBoxInput also lack the focus marker, but this branch does not touch them and that belongs in a follow-up.
One comment per finding, on the line it concerns. The two I would not merge without are the missing focus target on the Date/DateTime native branches and the unreachable wiring in InstantInput — both land directly on behaviour the issue names.
6adf91d to
ed7bbc4
Compare
Handle mobile completion across input types, preserve IME composition, and move focus without changing occurrences.
Handle mobile completion across input types, preserve IME composition, and move focus without changing occurrences.