Add scrollIntoView to fragment instances #32814
Conversation
github-actions
bot
added
the
React Core Team
| fiber => { | ||
| const hostNode = getHostNodeFromHostFiber(fiber); | ||
| const position = getComputedStyle(hostNode).position; | ||
| return position === 'sticky' || position === 'fixed'; |
There was a problem hiding this comment.
If a child is sticky or fixed, we really shouldn't need to call scrollIntoView on them because they're always in the viewport. At least fixed. Where as sticky might need it but not because it is sticky but because its parent might be outside the viewport which makes it no different from other nodes.
What makes a group interesting is really the parent of the hostNode, not the hostNode itself. It's the question of whether calling it would be able to shift a different parent than another node.
Unfortunately, the scrollable parent might not be the immediate DOM node parent. It might be any of the parents. In fact, commonly it's the root document.documentElement.
The other issue is that we'd have to call getComputedStyle(...) (checking overflow and position) on every parent above to figure out if it would. However, we can be smarter than that. To know if two nodes are in different scroll parents we only have to answer the question if there are any scrollable things between the shared common ancestor and each of the nodes.
In the common case the shared common ancestor is the parent node and so there are nothing in between and so no need to make a getComputedStyle call. Worst case something is like a portal in document.body whose child is not position stick/fixed and a deep node sibling. In that case we'd have to check every parent of the deep node to figure out if there's a scroll between. But this would be very unusual.
There was a problem hiding this comment.
Updated to walk up the DOM tree checking for scrollable containers between the instance and the common ancestor with the last instance checked as a way of partitioning the elements.
|
An interesting case is something like: Then you have a fragment with two portals that render into a and b. Showing both would require scrolling both the child inside a and the child inside b into view. However, if you call it on a and then b you might scroll a out of view as a result. You could flip it and scroll b into view first and then a since in general we want to show the top edge. I think technically scrolling b into view is not technically required by the API since we don't guarantee all children all visible since that may be impossible. However, if we didn't then if a is like a portal into a toolbar (for example a breadcrumb) then just showing that isn't sufficient to show the primary content which is in a nested scroll below the toolbar. So I do think we need to treat this case as first scrolling b into view and then scrolling a into view to attempt to show both. |
jackpope
force-pushed
the
fragment-refs-scrollintoview
branch
from
4b46c8c to
51e0162
Compare
|
@sebmarkbage I added a test case like the two portals example and also added a portal into a scroll container in the fixture. And also changed the semantics around the ordering of scrolling to each container based on alignToTop. alignToTop true (default call) will scroll to the first element in each container, in reverse order. alignToTop false will scroll to the last item in each container in top down order. This follows the example you showed and works in the current fixture. The remaining thing is checking the viewport to see if we can scroll to the next container without pushing the current one out. I'll work on that but might make it another PR. |
jackpope
force-pushed
the
fragment-refs-scrollintoview
branch
from
51e0162 to
faecda7
Compare
|
Comparing: ac7820a...5ede055 Critical size changesIncludes critical production bundles, as well as any change greater than 2%: Significant size changesIncludes any change greater than 0.2%: Expand to show |
jackpope
force-pushed
the
fragment-refs-scrollintoview
branch
from
faecda7 to
22f9647
Compare
This adds `scrollIntoView(alignToTop)`. It doesn't yet support `scrollIntoView(options)`.
Cases:
- No host children: Without host children, we represent the virtual space of the Fragment by attempting to scroll to the nearest edge by using its siblings. If the preferred sibling is not found, we'll try the other side, and then the parent.
- 1 host child: The simplest case where its the equivalent of calling the method on the child element directly
- Multiple host children in same scroll container:
- Here we find the first child in the list for `alignToTop=true|undefined` or the last child `alignToTop=false`. We call scroll on that element.
- Multiple host children in multiple scroll containers (fixed positioning or portal-ed into other containers):
- In order to handle the possibility of children being fixed or portal-ed, where the assumption is that isn't where you want to stop scroll, we work through groups of host children by scroll container and may scroll to multiple elements.
- `scrollIntoView` will only be called again if scrolling to the next element wouldn't scroll the previous one out of the viewport.
- `alignToTop=true` means iterate in reverse, scrolling the first child of each container
- `alignToTop=false` means iterate in normal order, scrolling the last child of each container
jackpope
force-pushed
the
fragment-refs-scrollintoview
branch
from
03c0543 to
5ede055
Compare
| }; | ||
| } | ||
|
|
||
| function isInstanceScrollable(inst: Instance): 0 | 1 | 2 { |
There was a problem hiding this comment.
Would be nice to have each of these constants represented by something named. The pattern I use elsewhere is:
const A = 0;
const B = 1;
const C = 2;
type Alphabet = 0 | 1 | 2;
function fn(): Alphabet {
}
| getFragmentParentHostFiber(this._fragmentFiber); | ||
| if (targetFiber === null) { | ||
| if (__DEV__) { | ||
| console.error( |
There was a problem hiding this comment.
Maybe a warn is sufficient? This might be a mistake but it's technically not invalid. You might not know and just optimistically do it.
| children: Array<Fiber>, | ||
| alignToTop: boolean, | ||
| ): void { | ||
| if (children.length === 0) { |
There was a problem hiding this comment.
You've really already checked this before calling this function. That's also the only caller, so this extra check is unnecessary.
| while (i !== (alignToTop ? -2 : children.length + 1)) { | ||
| const isLastGroup = i < 0 || i >= children.length; | ||
| // 1 = fixed, 2 = scrollable, 0 = neither | ||
| let isNewScrollContainer: null | 0 | 1 | 2 = null; |
There was a problem hiding this comment.
I try to avoid mixing numbers and null since VMs optimize numeric only values differently sometimes. Especially small ones. Another option might be -1.
|
|
||
| if (!isLastGroup) { | ||
| // Start a new group | ||
| currentGroupEnd = i; |
There was a problem hiding this comment.
This comment is a little confusing because it's not really starting a new group is it? It's just updating the end of the current group.
Since after the isLastGroup is true you're not going to use this value anymore, you don't really need to check that condition at all.
In fact, do you even need this variable to stick around? Isn't always just alignToTop ? i + 1 : i - 1] like you do when getting prevChild anyway.
| if (alignToTop) { | ||
| childToScrollIndex = isLastGroup ? 0 : currentGroupEnd; | ||
| } else { | ||
| childToScrollIndex = currentGroupEnd; |
There was a problem hiding this comment.
Seems suspicious that this doesn't have a isLastGroup check unlike the other one. Seems like they'd need parity in either direction.
|
|
||
| // Check if scrolling to current element would push previous element out of viewport | ||
| // alignToTop=true: current goes to top, check if prev would still be visible below | ||
| // alignToTop=false: current goes to bottom, check if prev would still be visible above |
There was a problem hiding this comment.
There's something fishy about this logic because you're not accounting for which parents will end up being the ones scrolling.
Like if they're independently scrolling parents.
|
|
||
| // Loop through the children, order dependent on alignToTop | ||
| // Each time we reach a new scroll container, we look back at the last one | ||
| // and scroll the first or last child in that container, depending on alignToTop |
There was a problem hiding this comment.
I guess one issue with this algorithm is that it doesn't account for groups interleaving. E.g.
<div />
{portal}
<div />
This would end up scrolling the first div, then the portal then the second div as if it's a new which is itself not really an issue if we assume that it has to scroll at all because the second scroll will override the first one.
However, that makes me wonder if the complex logic is needed at all or if it's as simple as just calling scrollIntoView on every child in reverse order?
This adds
experimental_scrollIntoView(alignToTop). It doesn't yet supportscrollIntoView(options).Cases:
alignToTop=true|undefinedor the last childalignToTop=false. We call scroll on that element.scrollIntoViewwill only be called again if scrolling to the next element wouldn't scroll the previous one out of the viewport.alignToTop=truemeans iterate in reverse, scrolling the first child of each containeralignToTop=falsemeans iterate in normal order, scrolling the last child of each containerDue to the complexity of multiple scroll containers and dealing with portals, I've added this under a separate feature flag with an experimental prefix. We may stabilize it along with the other APIs, but this allows us to not block the whole feature on it.