Skip to content

fix(content-types): focus the Name input when creating, nothing when editing - #36820

Open
oidacra wants to merge 5 commits into
mainfrom
issue-36816-content-type-create-dialog-focus-the-name-input-wh
Open

fix(content-types): focus the Name input when creating, nothing when editing#36820
oidacra wants to merge 5 commits into
mainfrom
issue-36816-content-type-create-dialog-focus-the-name-input-wh

Conversation

@oidacra

@oidacra oidacra commented Jul 30, 2026

Copy link
Copy Markdown
Member

Summary

Opening the Content Type dialog put initial focus on the new-content-editor banner checkbox instead of the Name input, so you had to click before typing. This affected both the create and the edit dialog.

Dialog Before After
Create (create/:type) Focus jumped to the banner checkbox Focus stays in the Name input
Edit (edit/:id) Focus jumped to the banner checkbox Nothing is focused

The Name input already declared the intent to be focused — dotAutofocus on the input plus a .focus() in the form's ngOnInit, neither gated by mode — and both were undone afterwards by PrimeNG's p-dialog:

  • focusOnShow defaults to true, and onAfterEnter() schedules getFocusableElements(content)[0].focus() using the transition duration (~150ms), which outlives dotAutofocus's 100ms timeout.
  • getFocusableElements walks the DOM in document order and ignores the [tabindex]="1" on the Name input, so the DOM-first banner checkbox won.

The visible symptom was the caret and focus ring appearing on the Name input and then jumping away.

Closes #36816

Approach

  1. Disable focusOnShow on the p-dialog, in both modes, so PrimeNG stops competing for the initial focus. There is no timer race to win — the competitor is removed.
  2. Make the form's focus mode-aware. Its two mechanisms were both mode-blind: the dotAutofocus directive and the ngOnInit .focus(). A directive cannot be applied conditionally through a binding, so both are replaced by one call in the component, leaving a single place that decides what gets focused.
  3. Defer that call with afterNextRender, because the input is not focusable while the form is still rendering inside the dialog.

Two things found along the way, both worth knowing:

  • The synchronous .focus() in ngOnInit was dead code. Removing dotAutofocus made the create-mode tests fail, which proved the directive's setTimeout was what actually landed the focus all along.
  • Edit mode had the same defect from the same cause, and it predates this branch: on main the p-dialog bound no focusOnShow at all, so PrimeNG's default of true applied to both modes. The issue was originally scoped to create only; since the cause and the fix are the same lever, it is resolved in both rather than leaving a known defect behind a mode guard.

focusTrap is deliberately left unbound — it is an independent code path ([pFocusTrapDisabled]="focusTrap === false"), so the dialog keeps trapping Tab. closable is untouched and still differs by mode as before.

Changes

File Change
dot-content-types-edit.component.ts readonly dialogFocusOnShow = false, with JSDoc explaining the PrimeNG race so the binding doesn't read as stray.
dot-content-types-edit.component.html [focusOnShow]="dialogFocusOnShow" on the p-dialog.
content-types-form.component.ts Focus the Name input from afterNextRender, only when not in edit mode.
content-types-form.component.html dotAutofocus removed from the Name input; the component now owns the decision.
dot-content-types-edit.component.spec.ts focusOnShow disabled asserted in both modes, focusTrap/closable pinned unchanged, ?open-config=true covered.
content-types-form.component.spec.ts Create-mode focus test driven through the render hook; new edit-mode no-focus test.
content-types-form-dialog-focus.spec.ts (new) Real form inside a real p-dialog, asserting actual document.activeElement for create and edit, plus a reproduction of the original defect.

Acceptance criteria

  • AC1 — Focus lands on #content-type-form-name when the create dialog opens. Asserted against real DOM focus. Both cited routes resolve to the same component, so coverage is component-level rather than per-URL.
  • AC2 — Typing fills the Name field. Covered by a synthetic input event on the focused element; see manual verification for the real-keystroke path.
  • AC3 — Holds with CONTENT_EDITOR2_ENABLED both true (banner visible) and false.
  • AC4 — The banner checkbox does not receive initial focus, and its control value is unchanged after typing.
  • AC5 — Parametrized over CONTENT and WIDGET for real focus.
  • AC6 — Opening the edit dialog focuses nothing. Covered for both entry points, the layout gear and ?open-config=true.
  • AC7focusTrap and closable asserted unchanged in both modes. ESC-to-close and Tab order are untouched by construction (no test asserts them; see manual verification).
  • AC8 — Create focus, edit no-focus, and the defect reproduction all have tests.

Test plan

