Writing flow: allow select all from empty selection #33446
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +128 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
ellatrix
commented
Jul 15, 2021
| @@ -75,7 +76,7 @@ function MaybeIframe( { | |||
| contentRef={ contentRef } | |||
| style={ { width: '100%', height: '100%', display: 'block' } } | |||
| > | |||
| { children } | |||
| <WritingFlow>{ children }</WritingFlow> | |||
There was a problem hiding this comment.
In the future, writing flow behaviour will be set on the iframe body element.
|
I'm seeing the following warning in the editor when building on this branch: |
Mamaduka
reviewed
Jul 15, 2021
It's still different from what you can observe in WP 5.7, you can click anywhere on the page and |
That doesn't make any sense. We shouldn't add globals shortcuts like that. What if you have multiple editors on a page? Or a different app or editor? |
I agree. I think we had incorrect behavior pre 5.8, and now this feels like a regression. Personally, I like this logic more. |
|
I don't mind re-adding the behaviour when we catch focus within the canvas, but outside of it, it feels wrong. We even may have other components that do custom things for cmd-a. I could very well imagine a code editor in the inspector for example. Shortcuts should generally be local behaviours. |
|
@audrasjb reported the issue so it looks like the old behavior has some usage on the post editor page. I guess it's fine to change it to the writing canvas because of the reasons @ellatrix mentioned because we won't be able to replicate it on the widgets editor page where several instances of the block editor can be present. |
youknowriad
reviewed
Jul 15, 2021
| @@ -102,6 +102,7 @@ export default function useTabNav() { | |||
| const direction = isShift ? 'findPrevious' : 'findNext'; | |||
|
|
|||
| if ( ! hasMultiSelection() && ! getSelectedBlockClientId() ) { | |||
| setNavigationMode( true ); | |||
There was a problem hiding this comment.
How is this related to this PR? What's the purpose of this change?
There was a problem hiding this comment.
It is needed to make e2e tests pass. We enable navigation mode when the user tabs into the writing flow focus capture, but with tab index set to -1 and when clearing block selection, focus will no longer move to the interface content element, but rather to the editor styles wrapper. When tabbing, it will not go through to focus capture to enable navigation mode, so we have to enable navigation mode when the user tabs in the content and there is no block selection.
There was a problem hiding this comment.
I don't think I follow here, why do we need to auto-enable navigation mode implicitly (not hitting "Escape").
If there's no block selected and we tab into the content, can't we just select the first one?
There was a problem hiding this comment.
We could, but that's not how the behaviour was. I want to preserve the current behaviour. This was discussed once with the some people from the accessibility team that if no block is selected, and you tab into the content, the editor should be in navigation mode to select a block. Supposedly it wouldn't make sense to decide for the user and pick the first one from which they then have to press Esc to pick another one. But all of this is a bit more opinionated, so for this PR I just want to make the tests pass and not change the current behaviour. :)
There was a problem hiding this comment.
👍 I didn't know this was the current behavior, that's fine by me.
|
Re-running some intermittent failures |
|
@ellatrix that widget test is one of the latest intermittent failing tests with reusable blocks, we should probably skip both. |
ellatrix
added a commit
that referenced
this pull request
Jul 15, 2021
youknowriad
pushed a commit
that referenced
this pull request
Jul 15, 2021
|
Thanks all for working on this issue 👍 |
|
Hi, while testing WP 5.8 RC4 package, I noticed it didn't fix the issue. |
|
Ok so if you first put the focus on the editor area then it works fine. I think it’s not as good as the previous behavior, but it’s not a blocker anymore for 5.8 in my opinion. |
|
@audrasjb The previous behavior has actually been considered wrong. See #33446 (comment) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes #33358. "Select all" was made only for when a block selection already exists, but it seems unexpected to people to not be able to select all blocks from an empty selection (e.g. after clicking in the canvas to clear block selection).
This PR should enable "select all" when no blocks are selected but the canvas is focussed.
How has this been tested?
Try this both in the normal post editor and the page template editor (iframed).
cmd+a.Screenshots
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).