Skip to content
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.

Sign up for GitHub

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

Table block: Fix focus loss after Create Table button is selected #40399

Merged
merged 9 commits into from
May 11, 2022
Merged

Table block: Fix focus loss after Create Table button is selected #40399

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e5c36dc
Fix focus after table creation.
alexstine Apr 16, 2022
3fb4f64
Fix E2E test failures. Add new E2E test.
alexstine Apr 16, 2022
b09ea5a
isFormElement now supports option for contentEditable attribute.
alexstine Apr 16, 2022
e4455e0
Fix E2E test.
alexstine Apr 16, 2022
b2af579
Allow Tab key to work for writing flow contentEditable elements. Allo…
alexstine Apr 16, 2022
5ccae83
Update API docs/function comment with better structure.
alexstine Apr 16, 2022
62e0e69
Only find contentEditable fields within a block for writing flow tab …
alexstine Apr 16, 2022
3430672
Merge branch 'trunk' of github.com:wordpress/gutenberg into improve/t…
alexstine Apr 20, 2022
de235a9
Revert changes to dom/block-editor packages for now. Will split these…
alexstine Apr 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions packages/block-library/src/table/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import classnames from 'classnames';
/**
* WordPress dependencies
*/
import { useEffect, useState } from '@wordpress/element';
import { useEffect, useRef, useState } from '@wordpress/element';
import {
InspectorControls,
BlockControls,
Expand Down Expand Up @@ -105,6 +105,9 @@ function TableEdit( {
const colorProps = useColorProps( attributes );
const borderProps = useBorderProps( attributes );

const tableRef = useRef();
const [ hasTableCreated, setHasTableCreated ] = useState( false );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/**
* Updates the initial column count used for table creation.
*
Expand Down Expand Up @@ -137,6 +140,7 @@ function TableEdit( {
columnCount: parseInt( initialColumnCount, 10 ) || 2,
} )
);
setHasTableCreated( true );
}

/**
Expand Down Expand Up @@ -341,6 +345,15 @@ function TableEdit( {
}
}, [ isSelected ] );

useEffect( () => {
if ( hasTableCreated ) {
tableRef?.current
?.querySelector( 'td[contentEditable="true"]' )
?.focus();
setHasTableCreated( false );
}
}, [ hasTableCreated ] );

const sections = [ 'head', 'body', 'foot' ].filter(
( name ) => ! isEmptyTableSection( attributes[ name ] )
);
Expand Down Expand Up @@ -426,7 +439,7 @@ function TableEdit( {
const isEmpty = ! sections.length;

return (
<figure { ...useBlockProps() }>
<figure { ...useBlockProps( { ref: tableRef } ) }>
{ ! isEmpty && (
<>
<BlockControls group="block">
Expand Down
15 changes: 14 additions & 1 deletion packages/e2e-tests/specs/editor/blocks/table.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,6 @@ describe( 'Table', () => {
// Create the table.
await clickButton( createButtonLabel );

await page.keyboard.press( 'Tab' );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This issue is fixed. 👍

await page.keyboard.type( '1' );
await page.keyboard.press( 'ArrowDown' );
await page.keyboard.type( '2' );
Expand All @@ -280,4 +279,18 @@ describe( 'Table', () => {

expect( await getEditedPostContent() ).toMatchSnapshot();
} );

it( 'should not have focus loss after creation', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this test to ensure it doesn't break in the future.

// Insert table block.
await insertBlock( 'Table' );

// Create the table.
await clickButton( createButtonLabel );

// Focus should be in first td.
const isInTd = await page.waitForSelector(
'td[contentEditable="true"]'
);
await expect( isInTd ).toHaveFocus();
} );
} );
16 changes: 0 additions & 16 deletions packages/e2e-tests/specs/editor/various/writing-flow.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was fixed.

await page.keyboard.press( 'Tab' );
// 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.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

await page.keyboard.press( 'Tab' );
expect(
await page.evaluate( () =>
document.activeElement.getAttribute( 'aria-label' )
)
).toBe( 'Post' );
await pressKeyWithModifier( 'shift', 'Tab' );
await pressKeyWithModifier( 'shift', 'Tab' );
expect(
await page.evaluate( () =>
document.activeElement.getAttribute( 'aria-label' )
)
).toBe( 'Table' );
} );

it( 'Should unselect all blocks when hitting double escape', async () => {
Expand Down