pnpm nx affected:lint --base=origin/main    # 3 projects, clean
pnpm nx affected:test --base=origin/main    # dotcms-ui 2177 passed / portlets-dot-publishing-queue-portlet 235 passed

New spec: 7/7. dot-content-types-edit.component.spec.ts: 48/48. content-types-form.component.spec.ts: 35/35.

Mutation-checked twice. Reverting the dialog binding makes the create-mode and ?open-config=true assertions fail; reverting the form change makes the edit-mode no-focus assertions fail. The tests gate both halves of the behavior rather than passing regardless — worth noting because the original 'should have name focus by default on create mode' test passed either way, since it never rendered a p-dialog.

Two jsdom constraints the new spec works around, both commented in place:

  • jsdom never fires a CSS transition end, so PrimeNG's onAfterEnter() is invoked directly instead of awaited. afterNextRender hooks are driven with ApplicationRef.tick(), which runs them synchronously and so coexists with the fake timers needed to control PrimeNG's own timer.
  • jsdom has no layout engine, so PrimeNG's isVisible/offsetParent check would make its focus() a no-op and the race under test would never happen. The spec shims HTMLElement.prototype.offsetParent in beforeAll and restores it in afterAll. This is load-bearing for the assertions' validity, not incidental setup. It cannot leak across files: Jest builds a fresh jsdom realm per test file.

Manual verification

Confirmed working in the running app for both modes. Steps, including what unit tests cannot reach:

Create

  1. Go to /dotAdmin/#/content-types-angular/create/content/fields — the caret must be in Content Name, with no visible jump to the banner checkbox.
  2. Type without clicking anywhere; the text must land in Content Name (the real-keystroke path behind AC2).
  3. Repeat with CONTENT_EDITOR2_ENABLED=false (AC3).
  4. Repeat on create/widget (AC5 in a real browser).

Edit

  1. Open an existing content type, then open the dialog from the layout gear — nothing must be focused.
  2. Same via ?open-config=true.

Both

  1. ESC still closes the dialog and Tab still cycles inside it (AC7).

…og opens

The Create Content Type dialog auto-opens on create/:type, but initial focus
landed on the new-content-editor banner checkbox instead of the Name input.

