Scroll to previous location in document with editor back button #73737
Conversation
tellthemachines
requested review from
aaronrobertshaw,
andrewserong and
ramonjd
tellthemachines
added
[Type] Enhancement
|
Size Change: +816 B (+0.03%) Total Size: 2.57 MB
ℹ️ View Unchanged
|
andrewserong
left a comment
andrewserong
left a comment
There was a problem hiding this comment.
Hey that's pretty impressive, and seems to be getting me back to where I was so far 👍 . Something I'm curious about: rather than using scroll position, would it be possible to select the block that was previously selected, i.e. possibly by using the id, or passing the clientId across? That way the user could be dropped back into the same block selection they were on previously?
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
🚀 Kapture.2025-12-04.at.14.59.05.mp4
This is how I thought it would work, but doesn't the client id (and therefore the id) change every time the block list is rendered? is there another way? |
Good idea, I just pushed a change that does that! |
It's when the blocks are initially parsed I believe, so if they already exist in state, they'll persist it seems. At least when I inspected markup going back and forth, the clientIds in the markup seemed to be the same. |
Thanks for setting my assumption straight there! |
packages/edit-site/src/components/block-editor/use-navigate-to-entity-record.js
Outdated
Show resolved
Hide resolved
| // Store selected block for current location | ||
| const currentPath = | ||
| window.location.pathname + window.location.search; | ||
| if ( currentSelectedBlockClientId ) { | ||
| window.sessionStorage?.setItem( | ||
| `gutenberg_selected_block_${ currentPath }`, | ||
| currentSelectedBlockClientId | ||
| ); | ||
| } |
There was a problem hiding this comment.
Just curious about dissimilarities between edit-post and edit-site implementations?
Can we store the session record with the useNavigateToEntityRecord callback and handle restoration via useNavigateToPreviousEntityRecord, instead of using a separate useRestoreBlockSelection hook?
I'm sorry, but I haven't played with code enough, and there might be obvious drawbacks I'm missing here.
There was a problem hiding this comment.
Good question! Yeah I just hooked onto the existing dispatch in the post editor onNavigateToEntityRecord callback, but the site editor one is quite different in that it doesn't dispatch anything to the store but just calls history.navigate. I'm not sure why the two different approaches were used originally. I guess we could try to refactor to use the same approach in both places? But I was trying to keep the changes minimal in this PR 😅
There was a problem hiding this comment.
I guess we could try to refactor to use the same approach in both places? But I was trying to keep the changes minimal in this PR.
100%. We can try to consolidate approaches in a follow-up PR. I can give it a try if I find some time this week.
There was a problem hiding this comment.
We can't use the same approach in both at the moment, because they use different routing behaviors.
There was a problem hiding this comment.
I understand, but the implementation pattern can be similar, which selects the client ID temporarily stored in the state.
Though it's no longer important, I like your suggestion more - #73737 (review) 😄
|
Nice, I think this can be a solid QoL change. Agreed on using the block selection as the trigger as it plays better into existing systems and would be more accurate. |
| // Use a small delay to ensure content is rendered | ||
| setTimeout( () => { | ||
| selectBlock( selectedBlockClientId ); | ||
| }, 100 ); |
There was a problem hiding this comment.
This seems like it can cause potential headaches to manage down the road. Would it make sense to handle this where the block selection triggers the scroll instead, and ensure it suspends until render is done? The code here seems like it should only be concerned with restoring selection not for accounting for the render cascade.
| const onNavigateToEntityRecord = useNavigateToEntityRecord(); | ||
|
|
||
| // Restore block selection when navigating | ||
| useRestoreBlockSelection(); |
There was a problem hiding this comment.
Why is this fix edit-site specific. I'd have expected most of the change to happen actually in "editor" package and potentially some small "implementation" changes in the onNavigateToEntity and onNavigateBack callbacks (edit-site and edit-post)
| const onNavigateToEntityRecord = useCallback( | ||
| ( params ) => { | ||
| // Capture currently selected block before navigating | ||
| const currentSelectedBlockClientId = getSelectedBlockClientId(); |
There was a problem hiding this comment.
This feels like something that should come from the "params" no?
youknowriad
left a comment
youknowriad
left a comment
There was a problem hiding this comment.
here's a suggestion. The onNavigatetoEntity and onNavigateBack both impact the "postType" and "postId" passed down to the Editor component.
What if this just behaves in the exact same way. What if we had an initialSelection prop in Editor component to refer to the thing we want to select first when we load the editor.
This would prevents setTimeout hacks and things like that potentially.
tellthemachines
force-pushed
the
try/back-nav-to-exact-place
branch
from
5ccea8d to
9daf681
Compare
|
Flaky tests detected in 5c65a62. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/20081831867
|
tellthemachines
requested review from
ajitbohra and
fabiankaegy
as code owners
|
Thanks for the feedback everyone! I've made a few changes according to the suggestions above:
Let me know what you think! |
| onNavigateToEntityRecord( { | ||
| postId: ref, | ||
| postType: 'wp_block', | ||
| selectedBlockClientId, |
There was a problem hiding this comment.
Instead of having users of onNavigateToEntityRecord passing the selectedBlockClientId all the time. What if we augmented onNavigateToEntityRecord when we receive it in the settings prop of BlockEditorProvider. That way the consumers wouldn't have to care about doing this all the time. I'm 80% convinced about my own suggestion though, so let me know what you think haha
There was a problem hiding this comment.
This is a good question. I did a bit more digging on the other places onNavigateToEntityRecord is called and I don't think we really need it anywhere else apart from the block and template part edit functions, and possibly the NavigationViewButton but in my testing that one doesn't work because the clientId of the Nav Link block changes when returning to the template editor (tested with a Nav block inside a Header template part in the blog home template).
This bug led me to unearth that whenever we're editing a post or page (both in the post and site editors) with "show template" enabled, the clientId of the blocks on canvas also changes, which breaks the back navigation to the selected block.
Unfortunately I'm not sure why this happens, or how to fix it.
Regarding your original question 😅 when not in show template mode, augmenting onNavigateToEntityRecord in the provider seems to work just as well as the current approach. I guess doing it that way will make things easier if we want to implement going back to the selected block in other places, so I'm slightly inclined towards it even though we currently only need that behaviour in 2 or 3 out of 9 places where onNavigateToEntityRecord is called.
| const { useHistory } = unlock( routerPrivateApis ); | ||
|
|
||
| // Store selected blocks per path in memory (not persisted across page reloads) | ||
| const selectedBlocksByPath = new Map(); |
There was a problem hiding this comment.
Instead of using a path, why not just have the id in the url (like all the other args)
There was a problem hiding this comment.
Mostly because I didn't want it to stick around on reload! Though I suppose it wouldn't do anything, because clientIds would change.
There was a problem hiding this comment.
I think the two separate behaviors is an adding a bit of complexity. I'd prefer if we just use the url
| templateId={ postWithTemplate ? postId : undefined } | ||
| settings={ settings } | ||
| settings={ editorSettings } | ||
| initialSelection={ initialBlockSelection } |
There was a problem hiding this comment.
The one thing that I'm uncertain about is that we pass "postId" to the Editor component. This means the "blocks" property of the "post" might or might not be present (the post might not have parsed the blocks yet).
I guess we're making an assumption that the the blocks are already parsed and clientIds stable. IT seems ok in the context of entity navigation but wanted to highlight this.
| hasRestoredSelectionRef.current = true; | ||
|
|
||
| // Use setTimeout to ensure blocks are fully rendered before selecting | ||
| const timeoutId = setTimeout( () => { |
There was a problem hiding this comment.
I'm guessing the timeout is needed because this code needs to run after the resetBlocks actions is called.
We should consider whether we should add a initialSelection to BlockEditorProvider (and pass down the prop) which would allow us to call this at the right moment.
There was a problem hiding this comment.
Also, maybe checking if the block is still the tree? Pretty edge casey I guess. I'm just thinking if block is deleted or changed (collaborative editing?) since navigation
There was a problem hiding this comment.
if block is deleted or changed (collaborative editing?) since navigation
If that happens nothing gets selected, which should be fine.
There was a problem hiding this comment.
We should consider whether we should add a
initialSelectiontoBlockEditorProvider(and pass down the prop) which would allow us to call this at the right moment.
I'm not sure I understand. What would be the right place to select the block if not the Editor component?
There was a problem hiding this comment.
What would be the right place to select the block if not the Editor component?
The component responsible for actually inserting the blocks into the canvas aka BlockEditorProvider
There was a problem hiding this comment.
Do you mean passing the previously selected block as the selection prop to useBlockSync? I can't get that to work because useBlockSync expects a selection object with clientIds, and at the point we'd have to calculate those clientIds in the provider, the block order still corresponds to the previous screen, so the clientId can't be found. I added some console.logs and with or without the timeout, the code in the Editor component still runs after any code in the Provider.
youknowriad
left a comment
youknowriad
left a comment
There was a problem hiding this comment.
I think the flow now is a lot clearer.
| settings={ settings } | ||
| initialEdits={ initialEdits } | ||
| useSubRegistry={ false } | ||
| initialSelection={ initialSelection } |
There was a problem hiding this comment.
Is this prop doing anything? I thought we would pass down the initial selection as a fallback or in place of the getEditorSelection value in the Editor component.
There was a problem hiding this comment.
No it isn't, well spotted! Leftover from a previous iteration 😄
ramonjd
left a comment
ramonjd
left a comment
There was a problem hiding this comment.
Just a flyby test. It's a great start, and very refreshing to return the footer template part after editing it 💯
I like the use of useNavigateToEntityRecord object structure to keep things consistent and backwards compatible.
| const history = useHistory(); | ||
| const currentPath = window.location.pathname + window.location.search; | ||
| const initialBlockSelection = | ||
| selectedBlocksByPath.get( currentPath ) || null; |
There was a problem hiding this comment.
I think Map.get() already returns undefined, unless null is specifically required?
| // Store selected block for current location if provided | ||
| const path = window.location.pathname + window.location.search; | ||
| if ( params.selectedBlockClientId ) { | ||
| selectedBlocksByPath.set( path, params.selectedBlockClientId ); |
There was a problem hiding this comment.
Just checking, how many items need to be stored in selectedBlocksByPath? As far as I can see the Map grows indefinitely as users navigate? Could we set a limit or clear during a session?
| hasRestoredSelectionRef.current = true; | ||
|
|
||
| // Use setTimeout to ensure blocks are fully rendered before selecting | ||
| const timeoutId = setTimeout( () => { |
There was a problem hiding this comment.
Also, maybe checking if the block is still the tree? Pretty edge casey I guess. I'm just thinking if block is deleted or changed (collaborative editing?) since navigation
|
Thanks for all the feedback, folks! I've updated the PR to augment In the course of work I found that reselecting the previously selected block won't work if "view template" is enabled in a post because the clientIds of the blocks change on navigation. I'm going to have a poke around and see if I can work out what's going on there. |
It looks like there's some cache clearing going on in useEntityBlockEditor. This is probably for good reason so I'm trying another approach in this PR: instead of using the clientId, which can change, we use the block position and content as identifiers. This has also enabled reselection to work when using the "View" button in a Navigation page link and returning to the template editor. I had to add a couple of utils to translate the block identifiers to and from its clientId, but apart from that the implementation remains pretty similar. This should now be ready for another round of feedback! |
packages/editor/src/utils/index.js
Outdated
| export { | ||
| useGenerateBlockPath, | ||
| useRestoreBlockFromPath, | ||
| } from './block-selection-path'; |
There was a problem hiding this comment.
IMO, this should be private APIs, just in case we change implementation later, so we don't have to worry about deprecations.
tellthemachines
force-pushed
the
try/back-nav-to-exact-place
branch
from
b405959 to
5c65a62
Compare
|
Any reason for the switch to "block path" instead of "block id". I'm guessing to avoid the parsing issues but did we actually notice any issues? Cause I feel like it's way too complex and introduce a new concept which might not be worth it for a small UX improvement. |
Yes! The clientIds aren't stable when templates are show in post and site editor. More context in this comment. It's an important UX improvement though 😄 and there will likely be other places where we'll want to navigate back to a particular selection in future. For instance, if we want to click on a link to go through to a page, like the "view" for Nav block page links now does, it would be good to be taken "back" to the place we clicked from. |
|
I'd prefer if we try to solve this using this approach instead #72405 (comment) (might be worth exploring in its own PR though because it's an impactful change on its own but it might have more benefits than just what this PR needs) |
What?
Fixes #73679.
Keeps track of scroll position when navigating to a new entity with
onNavigateToEntityRecordand applies the position when navigating back withonNavigateToPreviousEntityRecordin both the post and the site editors.Testing Instructions
back-button.mp4