Repository navigation
Undo action doesn't update block selection #12002
Description
Note: this is easier to spot if you don't use "Unified Toolbar" mode, as the block toolbar is positioned on top of the selected block, but the same behavior applies.
Related #11996 (reverted) #12052
Steps to reproduce the behavior:
For duplication:
- Open a post
- Select a block and duplicate it using the block settings
- Observe how the focus is changed to the new block
- Click the undo button
- Observe how the focus is lost
The expected result is that the focus was returned to the previous block.
For deletion:
- Open a post
- Select a block and duplicate it using the block settings
- Observe how the focus is changed to the previous block
- Click the undo button
- Observe how the focus is maintained by the previous block
Activity
oandregal commentedon Nov 16, 2018
More context. It looks like the
select( 'core/editor' ).getSelectedBlockClientId()isn't updated after the undo operation. In the first case (Duplication + Undo) what ends happening is that the selected block no longer exists.ellatrix commentedon Nov 17, 2018
Just noting that this has been an issue for a while. We don't save any selection state to in history, neither block selection, nor rich text selection. In fact, rich text selection state is local in the RichText component. This will need some rework... I thought there was an older issue about this.
ellatrix commentedon Nov 17, 2018
In my opinion, this seems like something that should be looked at soon. At least make an explicit decision whether this should be part of 5.0 or not.
mcsf commentedon Nov 21, 2018
I see a PR for this that then got closed, and I see the issue is in the 5.0 milestone. What's the status of this? From here it looks this belongs in post-5.0.
oandregal commentedon Nov 21, 2018
This is what I've considered so far:
UNDO/REDOactions. Discarded as this would cause problems when the user is editing a block.UNDO/REDOactions if the block doesn't exist. This seemed promising. The way we would do this is by catching theUNDO/REDOactions in theblockSelectionreducer and do the proper checks. The problem is that we need other state subtrees to check if a block exists in the current post (like theeditor.presentsubtree). Normally, a reducer gets this kind of data from the dispatched action, but in this caseUNDO/REDOdon't attach any.UNDO/REDO? No. The component would be updated first, causing this error.blockSelectionhistory aware by wrapping it withwithHistory. The issue with this approach is that selection could contain the same block several times andUNDOto get past selections wouldn't ensure the block selected is a valid block, it could be actually the same block that has been deleted. We'd still need the ability to check whether the block still exists in the post (same problem than option 2).I'm sharing this in the hope I've missed some obvious approach that we can implement easily. cc @WordPress/gutenberg-core for feedback.
oandregal commentedon Nov 21, 2018
@mcsf I was just writing above comment when closing #12133 At the moment I think we should merge #12052 for 5.0 and revisit this post-5.0.
oandregal commentedon Nov 22, 2018
I actually found a way that fixes the external API (the
getSelectedBlockClientIdselector) #12214aduth commentedon Nov 26, 2018
Is this closed now by #12214 ?
Tip: You can auto-close issues by pull requests using specific keywords.
aduth commentedon Nov 26, 2018
Related: #12327 (accounting for selection in undo history)
oandregal commentedon Nov 26, 2018
Not from my point of view, that's why I haven't marked it as closed. 12214 is only a workaround while this issue is not fixed.
youknowriad commentedon Nov 27, 2018
Which means this is a duplicate of #12327 (we should close one of them).
oandregal commentedon Nov 27, 2018
Oh, I see. I assumed we were using this to track the bug, but I've now seen 12327 and it looks more implementation focused than this one. Whatever you feel is useful for not losing sight of the problem would work for me.
aduth commentedon May 21, 2019
Let's consolidate to #12327. I've back-referenced to this issue and reclassified it as a bug to ensure appropriate urgency.