The Name input already declared the intent to be focused (dotAutofocus, plus a
synchronous .focus() in the form's ngOnInit), and both were undone afterwards by
PrimeNG's p-dialog: focusOnShow defaults to true, and onAfterEnter() schedules
getFocusableElements(content)[0].focus() with the transition duration (~150ms),
outliving dotAutofocus' 100ms timeout. getFocusableElements walks the DOM in
document order and ignores the [tabindex]="1" on the Name input, so the
DOM-first banner checkbox won. The visible symptom was the caret and focus ring
appearing on the Name input and then being taken away.

Bind focusOnShow to the edit-mode flag so PrimeNG no longer competes in create
mode, letting the existing focus land and stay. Edit mode keeps the PrimeNG
default of true and is unchanged. focusTrap is left untouched -- it is an
independent code path, so the dialog keeps trapping Tab in both modes.

Closes #36816
@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Claude finished @oidacra's task in 2m 38s —— View job


PR Review

  • Read the diff and changed files
  • Analyzed focus logic + edit-mode detection
  • Reviewed tests
  • Posted findings

I reviewed the production changes (content-types-form.component.ts, dot-content-types-edit.component.{ts,html}, the two HTML templates) and the tests against origin/main.

New Issues

None. The production change is small and sound:

  • afterNextRender(..., { injector }) is the right hook — a synchronous focus() in ngOnInit on an element inside a p-dialog that is still animating open is a no-op, and passing the explicit injector makes the call valid outside the constructor injection context.
  • Gating on isEditMode() (!!(data && data.id)) reads $contentType(), which is populated by the time ngOnInit runs, so the create/edit branch is decided correctly.
  • Disabling focusOnShow on the dialog removes the PrimeNG post-transition getFocusableElements(content)[0].focus() that was overriding the form's own focus. focusTrap/closable are correctly left untouched (independent inputs), and the specs pin those as tripwires.
  • dotAutofocus was removed only from this input; the directive is still imported/used elsewhere (name-property component, and still declared in dot-content-types-edit.module.ts), so nothing is broken by the removal.

Minor (non-blocking)

  • 🟡 content-types-form-dialog-focus.integration.spec.ts:112afterAll restores offsetParent via Object.defineProperty(HTMLElement.prototype, 'offsetParent', originalOffsetParent). If getOwnPropertyDescriptor had returned undefined (it won't in current jsdom, which defines offsetParent on the prototype), this line would throw. It's safe today; just a latent coupling to jsdom internals worth a comment. No action required.

Notes

  • The new *.integration.spec.ts is picked up by Nx's default testMatch (any *.spec.ts), so the added coverage does run in CI — I verified there's no custom testMatch narrowing it out.
  • Extracting the shared mocks into content-types-form.testing.ts removes real duplication and is correctly scoped as test-only (unreachable from main.ts).
  • The "reproduces the bug" test (focusOnShow: true) is a nice touch — it pins why the binding must stay false, and the PR's mutation check (revert each half → the matching assertions fail) confirms the tests actually gate both halves of the behavior.

This looks good to merge.

Edit mode had the same defect as create, from the same cause, and it predates
this branch: on main the p-dialog bound no focusOnShow at all, so PrimeNG's
default of true applied to both modes, and the form's focus attempts are not
gated by mode either (content-types-form.component.ts ngOnInit .focus() and the
dotAutofocus on the input both run regardless). So opening the Edit Content
dialog also showed focus landing on the Name input and then jumping to the
banner checkbox.

Since the cause and the fix are identical, resolve it in the same place instead
of leaving a known defect behind a mode guard: dialogFocusOnShow is now a
readonly false rather than being derived from isEditMode().

Tests follow: the edit-mode assertions now expect focusOnShow disabled, and the
focusOnShow-enabled case in the dialog focus spec is retitled to what it
actually is -- a reproduction of the defect, pinning why the binding must stay
off rather than documenting intended behavior.
@oidacra oidacra changed the title fix(content-types): keep focus on the Name input when the create dialog opens fix(content-types): keep focus on the Name input when the content type dialog opens Jul 30, 2026
…en editing

Follow-up on the focus behavior: creating still focuses the Name input, editing
now focuses nothing. Previously the form focused the Name input unconditionally,
so opening the Edit Content dialog moved the caret into an already-filled field.

The form's two focus mechanisms were both mode-blind: the dotAutofocus directive
on the input, and a .focus() in ngOnInit. A directive cannot be applied
conditionally through a binding, so both are replaced by a single mode-aware
call in the component, leaving one place that decides what gets focused.

That call is deferred through afterNextRender because the input is not focusable
while the form is still rendering inside the dialog -- the previous synchronous
.focus() in ngOnInit was in fact dead code, and dotAutofocus' own setTimeout was
what actually landed the focus.

Tests assert both modes end to end against real DOM focus, and drive the render
hook with ApplicationRef.tick().
@oidacra oidacra changed the title fix(content-types): keep focus on the Name input when the content type dialog opens fix(content-types): focus the Name input when creating, nothing when editing Jul 30, 2026
@oidacra
oidacra marked this pull request as ready for review July 30, 2026 19:47
The spec renders the real form inside a real PrimeNG dialog, so it is not a unit
test: the behavior it covers only exists when both collaborate. Neither sibling
unit spec can cover it -- content-types-form.component.spec.ts renders no dialog,
and dot-content-types-edit.component.spec.ts stubs the form away.

Follows the existing convention in dot-uve.store.integration.spec.ts: an
.integration.spec.ts filename plus an "Integration Tests" suffix on the root
describe. Still matches Jest's testMatch, so it runs with the rest.
- Use the ECMAScript private field for the injected Injector, per
  TYPESCRIPT_STANDARDS ("do not use the private keyword for instance
  properties"). The repo already has ~620 `readonly #` fields, so this is the
  live convention rather than an aspirational one.
- Extract the license-service double, the message-service labels and the
  ActivatedRoute stub into content-types-form.testing.ts. Both form specs were
  duplicating them verbatim. Test-only: nothing there is reachable from main.ts,
  so it never reaches the app bundle.
- Add data-testid to the Name input and select it that way in the integration
  spec, per the testing rules' preference over id selectors. The banner checkbox
  keeps its inputId selector on purpose: a data-testid on p-checkbox lands on the
  host element, not the inner input that actually receives focus.
- Note in both dialog specs that the focusTrap assertions pin PrimeNG's own
  default as a tripwire, so nobody reads them as asserting something we set.

Not done, deliberately: converting the integration spec from raw TestBed to
Spectator's createHostFactory, which the testing rules would prefer. Attempted
and reverted -- under Spectator the change-detection sequence differs enough that
PrimeNG's focus-trap sentinel span ends up focused inside the dialog in edit
mode, which the raw-TestBed harness does not do. Making it pass required
weakening the "nothing is focused" assertions to accommodate the harness, which
would have made the test less faithful to production than it is now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area : Frontend PR changes Angular/TypeScript frontend code

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

Content Type create dialog: focus the Name input when the dialog opens

1 participant