Repository navigation
Conversation
…ld one component:HoverCard FR2/FR3 and architecture:react-component-runtime INV6 (node replacement is a lifecycle event). The element-children wiring ran only when open state, label, id or the hook's ref changed, so a trigger replaced after mount (new children, or a child that swaps its own root without a render here) had no popup attributes, description, or hover and focus opening. A MutationObserver on the display:contents wrapper (the DOM is the external system) records the replaced first element in state, which the wiring effect depends on: on a replacement the old element is released (attributes restored, listeners removed) before the new one is wired. No extra render on mount. Tests: new children, a child that swaps its own root, and an unnamed card (red before). Record: FR2 states it, the Current-state bullet is dropped, and the verification row names the tests.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
github-actions
Bot
added
the
needs:spec-owner-review
PR Analysis Report
Generated by PR Enrichment workflow | View current CI run |
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes: the fix restores HoverCard's wiring for a replaced trigger as the HoverCard record's FR2 and FR3 require, and the new tests prove it, but the mechanism needs two adjustments before it can merge.
- Subscribe through
observeMutations(wrapper, {childList: true}, …)fromutils/sharedMutationObserverinstead of a component-ownedMutationObserver, so repeated cards share one observer. - Track a replacement with a number, keeping the last observed element in a ref, instead of storing the
Elementin state (architecture:react-component-runtimeINV1). - Change the record's "eight further implementation gaps" to seven, now that this gap is closed.
A test where the old first element stays in the DOM (children reordered) would also cover the old element's release.
[Automated review]
| ) { | ||
| return; | ||
| } | ||
| const observer = new MutationObserver(() => { |
There was a problem hiding this comment.
This duplicates observeMutations in utils/sharedMutationObserver.ts, which shares one observer across every element watched with the same options; as written, 50 cards create 50 observers. Subscribe with return observeMutations(wrapper, {childList: true}, onChildListChange); and drop the local typeof MutationObserver guard, which the utility already has.
| // mount: by new children, or by a child component that swaps its own root | ||
| // without a render here. Syncs with the DOM: on a replacement the wiring | ||
| // below re-runs, so the old element is released before the new one is wired. | ||
| const [replacedTrigger, setReplacedTrigger] = useState<Element | null>(null); |
There was a problem hiding this comment.
architecture:react-component-runtime INV1 keeps DOM nodes out of React state. Keep the last observed element in a ref, bump a number (setTriggerKey(key => key + 1)) only when wrapper.firstElementChild differs from it, and let the wiring effect depend on that number.
| `auto` does not recognize, such as a `<summary>` or an SVG link, never opens | ||
| on focus. `architecture:interaction-modality` INV4 keeps hover from being the | ||
| only discovery path. | ||
| - **A replaced trigger loses its wiring.** The card wires its trigger element |
There was a problem hiding this comment.
With this entry removed, seven implementation gaps remain, but the Compatibility/readiness row (line 48) still says "eight further implementation gaps".
|
/approve-spec 7f2d8a4 |
github-actions
Bot
removed
the
needs:spec-owner-review
…r, and count replacements CodeBunny on #9593: subscribe through utils/sharedMutationObserver's observeMutations, so repeated cards share one observer, and track a replacement as a number with the last observed element in a ref instead of an Element in state (react-component-runtime INV1). A reorder test now checks that the old first element, still in the DOM, is released; the readiness count drops to seven gaps.
|
/approve-spec d941bcc |
There was a problem hiding this comment.
Recommending approval: the change rewires a replaced HoverCard trigger as the HoverCard record's FR2 and FR3 require, and the new tests fail without the fix and pass with it. Two optional follow-ups: a test that focus on the swapped element opens the card, since the changeset claims it, and a shorter comment above the new observer effect (suggested inline).
[Automated review]
| // The wrapper's first element is the trigger, and it can be replaced after | ||
| // mount: by new children, or by a child component that swaps its own root | ||
| // without a render here. Syncs with the DOM: on a replacement the wiring | ||
| // below re-runs, so the old element is released before the new one is wired. |
There was a problem hiding this comment.
Optional (comment style): the first sentence runs 31 words over two topics in the passive. A shorter form that keeps the reason and the system note:
// New children, or a child that swaps its own root, can replace the trigger
// without a render here. Syncs with the DOM. A new first element re-runs the
// wiring below, and its cleanup releases the old element first.
User impact
A
HoverCardwraps an element whose node changes after mount. For example, a list row re-renders a name as a link once it loads, or a child component swaps its root from a<button>to an<a>. The new element is then unwired:aria-haspopup/aria-controls(named card) oraria-describedby(unnamed card), so the popup isn't announced.labelchanges.Problem and solution fit
label, the layer id and the hook's ref. A new first element changes none of those, so the effect never sees it. A child component that swaps its own root doesn't even re-render HoverCard.architecture:react-component-runtimeINV6 makes node replacement a lifecycle event that owners must handle.Expected behavior and authority
component:HoverCardFR2 (element children attach to their first element) and FR3 (trigger popup attributes). The record lists "A replaced trigger loses its wiring" under Current-state impact as a gap of FR2/FR3.architecture:react-component-runtimeINV6: a node owner handles replacement, and cleanup for the old node completes before the new node becomes active.Smallest restoration
display: contentswrapper's child list is watched through the sharedobserveMutations(utils/sharedMutationObserver), so repeated cards share one observer. The Effect names the DOM as the external system it syncs, and cleanup unsubscribes.Evidence
aria-haspopup(named) oraria-describedby(unnamed), and hover doesn't open the card.Scope
Testing
pnpm vitest run packages/core/src/HoverCard: 69 passed.pnpm check:knowledge: aligned.MutationObservernatively.Writing rubric results
Run over the FR2 row, Current-state impact, and the FR1/FR2/FR8 verification row (
docs/contributing/spec-writing.md):