fix(content-types): focus the Name input when creating, nothing when editing - #36820
Open
oidacra wants to merge 5 commits into
Open
fix(content-types): focus the Name input when creating, nothing when editing#36820oidacra wants to merge 5 commits into
oidacra wants to merge 5 commits into
Conversation
…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
8 tasks
Contributor
|
Claude finished @oidacra's task in 2m 38s —— View job PR Review
I reviewed the production changes ( New IssuesNone. The production change is small and sound:
Minor (non-blocking)
Notes
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.
…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
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.
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.
create/:type)edit/:id)The Name input already declared the intent to be focused —
dotAutofocuson the input plus a.focus()in the form'sngOnInit, neither gated by mode — and both were undone afterwards by PrimeNG'sp-dialog:focusOnShowdefaults totrue, andonAfterEnter()schedulesgetFocusableElements(content)[0].focus()using the transition duration (~150ms), which outlivesdotAutofocus's 100ms timeout.getFocusableElementswalks 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
focusOnShowon thep-dialog, in both modes, so PrimeNG stops competing for the initial focus. There is no timer race to win — the competitor is removed.dotAutofocusdirective and thengOnInit.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.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:
.focus()inngOnInitwas dead code. RemovingdotAutofocusmade the create-mode tests fail, which proved the directive'ssetTimeoutwas what actually landed the focus all along.mainthep-dialogbound nofocusOnShowat all, so PrimeNG's default oftrueapplied 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.focusTrapis deliberately left unbound — it is an independent code path ([pFocusTrapDisabled]="focusTrap === false"), so the dialog keeps trapping Tab.closableis untouched and still differs by mode as before.Changes
dot-content-types-edit.component.tsreadonly 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 thep-dialog.content-types-form.component.tsafterNextRender, only when not in edit mode.content-types-form.component.htmldotAutofocusremoved from the Name input; the component now owns the decision.dot-content-types-edit.component.spec.tsfocusOnShowdisabled asserted in both modes,focusTrap/closablepinned unchanged,?open-config=truecovered.content-types-form.component.spec.tscontent-types-form-dialog-focus.spec.ts(new)p-dialog, asserting actualdocument.activeElementfor create and edit, plus a reproduction of the original defect.Acceptance criteria
#content-type-form-namewhen 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.inputevent on the focused element; see manual verification for the real-keystroke path.CONTENT_EDITOR2_ENABLEDbothtrue(banner visible) andfalse.CONTENTandWIDGETfor real focus.?open-config=true.focusTrapandclosableasserted unchanged in both modes. ESC-to-close and Tab order are untouched by construction (no test asserts them; see manual verification).Test plan
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=trueassertions 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 ap-dialog.Two jsdom constraints the new spec works around, both commented in place:
onAfterEnter()is invoked directly instead of awaited.afterNextRenderhooks are driven withApplicationRef.tick(), which runs them synchronously and so coexists with the fake timers needed to control PrimeNG's own timer.isVisible/offsetParentcheck would make itsfocus()a no-op and the race under test would never happen. The spec shimsHTMLElement.prototype.offsetParentinbeforeAlland restores it inafterAll. 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
/dotAdmin/#/content-types-angular/create/content/fields— the caret must be in Content Name, with no visible jump to the banner checkbox.CONTENT_EDITOR2_ENABLED=false(AC3).create/widget(AC5 in a real browser).Edit
?open-config=true.Both