-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Writing Flow: Allow Escape key to deselect blocks and selection during multiselection #48904
Writing Flow: Allow Escape key to deselect blocks and selection during multiselection #48904
Conversation
Size Change: +20 B (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
This is feeling really good to me. Mostly intuitive and transparent, and depending on feedback by other interested parties, I'd love to see this land. Here's a GIF showing selecting multiple paragraphs, pressing Escape to deselect. Pressing Escape again to enter select mode. If only one paragraph is selected, Esc goes straight to select mode. Esc also closes the list view when selecting inside that: The only gotcha is: if you select multiple blocks inside the list view, press Escape to close the list view, you still have multiple blocks selected in the canvas, and I would then expect Esc to deselect them at that point. What do you think? Edit: it seems like it might be because once you close the List View, focus is expectedly on the list view button. Is that why Esc doesn't deselect blocks in the canvas in this state? @alexstine if you have time to review this PR, I'd love your input. The behavior seems fairly intuitive to me, but I could be missing something obvious. |
@andrewserong See above. That might be a bug of sorts. It should be impossible to close the list view if blocks are still selected I would think. If you select blocks in the canvas, does that not sync to the list view via React store? |
Thanks for the feedback!
Yes, the Escape key handling is pretty specific as to where focus currently is, so I'd expect Escape when focused on the list view button to do nothing since it's neither within the editor canvas, nor the sidebar/list view. In terms of fixing this use case, though, I was actually imagining that we'd look at merging both this PR and #48708 so that while focus is within the list view, the first Escape press would clear the selection instead of closing the list view. I've kept the PRs separate since their scope is a little different (i.e. the List View PR doesn't need to worry about the writing flow behaviour, and the writing flow doesn't have to worry about what's going on in the List View). What do you think about having both in place? One benefit of keeping the PRs separate, is that we might be able to merge #48708 sooner, because it doesn't involve changing writing flow behaviour, where I thought this PR might be a little more contentious if folks were expecting the switch-to-Navigation-mode behaviour. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this change makes sense as far as select mode behaviour goes, and a quick test shows it's working as expected.
Agree we should tackle the list view behaviour in #48708.
One thing I'm not sure about is where do we expect keyboard focus to land when deselecting the blocks in the canvas? Currently it's landing on the editor iframe.
@andrewserong @jasmussen Sorry about my earlier comment, I only now realize this is a different PR from the list view PR. Not enough coffee I guess this morning. To answer @tellthemachines , I believe focus should always be in a consistent place. When block selection occurs, can you grab the starting clientId of the block selected and when deselecting all blocks, drop the user back where they started selecting? That is probably the best way to go about this. Thanks. |
e01bc2f
to
ae95964
Compare
Hi folks, I'm just dusting off this old PR that allows for deselecting blocks in the editor canvas by pressing the Escape key when multiple blocks are selected. I've updated the logic so that if multiple blocks are selected when the 2023-06-20.15.16.24.mp4We're pretty close to the feature freeze for 6.3, but I just thought I'd give this PR another try in case we wind up being close to something that could work for now. Happy for any feedback or to park it and revisit for 6.4 if need be 🙂 |
Flaky tests detected in ae95964. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5318792235
|
I'm not over the details but I've been testing this in the post and site editors and it works as described, and definitely an improvement on trunk. Nice! Here's me flailing around in trunk trying to hit edit, and entering Select mode... and generally losing patience: In this branch: When selecting in list view, confirming that the first escape closes the list view (looking at above comments that suggest there's a possible follow up) Selecting in list![2023-06-20 15 50 54](https://github.com/WordPress/gutenberg/assets/6458278/00c1a3a0-0cdc-4917-bffb-b08a973ccdd2)I'll let others have the final say, but LGTM |
Thanks for taking this for a spin @ramonjd! Let's see what other folks think, too 👍
Yes, to keep things granular and look at behaviour one piece at a time, this PR just looks at behaviour when we're within the editor canvas. I thought I'd wait and see how we go with the direction in this PR, and if this one winds up looking good / landing, then I'll dust off #48708 as a follow-up. 🙂 |
Thanks so much for keeping on this, it will be such a quality of life improvement. The basic behavior works really well. Here's a GIF showing paragraphs and an image, with me multi selecting both, then pressing Esc to deselect. Also, if just a single block is selected, Esc goes into select mode again, which is a fine compromise: I'm seeing an issue with esc in the list view. It isn't entirely clear to me that it is an issue, or intentional, and I don't know that it should be blocking. But for clarity, this GIF shows me multi selecting multiple blocks using the list view, and then pressing Esc, which of course closes the list view: The tricky part is that once it's closed, the multi selection remains, no matter how much I press Esc. I think that if I were to focus the canvas, then the Esc keyboard shortcut would work again, but I'm not sure. What I would expect here, and correct me if this would be bad practice, is that Esc in this case should be a global shortcut that always deselects. Perhaps unless focus is inside the list view when it acts as a modal. Then pressing Esc twice would first close the list view, and then de-select. Make sense? |
@jasmussen There is another in progress PR that would bring escape to deselect to the list view. Press escape to deselect then escape will close the list view. Thanks. |
Noting that Escape failing to deselect blocks was also flagged as an issue in #51356. |
@jasmussen thanks for taking this for a spin!
Yes, that's the case. This PR is only adjusting the behaviour for when focus is within the editor canvas. It'd be good to look into further follow-ups for when focus is in other places. For the list view, we'll look at escape key behaviour separately over in #48708 as there's more nuances to getting it working correctly within the list view. I could also imagine follow-ups that might expand the behaviour to other areas such as if focus is on the toolbar, or otherwise not within the editor canvas, but we might need to tread carefully if exposing global shortcuts like that to make sure we don't introduce unexpected behaviour. For now, is everyone happy with the behaviour within the editor canvas? I'm keen for us to land these improvements in incremental steps if we can since the change for the editor canvas is much simpler to figure out than the list view behaviour, which might not make it in time for 6.3. Thanks for taking a look, everyone! 🙇 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Wonderful, thanks for approving @alexstine! I'll merge this one in now. |
…n during multiselection (WordPress#48904) * Writing Flow: Try allowing Escape key to deselect blocks and selection during multiselection * Try selecting the first block of the deselection within the unselect action
What?
Fixes: #47172, #5524
This is a small PR that updates the check for switching to navigation mode when a user presses Escape. It's small but has a side effect:
When multiple blocks are selected and a user presses Escape, the blocks will be deselected, along with any partial selection across blocks. This is because the
core/block-editor/unselect
shortcut will be allowed to fire.Why?
There appear to have been a number of iterations surrounding this behaviour over the past year or two, however the recent discussion over in #47172 highlights that it'd be good to make it easier for users to be able to quickly and easily deselect blocks via the keyboard.
From my perspective, when multiple blocks are selected in the editor canvas, it feels more likely that someone trying to press Escape will be looking to remove their selection rather than switch editing modes. I'm only a sample size of one, but to me, this feels a bit more intuitive.
How?
Update the check when switching modes to Navigation to only do so if not in a multiple block selection. This means that when multiple blocks are selected, the keyboard shortcut for clearing blocks will fire.
Then, in the logic for the unselect shortcut, if we're in a multi selection, select the first block of the selection. To a user, this should have the "feeling" as though the multi selection has been deselected, with focus redirected back to the beginning of the selection.
Testing Instructions
Screenshots or screencast