List View: Scroll selected block into view when single block selection changes #46895
Conversation
andrewserong
added
[Type] Enhancement
|
Size Change: +172 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
| const { innerHeight } = window; | ||
|
|
||
| // If the selected block is not visible, scroll to it. | ||
| // The hard-coded value of 110 corresponds to the position at the top of the scrollable area. |
There was a problem hiding this comment.
What does the "position at the top of the scrollable area" mean here? Is that the height of the UI at the top?
There was a problem hiding this comment.
Is that the height of the UI at the top?
Yep! Thanks for the question here — I couldn't quite work out how to describe it. The top value is from the top of the screen, but we want to calculate the value in relation to the visible area which is up to the bottom of this tabbed area:
We can get that value by doing a look up using getScrollContainer in the @wordpress/dom package, but I included a hard-coded value for now, since I think that might be a bit more performant than having to do a look up. Happy to swap it out for the "real" calculation if you think that'd be a better approach (or to try to update the comment to be clearer!)
There was a problem hiding this comment.
I definitely think it'd be better to use getScrollContainer, especially as this UI has undergone recent change, and might end up being changed further.
There was a problem hiding this comment.
Thanks, I thought there might be a solution that wouldn't cause problems if the UI changed. 👍
| firstSelectedBlockClientId: selectedClientIds?.[ 0 ], | ||
| numBlocksSelected: selectedClientIds?.length, |
There was a problem hiding this comment.
The interface could potentially be simplified to just useListViewScrollIntoView( { selectedClientIds, selectedItemRef } ), moving the logic that grabs the first clientId and the length into the hook.
| const invisibleSelectedItemRef = useRef(); | ||
| useListViewScrollIntoView( { |
There was a problem hiding this comment.
Is it possible to use the hook only here and remove the usage in ListViewBlock?
I find the naming invisibleSelectedItemRef a bit confusing, and it might be cleaner if there were only one selectedBlockRef that were passed to either the placeholder row or ListViewBlock as a prop.
| const { innerHeight } = window; | ||
|
|
||
| // If the selected block is not visible, scroll to it. | ||
| // The hard-coded value of 110 corresponds to the position at the top of the scrollable area. |
There was a problem hiding this comment.
I definitely think it'd be better to use getScrollContainer, especially as this UI has undergone recent change, and might end up being changed further.
| } ) { | ||
| useEffect( () => { | ||
| if ( | ||
| numBlocksSelected !== 1 || |
There was a problem hiding this comment.
What's the reason for the early return when there's a multiselection?
There was a problem hiding this comment.
This was mostly to try to simplify the approach — when a multiselection is applied, we can't know (in this implementation) whether or not it was done from the list view, in which case we wouldn't want the list view to be scrolled. For example, if a user selects a block, scrolls down the list view and then shift-selects a block further down, we don't want the list view to suddenly scroll back up to the initially selected block.
I imagined the main use case for scrolling the list view is changing single block selection, but happy to dig in further if you think we should handle multiselection from the outset.
There was a problem hiding this comment.
Ah, cool, that does sound like a good reason to start with single block selection. It'd be good to add a comment to that effect.
| }, [ | ||
| firstSelectedBlockClientId, | ||
| numBlocksSelected, | ||
| selectedItemRef?.current, |
There was a problem hiding this comment.
A ref changing won't trigger a re-render, so I don't think this needs to be specified.
| useListViewScrollIntoView( { | ||
| firstSelectedBlockClientId: selectedClientIds?.[ 0 ], | ||
| numBlocksSelected: selectedClientIds?.length, | ||
| selectedItemRef: isSelected ? cellRef : undefined, |
There was a problem hiding this comment.
There's a small inconsistency in that in one place the ref is bound to a row, and in the other a cell. I think it'd be tidied up by only calling useListViewScrollIntoView in ListViewBranch (removing it here) and having ListViewBlock accept a ref to its row.
|
Great feedback, thanks for the detailed review, Dan! I ran out of time today, but I'll dig through the rest of the feedback and update this PR hopefully later on this week 🙂 |
|
I've removed the hard-coded top position of the scroll container and replaced with |
|
Flaky tests detected in 1e6580a. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4169445716
|
andrewserong
force-pushed
the
try/list-view-scroll-off-screen-selected-blocks-into-view
branch
from
308cc7d
to
be7edb2
Compare
|
Update: in be7edb2 I've moved things around a bit. Some of the issues I ran into while messing around with the refs: Using a ref at the branch level seemed to cause reliability issues, as conditionally setting the ref between the real selected item or the placeholder row caused an issue once we removed it from the block level and only attached it at the branch level. I think this might have been either due to the async rendering via the async provider (I'm not too sure on the details there), or because changing the ref value doesn't cause a re-render. The result was that sometimes if you clicked from far down in the document back to the top of the document, the I think a more explicit way to do it might be to ensure that the hook is always attached, but setting it at the row level. So far I've done this by doing the following:
With the above changes, we're no longer trying to wrestle with the I'm not sure I'm entirely sold on the approach I've gone with here, but it's the one that feels the most stable in usage to me, so far. Very happy for feedback on this if and when folks have time — I'm quite happy to go in another direction with this, as the current one doesn't feel quite neat enough for my own liking, but I wasn't too sure how else to achieve it while avoiding the issue with conditionally attaching |
There was a problem hiding this comment.
I think this might have been either due to the async rendering via the async provider
Yep, that's probably it. The problem is likely that if you call the hook in ListViewBranch, the hook will be outside the <AsyncModeProvider value={ false}>, so any data updates will be async instead of sync, and may not trigger a timely re-render.
useListViewScrollIntoView needs to be defined in one of the children of the async mode provider to solve that, which seems to match what the latest commits do.
The idea of making the Placeholder its own component seems a good one and makes things more consistent, but I think it could be in a separate file with some docs to explain its purpose. Or the logic is moved into ListViewBlock or ListViewLeaf.
| ( isSelected ? ( | ||
| <PlaceholderRow | ||
| clientId={ clientId } | ||
| isSelected={ isSelected } | ||
| selectedClientIds={ selectedClientIds } | ||
| /> | ||
| ) : ( | ||
| <tr> | ||
| <td className="block-editor-list-view-placeholder" /> | ||
| </tr> | ||
| ) ) } |
There was a problem hiding this comment.
I don't quite follow the reasoning for the ternary here, as the else state seems to render the same thing as PlaceholderRow.
There was a problem hiding this comment.
I wasn't sure if there was a performance reason for using <tr> directly over a separate React component for the placeholder rows, so this ternary preserved the <tr> for the majority of the placeholders. It'd be neater to remove it and just use PlaceholderRow, though, so I'll have a play with consolidating and see how that goes.
Sounds good! Thanks for the feedback, Dan — I was a little on the fence about the approach, but I think this will help tidy it up 👍 |
|
I've moved the placeholder into a separate file and renamed it Let me know if anyone would like any other changes made, and I can keep tinkering! It'd also be good to double-check the performance stats once the performance job finishes, to make sure this doesn't introduce any regressions there. |
|
Thanks for taking this for another spin @apeatling! Yeah, in theory I wouldn't think this would affect performance like that, so worth running it again. I'll give this PR a rebase, then we can see what the results are looking like. For posterity, the current performance results as of the last commit appear to be: |
andrewserong
force-pushed
the
try/list-view-scroll-off-screen-selected-blocks-into-view
branch
from
4e47f7d
to
c51ad2e
Compare
|
Update: after a rebase, I'm still seeing |
packages/block-editor/src/components/list-view/use-list-view-scroll-into-view.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/list-view/use-list-view-scroll-into-view.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/list-view/use-list-view-scroll-into-view.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/list-view/use-list-view-scroll-into-view.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/list-view/use-list-view-scroll-into-view.js
Outdated
Show resolved
Hide resolved
|
Thanks for the detailed review @kevin940726, much appreciated! 🙇 |
packages/block-editor/src/components/list-view/use-list-view-scroll-into-view.js
Outdated
Show resolved
Hide resolved
|
After giving this a re-test, I think this PR currently introduces a regression when scrolling a very long list quickly — the list snaps back up to the top unexpectedly. It's possibly due to the switching between list view items being rendered as a placeholder versus a "real" row item — my hunch is that when the component is swapped, the hook gets fired thinking that the block has been freshly selected, or something along those lines... here's a screengrab:
I might need to give the hook logic a little more thought — we only want it to fire when a selection change has been made 🤔 |
…selection changes
…area instead of using a hard-coded value
Co-authored-by: Kai Hao <kevin830726@gmail.com>
Co-authored-by: Kai Hao <kevin830726@gmail.com>
Co-authored-by: Kai Hao <kevin830726@gmail.com>
… blocks are rendered as real ListViewBlock components
andrewserong
force-pushed
the
try/list-view-scroll-off-screen-selected-blocks-into-view
branch
from
f9b42c6
to
1e6580a
Compare
|
Update: I believe I've fixed the regression I encountered yesterday (#46895 (comment)) by doing the following:
The result is that now there's only one "version" of a selected block in the list view, so the hook's I think this is working pretty well now, but could probably use another review / re-test to confirm that I haven't missed anything if anyone gets a chance 🙂 Thanks everyone for all the reviews and help so far! 🙇 |
|
Thanks for the re-reviews! Much appreciated 🙇 |
andrewserong
changed the title
andrewserong
deleted the
try/list-view-scroll-off-screen-selected-blocks-into-view
branch
ntsekouras
added
the
Backport to WP 6.7 Beta/RC
…n changes (#46895) * List View: Try scrolling selected blocks into view when single block selection changes * Use getScrollContainer and calculate real top position of scrollable area instead of using a hard-coded value * Try rearranging things so that the ref is always attached at the row level * Move placeholder to its own file * Tidy up a little * Tidy comments * Remove unneeded optional chaining Co-authored-by: Kai Hao <kevin830726@gmail.com> * Simplify and improve logic based on feedback Co-authored-by: Kai Hao <kevin830726@gmail.com> * Remove unneeded optional chaining Co-authored-by: Kai Hao <kevin830726@gmail.com> * Revert placeholder component, update showBlock logic so that selected blocks are rendered as real ListViewBlock components --------- Co-authored-by: Kai Hao <kevin830726@gmail.com>
|
I just cherry-picked this PR to the wp/6.2 branch to get it included in the next release: 7951add |
ntsekouras
removed
the
Backport to WP 6.7 Beta/RC
What?
Fixes #46828, fixes #30432
While the list view is open, scroll the selected item into view if it is not already in view.
A caveat on the approach in this PR: I'm not sure how performant it is, so very happy for any feedback on this one!
Why?
As described in #46828, with the list view open and a long post or page, it can be confusing if you change the selection within the editor canvas, but the list view does not scroll that block into view. It can be quite easy to lose your place on longer posts.
How?
ListViewBlocklevel, and also at theListViewBranchlevel to factor in very long posts/pages where the placeholder for the selected block falls outside of the current window where "real" list view blocks are rendered. This way, if a user selects a block that is outside of the currently windowed area, we can still scroll it into view.Testing Instructions
In a really long post or page, with the list view opened, scroll the editor canvas to far down the page or post and select a block outside of the currently visible area within the list view. The list view should scroll the selected block into view.
To test the windowed area versus the non-windowed area, add a very large number of blocks (e.g. 60+) and try selecting from the very top block to the very bottom block in the list, via the editor canvas.
Testing Instructions for Keyboard
It should be possible to test this via keyboard – with the list view opened and focus on the editor canvas, press the down arrow to navigate down blocks, and as you scroll past the bottom of the screen, or past the top of the screen, the list view should always display the selected block.
Screenshots or screencast
In the below screengrab, with the list view open, we go from the first block being selected, to scrolling the editor canvas and selecting a block that is outside of the visible area of the list view. When the block is selected, the list view is scrolled so that the selected block is at the top of the visible area of the list view.
list-view-scroll-into-view.mp4