Make sure when on last block focus cannot enter the block. #37965
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
alexstine
added
[Type] Bug
alexstine
requested review from
youknowriad,
ellatrix,
kevin940726 and
tellthemachines
8 tasks
|
Size Change: +21 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
alexstine
commented
Jan 14, 2022
| @@ -67,6 +68,7 @@ const SettingsHeader = ( { sidebarName } ) => { | |||
| className={ `edit-post-sidebar__panel-tab ${ templateActiveClass }` } | |||
| aria-label={ templateAriaLabel } | |||
| data-label={ __( 'Template' ) } | |||
| data-focus-at-end | |||
There was a problem hiding this comment.
Not sure if this is needed. Not familiar with how Template Mode works.
| @@ -196,6 +196,15 @@ function BlockSelectionButton( { clientId, rootClientId, blockElement } ) { | |||
| if ( navigateDown ) { | |||
| nextTabbable = focus.tabbable.findNext( blockElement ); | |||
|
|
|||
| if ( null === getNextBlockClientId( clientId ) ) { | |||
There was a problem hiding this comment.
Couldn't think of a better way to do this. Open to feedback on how to better focus the Settings Sidebar. This code was great before these new Blocks, it just doesn't work with the more dynamic Blocks.
|
So just to clarify, the problem here is that we get stuck tabbing between the last block and the "button" inside it right instead of going to the sidebar of the editor? It feels to me the solution here is not to add a specific element to tab into like the PR tries to do with Basically I think the fix should be somewhere in these lines https://github.com/WordPress/gutenberg/pull/37965/files#diff-11abd7881539ce65efd801429f377af4876374040aef15b4255bc96658b790c2R208-R212 where we should find the next tubbable after the last block (but not after the last block's label which include elements inside the block itself) |
|
@youknowriad Yes, that is correct. You get stuck there and you can only move backwards. How exactly could I determine the next element would be inside a block? It seems difficult to determine as there are not really any unique classes I could ignore. Maybe trying to detect the parent element? Either way, there's going to be some gymnastics involved. I really don't favor my solution either but I am not sure we should be reliant on classes to tell us if focus would be in block content or not. 😕 |
…for last block to keep it somewhat efficient.
|
@youknowriad I fixed it but the solution is super ugly. Let me know your thoughts about this approach.
I think it was a rather interesting idea of mine but not sure how reasonable it is as there's a lot of DOM interaction and checking. |
youknowriad
reviewed
Jan 17, 2022
| @@ -196,6 +196,28 @@ function BlockSelectionButton( { clientId, rootClientId, blockElement } ) { | |||
| if ( navigateDown ) { | |||
| nextTabbable = focus.tabbable.findNext( blockElement ); | |||
There was a problem hiding this comment.
I've been looking at this PR again and for me the root cause is in this line basically what we're saying here is:
If there's no next block, focus the next tabbable element after the current block element, the problem is that this doesn't exclude the content of the current block (because it comes after blockElement), so I think a proper if is just to continue calling "findNext" until we're out of the current block.
Something like this maybe?
nextTabbable = blockElement;
do {
nextTabbable = focus.tabbable.findNext( nextTabbable );
} while ( nextTabbable && blockElement.contains( nextTabbable ) );
There was a problem hiding this comment.
I am glad you knew what I was trying to do. 👍 This works perfectly. 🙇♂️ Thanks for suggestion.
…-selection-button-last-focus
…-selection-button-last-focus
youknowriad
approved these changes
Jan 18, 2022
|
Waiting for Accessibility team to check it. https://wordpress.slack.com/archives/C02RP4X03/p1642512823187900 I am hoping I can find at least one other reviewer just to make sure I haven't missed anything here as far as accessibility goes. |
tellthemachines
approved these changes
Jan 19, 2022
kevin940726
approved these changes
Jan 24, 2022
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
Whenever you are in Gutenberg Navigation Mode, I found a bug that effects how next focus works. More details in testing instructions below.
How has this been tested?
Tested in Firefox with NVDA screen reader.
Testing Instructions
Screenshots
Types of changes
Bug fix
Checklist:
*.native.jsfiles for terms that need renaming or removal).