Uh oh!
There was an error while loading. Please reload this page.
[DOM] Scroll to text siblings of empty Fragments instead of the parent - #37060
Merged
Conversation
eps1lon
marked this pull request as ready for review
July 19, 2026 10:13
…eFromHostFiber callsites honestly `getInstanceFromHostFiber` returns a different kind of instance depending on the fiber tag: an `Instance` for HostComponent, a `TextInstance` for HostText, and a `Container` for HostRoot. The generic was unconstrained, and every callsite instantiated it as `<Instance>`, which is unsound whenever the passed fiber is not guaranteed to be a HostComponent. The generic is now bounded as `<I: Instance | TextInstance | Container>` so it can only be constrained to types the function can actually return. The fragment traversal helpers now carry the instance types of the fibers they can yield in their names (`traverseFragmentInstance` becomes `traverseFragmentInstancesAndTextInstances`, `getFragmentParentHostFiber` becomes `getFragmentParentInstanceOrContainerFiber`, and `getNextSiblingHostFiber` becomes `getNextSiblingInstanceOrTextInstanceFiber`), so each callsite can instantiate the generic with the union that actually matches: `<Instance>` only where a preceding HostText guard leaves HostComponent, `<Instance | TextInstance>` for traversal children, and `<Instance | Container>` for parent host fibers. Naming by instance type rather than fiber tag keeps these names stable once HostSingleton and HostHoistable, whose `stateNode` is also an `Instance`, are handled by the helpers. The fragment handle path now uses a `HostNodeWithFragmentHandles` type based on `Instance | TextInstance` since those children can be text fibers. The honest types reveal a real bug in `FragmentInstance.prototype.scrollIntoView`: when a fragment has no children and no host component siblings, the target falls back to the parent host fiber, which can be a HostRoot. Its instance is a `Container` (potentially a `Document` or `DocumentFragment`), which has no `scrollIntoView` method, so the call crashes at runtime. That callsite is marked with a TODO and a targeted suppression; the runtime fix will land separately. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…empty fragment When an empty FragmentInstance only has text node siblings, they are skipped by `getFragmentInstanceSiblings`, which only collects HostComponent fibers. The default `scrollIntoView()` call then falls back to scrolling the parent host instance instead of using the Range API to scroll to the adjacent text node, and `scrollIntoView(false)` finds no target at all and warns without performing any scroll. The test mocks the parent's `scrollIntoView` and `window.scrollTo` and asserts on the calls to pin down this behavior. Once text siblings are collected and scrolled to with the Range API, like text children already are, these assertions should flip to verifying the actual scroll target. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
An empty FragmentInstance with only text node siblings previously scrolled the parent host instance on the default `scrollIntoView()` call and performed no scroll at all for `scrollIntoView(false)`, because `findFragmentInstanceSiblings` only collected HostComponent fibers. The sibling search now also collects HostText fibers when `enableFragmentRefsTextNodes` is enabled, and both functions carry the new instance type in their names as `getFragmentInstanceOrTextInstanceSiblings` and `findFragmentInstanceOrTextInstanceSiblings`. When the scroll target fiber is a HostText, `scrollIntoView` computes the position with the Range API, using the same logic that already handled text children. That logic is extracted into a shared `scrollTextNodeIntoView` helper, and the `getInstanceFromHostFiber` calls on text fibers are instantiated as `<TextInstance>` now that the preceding tag checks guarantee it. The test added in the previous commit flips from documenting the broken fallback behavior to asserting that both scroll directions target the adjacent text sibling through `window.scrollTo`. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
eps1lonforce-pushed
the
sebbie/getinstancefromhostfiber-types-8d5392
branch
from
July 19, 2026 10:53
a96f72b to
6d6a06bComparedaltino
approved these changes
Jul 19, 2026
daltino
left a comment
There was a problem hiding this comment.
This PR refines the behavior for scrolling to empty fragments' text siblings. The naming improvements in the functions (e.g., traverseFragmentInstancesAndTextInstances) and updates to internal tests suggest thoughtful updates and alignment across DOM and React Native. The changes seem well-scoped and add clarity while maintaining functionality.
jackpope
approved these changes
Jul 19, 2026
Uh oh!
There was an error while loading. Please reload this page.
github-actionsBot
pushed a commit
that referenced
this pull request
Jul 19, 2026
#37060) When a Fragment has no children, React would consider scrolling to siblings first and then to parents. However, React only considered `HostComponent` for the siblings. Since we already have a heuristic for scrolling to `HostText`, we can reuse that same heuristic. --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> DiffTrain build for [2ba07c6](2ba07c6)
github-actionsBot
pushed a commit
that referenced
this pull request
Jul 19, 2026
#37060) When a Fragment has no children, React would consider scrolling to siblings first and then to parents. However, React only considered `HostComponent` for the siblings. Since we already have a heuristic for scrolling to `HostText`, we can reuse that same heuristic. --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> DiffTrain build for [2ba07c6](2ba07c6)
This was referenced Jul 20, 2026
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
When a Fragment has no children, React would consider scrolling to siblings first and then to parents. However, React only considered
HostComponentfor the siblings.Since we already have a heuristic for scrolling to
HostText, we can reuse that same heuristic.I used the opportunity to encode the state node types of the returned/handled Fibers in the relevant methods which makes this bug more obvious and revealed two more:
document.ownerDocument.activeElement.ownerDocumentisnullforDocumentinstead of a circular pointer. React hits this case when callingfragmentInstance.blur()on a Fragment below the document.DocumentorShadowRoot. Those don't havescrollIntoViewI also noticed we skip
HostSingleton. Seems to me we should start collecting these. TheirstateNodewill also be anInstanceand not hoisted away. Forreact-domthat would mean a Fragment below<body>would scroll the body