feat(focus): §6.6.4 get the focusable area + shadow delegatesFocus retarget (PR-A1)#354
feat(focus): §6.6.4 get the focusable area + shadow delegatesFocus retarget (PR-A1)#354send wants to merge 5 commits into
Conversation
…target (PR-A1) Add the engine-independent §6.6.4 "get the focusable area" retarget to elidex-dom-api: `get_the_focusable_area` (shadow `delegatesFocus` branch 6 + the 6.1/6.2 "keep focus already inside the host" shortcut), `focus_delegate`, `autofocus_delegate`, and the `FocusTrigger` enum. The focus-delegate walk uses plain (light-tree) descendants — `EcsDom::children` excludes shadow roots — recursing through get-the-focusable-area for nested hosts, per spec. The shell pointer-click path retargets the raw hit through the new `focus::focus_target_for_click` before `set_focus`; called unconditionally (it returns null for any genuine focusable area, reproducing focusing-steps step 1 without a §6.6.2 C2-aware predicate — kept C2-blind so Tab-nav / reconcile_focus are untouched, that is PR-A3's axis). First slice of the `#11-canonical-focus-update-steps` umbrella; closes the get-area half of `#11-shadow-focus-delegation`. The transition core (focusing steps / focus update steps / event sink / editing-host fallback) is PR-A2. The focus-delegate `<dialog>` branch is deferred to its real caller (showModal / autofocus) rather than landing as unreachable, untestable code. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`#focus-trigger` is not an anchor — the "focus trigger" string is defined in get-the-focusable-area; cite `#get-the-focusable-area`. "click focusable" is a §6.6.2 dfn (`#click-focusable`), not the coarser §6.6. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…lf (pre-push review) The focus-delegate descendant walk (§6.6.4 focus-delegate step 6.3 / autofocus-delegate step 1.2) tested "is the descendant a focusable area" with `is_focusable`, which deliberately omits §6.6.2 criterion 2 (a delegates-focus shadow host is NOT a focusable area; the global predicate keeps it focusable so `host.focus()` is not a no-op — slot #11-shadow-focus-delegation). A nested delegates-focus host that also carried a `tabindex` was therefore returned as itself instead of delegating into its shadow tree (step 6.4). Add a criterion-2-aware `is_focusable_area` predicate scoped to the delegate walk + a shared `child_focusable_area` sub-step (folds the byte-identical area-resolution out of both walkers); regression test for the tabindex-bearing nested host. Tighten get_the_focusable_area's safety doc (the unconditional shell retarget is correct because branch 6 only fires for delegates-focus hosts, which §6.6.2 says are not focusable areas — not because `is_focusable` rejects them). Switch the two walkers to zero-alloc `children_iter`. Document that the other pointer-click→content-focus entries (in-process / OOP iframe, legacy App) converge in PR-A2's shared focus-update-steps seam rather than sprinkling the retarget at N call sites (One-issue-one-way). /code-review + /simplify findings; mise run ci green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b608de5eed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex R1 surfaced 6 threads; this fixes the one in PR-A1's own live behavior. **F1 (P2, real, fixed)**: clicking a `delegatesFocus` shadow host that carries `tabindex` but has no focusable shadow delegate focused the *host* — `get_the_ focusable_area` returns null (no delegate), the old `unwrap_or(hit)` passed the host to `set_focus`, and the shell predicate uses `is_focusable`, which omits §6.6.2 criterion 2, so the host (tabindex) was accepted and received `FOCUS` even though a delegates-focus host is not a focusable area. Rewrite `focus_target_for_click` as the proper §6.6.4 focusing-steps step-1 gate returning `Option<Entity>`: retarget → delegate; else if the hit is itself a criterion-2-aware focusable area (`is_focusable_area`) → the hit; else `None` → `handle_click` blurs (the host must not receive focus). The retarget is still called unconditionally, so a delegatesFocus host *with* a tabindex still delegates (PR-A1 plan-review F1 preserved). Expose `is_focusable_area` from elidex-dom-api; regression test for the empty-shadow-tree host. Deferred (genuine plan-reviewed cross-PR scope boundaries, recorded with slots): - F2 iframe click retarget → PR-A2 shared focus-update-steps seam (sprinkling at N sites = strangler; the seam owns it). - F4 split focus.rs (1362→1771) → PR-A2 `#11-focus-module-split`. - F5 form-disabled delegate candidate → PR-A3 `#11-focusable-area-fieldset- inherited-disabled` (dom-api cannot depend on elidex-form — umbrella F1). - F6 delegatesFocus host `:focus` focus-chain matching → PR-A3 `#11-delegates-focus-host-focus-chain` (downstream of the retarget). F3 (FP): get-the-focusable-area does not ancestor-delegate a non-focusable shadow descendant to the host (not a §6.6.4 branch); residual blur is the pre-existing N5 / focusing-steps step-1 return semantics (PR-A2). mise run ci green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Codex R1 disposition (commit `fc9732d0`)
All six map to A1's own fix (F1), an FP, or genuine plan-reviewed cross-PR scope boundaries (A2/A3) with explicit slots. `mise run ci` green. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc9732d077
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex R2 (on fc9732d) surfaced 5 threads; one is a new A1-scope correctness regression, the rest are re-raises of R1's deferred A2/A3 work. **R2-c (real, fixed)**: the §6.6.4 focus-delegate descendant walk (`autofocus_delegate` / `first_delegate_descendant`) used `children_iter`, which caps iteration at `MAX_ANCESTOR_DEPTH` (10_000) *siblings* — a silent breadth truncation introduced by the prior /simplify pass (it replaced the uncapped, cycle-bounded `children()`). The delegate search is a spec-ordered *exhaustive* traversal, so dropping a later sibling would skip the real focus delegate. Switch both walkers to `EcsDom::child_list_uncapped` — the core-DOM primitive that exists precisely for searches where "dropping children is a correctness bug, not a safe approximation". The recursion `depth` guard is retained (it bounds nesting depth for stack safety, not sibling breadth — the dimension Codex flagged). Re-raises (R1 dispositions unchanged, genuine cross-PR scope boundaries): - R2-a (non-focusable shadow-descendant click should delegate) — reclassified from R1-FP to **real → PR-A2**: `get the focusable area` correctly returns null for a non-host descendant (verified: its branches only match a shadow *host*), so the desired "click anywhere in a delegatesFocus host delegates" behavior needs the click→focus-target *ancestor* resolution (the focusing-steps step-2 / editing-host-fallback walk), which is A2's canonical focus-update-steps, not a get-the-focusable-area branch. - R2-b form-disabled delegate candidate → A3 (`#11-focusable-area-fieldset-inherited-disabled`). - R2-d split focus.rs → A2 (`#11-focus-module-split`). - R2-e route every click entry through the retarget → A2 shared seam. mise run ci green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Codex R2 disposition (commit `eef79bca`)
R2-a/b/d/e are genuine plan-reviewed cross-PR scope boundaries (A2/A3) with explicit slots; R2-c is fixed in A1. `mise run ci` green. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eef79bcaa7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let Some(area) = get_the_focusable_area(dom, hit, FocusTrigger::Click) { | ||
| Some(area) | ||
| } else if is_focusable_area(dom, hit) { |
There was a problem hiding this comment.
When hit testing returns a non-focusable element inside a delegatesFocus shadow tree (layout hit testing walks composed_children_flat, so shadow children can be the raw hit), this only asks get_the_focusable_area about that inner element; because it is not itself a delegates-focus host, the helper returns None and handle_click blurs. W3C Shadow DOM §6.2 Focus requires a click on a non-focusable node in a delegating host's shadow tree to focus the host's delegate, so common wrapper markup like <span><input></span> in the shadow root still fails unless this path climbs to the nearest delegating host before deciding there is no focus target.
Useful? React with 👍 / 👎.
| match focus_target_for_click(&state.pipeline.dom, hit_entity) { | ||
| Some(target) => set_focus(&mut state.pipeline, target), |
There was a problem hiding this comment.
When this path retargets a clicked host to its shadow delegate, set_focus sets ElementState::FOCUS only on the delegate; existing selector matching for :focus reads that bit directly on the candidate (crates/css/elidex-css/src/selector/matching.rs:204/:356). For a delegatesFocus host with host :focus styles, the click now focuses the inner control but the host no longer matches :focus, even though W3C Shadow DOM §6.2 Focus says :focus applies to the host in addition to the focused shadow-tree element.
Useful? React with 👍 / 👎.
| /// §6.6.4 "get the focusable area" (WHATWG HTML `#get-the-focusable-area`) — map | ||
| /// a focus target that is **not itself a focusable area** to the focusable area | ||
| /// that should receive focus, performing the shadow-`delegatesFocus` retarget. |
There was a problem hiding this comment.
The Axis 5 project-context check asks reviewers to surface substantive additions to files already over 1000 lines: focus.rs was 1362 lines before this patch and is now 1790, with the new §6.6.4 delegate algorithm and tests adding hundreds more lines to the same module. Before PR-A2/A3 add more focus-update logic here, splitting the delegate algorithm/tests into a focused submodule would reduce the decision surface and keep the canonical focus path easier to review.
Useful? React with 👍 / 👎.
| if is_focusable_area(dom, child) { | ||
| Some(child) |
There was a problem hiding this comment.
When a delegatesFocus host's first focusable-looking descendant is disabled only through the form subsystem (FormControlState.disabled, e.g. fieldset inheritance), this predicate still treats it as a focusable area because elidex_dom_api::focus::is_focusable is attribute-only; the shell overlay rejects it later in set_focus, causing blur instead of continuing to the next eligible descendant. Since the new delegate search runs before that overlay, clicks on such hosts select the wrong delegate and fail to focus a later enabled control.
Useful? React with 👍 / 👎.
| if dom.is_host_including_ancestor_or_self(target, focused) { | ||
| return Some(focused); |
There was a problem hiding this comment.
If script or an older path has already focused a delegatesFocus host itself (possible today because is_focusable intentionally omits criterion 2), a subsequent click on that host takes this keep-current-focus branch because the host is an inclusive ancestor of itself and returns the host instead of its delegate. That leaves the click path in exactly the state this PR is meant to avoid: a delegating shadow host remains the focused area until some later focus move.
Useful? React with 👍 / 👎.
1 participant
Summary
First slice (PR-A1) of the canonical focus-transition program (structural slot
#11-canonical-focus-update-steps). Builds the WHATWG HTML §6.6.4 get the focusable area retarget in engine-independentelidex-dom-api, and wires the content-thread click→focus path through it so a click on a shadow host withdelegatesFocusfocuses the host's delegate instead of the host.What lands
get_the_focusable_area(dom, target, trigger)(§6.6.4#get-the-focusable-area) — branch 6 (delegates-focus shadow host →focus_delegate) with the 6.1/6.2 "keep focus already inside this host" shortcut. Branches 1–5 (image-map area / scrollable region / viewport / navigable / navigable container) are unmodelled-or-cross-frame in elidex's single-EcsDommodel and documented as falling through; branch 7 = null.focus_delegate/autofocus_delegate/ first-focusable-descendant (§6.6.4#focus-delegate/#autofocus-delegate) — light-tree descendant walks (the spec is explicit these are not shadow-including), recursing throughget_the_focusable_areafor nested hosts.FocusTrigger{Click,Other}— the spec's focus trigger (drives the autofocus click-focusable filter).focus_target_for_clickwired at the content-threadhandle_click=get_the_focusable_area(hit, Click).unwrap_or(hit), called unconditionally (branch-6-only non-null ⇒ reproduces the focusing-steps step-1 gate without a §6.6.2-complete predicate).Spec / criterion-2 correctness (pre-push review fix)
is_focusabledeliberately omits §6.6.2 criterion 2 (a delegates-focus shadow host is not a focusable area — kept focusable globally sohost.focus()is not a no-op; slot#11-shadow-focus-delegation). The descendant walk therefore uses a criterion-2-awareis_focusable_area(via the sharedchild_focusable_areasub-step) so a nested delegates-focus host that also carriestabindexdelegates into its shadow tree (step 6.4) rather than being returned as itself. Regression-tested.Scope boundary (→ PR-A2)
App) converge in PR-A2, whenset_focusbecomes a thin caller of the canonical §6.6.4 focus update steps and the retarget moves to that single shared seam — rather than sprinklingfocus_target_for_clickat N call sites (One-issue-one-way: no strangler middle state). Documented in-code.<dialog>focus-delegate branch omitted (<dialog>is not a valid shadow host; reached only fromshowModal()/autofocus → slot#11-flush-autofocus-candidates-page-load).Layering / ECS-native
Algorithm lives entirely in engine-independent
elidex-dom-api; the shell holds only a thin wrapper (Layering mandate). State is read from theShadowRootcomponent +ElementState::FOCUSbit — no side-store.Tests / gate
cargo fmt+mise run cigreen (11751 tests, check/doc/lint/deny). Pre-push gate:/elidex-plan-review(F1 spec-bug caught pre-impl) +/code-review(C2 bug) +/simplify+/review+/elidex-review.🤖 Generated with Claude Code