Table block: Fix focus loss after Create Table button is selected #40399
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] Enhancement
alexstine
requested review from
talldan,
ellatrix,
tellthemachines and
aaronrobertshaw
|
Size Change: +92 B (0%) Total Size: 1.23 MB
ℹ️ View Unchanged
|
…w contentEditable elements for focus first.
alexstine
added
[Package] DOM
alexstine
commented
Apr 16, 2022
| const { tagName } = element; | ||
| const checkForInputTextarea = isInputOrTextArea( element ); | ||
| let checkContentEditable = false; |
There was a problem hiding this comment.
I added this because some blocks such as the table block have contentEditable instead of a real form element.
| @@ -269,7 +269,6 @@ describe( 'Table', () => { | |||
| // Create the table. | |||
| await clickButton( createButtonLabel ); | |||
|
|
|||
| await page.keyboard.press( 'Tab' ); | |||
There was a problem hiding this comment.
This issue is fixed. 👍
| @@ -280,4 +279,18 @@ describe( 'Table', () => { | |||
|
|
|||
| expect( await getEditedPostContent() ).toMatchSnapshot(); | |||
| } ); | |||
|
|
|||
| it( 'should not have focus loss after creation', async () => { | |||
There was a problem hiding this comment.
Added this test to ensure it doesn't break in the future.
| @@ -678,27 +678,11 @@ describe( 'Writing Flow', () => { | |||
| await page.keyboard.press( 'Tab' ); | |||
| // Create the table. | |||
| await page.keyboard.press( 'Space' ); | |||
| // Return focus after focus loss. This should be fixed. | |||
| // Navigate to the second cell. | ||
| await page.keyboard.press( 'ArrowRight' ); | ||
| await page.keyboard.type( '2' ); | ||
| // Confirm correct setup. | ||
| expect( await getEditedPostContent() ).toMatchSnapshot(); | ||
| // The content should only have one tab stop. |
There was a problem hiding this comment.
This is not the case. Each Tab key press will land in a cell so removed this. Let me know if I should do anything differently.
| @@ -105,6 +105,9 @@ function TableEdit( { | |||
| const colorProps = useColorProps( attributes ); | |||
| const borderProps = useBorderProps( attributes ); | |||
|
|
|||
| const tableRef = useRef(); | |||
| const [ hasTableCreated, setHasTableCreated ] = useState( false ); | |||
There was a problem hiding this comment.
I am using the state to detect when table creation happens and then it fires the effect later. From my understanding, effects cannot listen for function execution but they can listen for states. If the state is set to true on creation, the effect will execute which will place focus in the table cell. After, it will set the state to false so it won't fire anymore. I set the ref as a property on the block so I had good targeting on my querySelector since that should be avoided.
alexstine
added
the
[Feature] Writing Flow
talldan
reviewed
Apr 20, 2022
| export default function isFormElement( element ) { | ||
| export default function isFormElement( | ||
| element, | ||
| includeContentEditable = false |
There was a problem hiding this comment.
I wonder if instead of adding an extra parameter, another option would be adding an isContentEditable function to the dom package and using that instead.
I'm never a huge fan of boolean parameters, as the calling code is very unclear. For example isFormElement( element, true ), I have to look up what the true means.
An alternative is to add an options object, so it becomes isFormElement( element, { includeContentEditable: true } );, which is more verbose, but means readers don't have to look up documentation.
But I also think isFormElement( element ) || isContentEditable( element ) is very readable and the separate function might be more useful in a wider range of situations.
PS: I think this function is unusual anyway. It doesn't return true for a <form> element, which contradicts the name 😆
There was a problem hiding this comment.
Removed this for now.
| @@ -115,7 +115,7 @@ export default function useTabNav() { | |||
| // these are not rendered in the content and perhaps in the | |||
| // future they can be rendered in an iframe or shadow DOM. | |||
| if ( | |||
| ( isFormElement( event.target ) || | |||
| ( isFormElement( event.target, true ) || | |||
| event.target.getAttribute( 'data-block' ) === | |||
| getSelectedBlockClientId() ) && | |||
| isFormElement( focus.tabbable[ direction ]( event.target ) ) | |||
There was a problem hiding this comment.
Should this call to isFormElement also check for contenteditables?
It looks like it says 'if the current element is a form element, content editable or a block boundary, and the next element is a form element, don't do anything'. But I'm not sure what doing nothing achieves.
There was a problem hiding this comment.
No, it should not. Adding true here will find another block on next Tab key press vs. jumping to the sidebar.
There was a problem hiding this comment.
@talldan I could not make this work again when I restarted my dev environment so I removed the changes. I'll try again in another PR. This whole writing flow feature still needs some work for screen reader users.
…able-block-accessibility
… in to another PR.
alexstine
added
[Type] Bug
|
Updated the PR description to reflect latest changes. |
talldan
approved these changes
May 11, 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.
What?
The PR fixes focus on table creation.
Why?
When focus loss occurs after table creation, users may be confused.
How?
After the table creation function executes, focus is placed in the table.
Testing Instructions
Screenshots or screencast