Skip to content

Fix useFocusFirstElement in non-contenteditable Blocks #37934

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

Merged
alexstine merged 39 commits into from
Mar 15, 2022
Merged

Fix useFocusFirstElement in non-contenteditable Blocks #37934

Show file tree
Hide file tree
Changes from 34 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
160067a
Finally fix selector in test I think.
alexstine Jan 14, 2022
1251408
If not contenteditable Block, find next focussable element.
alexstine Jan 31, 2022
e735aa3
Try to make tests stable.
alexstine Jan 31, 2022
3c6311d
Try to fix tests.
alexstine Jan 31, 2022
f18270d
Update snapshot.
alexstine Jan 31, 2022
8f352e8
Fix focus getting stuck on child Block wrapper. Adjust to find only s…
alexstine Jan 31, 2022
b947e8c
Revert tests and start fresh.
alexstine Feb 1, 2022
6ee7747
More work on tests.
alexstine Feb 1, 2022
818115e
Add td to whitelisted elements. Check to see if first element has foc…
alexstine Feb 1, 2022
21e15dd
Revert some code.
alexstine Feb 2, 2022
e3f373b
Back to test fixing.
alexstine Feb 2, 2022
038d1b8
Try again to prevent focus in child Blocks. Tests should pass this ti…
alexstine Feb 2, 2022
be095b8
Update another snapshot.
alexstine Feb 3, 2022
fbffdf5
More work on E2E tests.
alexstine Feb 16, 2022
c421834
Cleanup code and try to fix more tests.
alexstine Feb 21, 2022
5007b54
Fix snapshot.
alexstine Feb 22, 2022
cc894c4
Maybe fix another snapshot.
alexstine Feb 22, 2022
643e470
Make sure returning early doesn't prevent false positive of still hav…
alexstine Feb 22, 2022
df7bf85
Update snapshots.
alexstine Feb 22, 2022
51d8e7a
Fix test snapshot.
tellthemachines Feb 23, 2022
316b9cc
Update tests with focus order changes.
tellthemachines Feb 28, 2022
8969d23
Make the implementation more stable. Fix the List View focus bug.
alexstine Mar 1, 2022
ce52e45
Use target everywhere. Add a check for Block Inserter trigger.
alexstine Mar 1, 2022
01e0210
Merge branch 'trunk' of github.com:wordpress/gutenberg into try/acces…
alexstine Mar 1, 2022
c0027c4
Use Tab to exit the toolbar so blocks could be navigated. Only seems …
alexstine Mar 1, 2022
41ffcec
Fix failing test and update snapshot.
tellthemachines Mar 4, 2022
cdea00f
Fix group block still places focus on Add block button. Add some E2E …
alexstine Mar 4, 2022
855bae1
Merge branch 'trunk' of github.com:wordpress/gutenberg into try/acces…
alexstine Mar 4, 2022
4ac00b0
Merge branch 'try/accessible-block-content-dialog' of github.com:word…
alexstine Mar 4, 2022
72a5933
Fix code comment.
alexstine Mar 4, 2022
065ba6f
Fix bugs.
alexstine Mar 5, 2022
53b7cd5
Merge branch 'trunk' of github.com:wordpress/gutenberg into try/acces…
alexstine Mar 5, 2022
c2a8ba9
Formatting fixes.
alexstine Mar 5, 2022
300c302
Update to use class name.
alexstine Mar 5, 2022
b66fbd2
Try again to fix my own test.
alexstine Mar 5, 2022
f8871ca
Merge branch 'trunk' of github.com:wordpress/gutenberg into try/acces…
alexstine Mar 9, 2022
17f1dd0
Merge branch 'trunk' of github.com:wordpress/gutenberg into try/acces…
alexstine Mar 14, 2022
3f4df14
Try again to rewrite toolbar roving tabindex now that E2E is working.
alexstine Mar 14, 2022
948f325
Better code commenting.
alexstine Mar 14, 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
29 changes: 29 additions & 0 deletions packages/block-editor/src/components/block-list/use-block-props/use-focus-first-element.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,16 @@ function useInitialPosition( clientId ) {
);
}

function isFormElement( element ) {
Copy link
Contributor Author

@alexstine alexstine Mar 1, 2022

Choose a reason for hiding this comment

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

I borrowed this function from writing-flow. Should probably throw this somewhere else in the future.

const { tagName } = element;
return (
tagName === 'INPUT' ||
tagName === 'BUTTON' ||
tagName === 'SELECT' ||
tagName === 'TEXTAREA'
);
}

/**
* Transitions focus to the block or inner tabbable when the block becomes
* selected and an initial position is set.
Expand Down Expand Up @@ -96,6 +106,25 @@ export function useFocusFirstElement( clientId ) {
return;
}

// Check to see if element is focussable before a generic caret insert.
if ( ! target.getAttribute( 'contenteditable' ) ) {
const focusElement = focus.tabbable.findNext( target );
// Ensure is not block inserter trigger, don't want to focus that in the event of the group block which doesn't contain any other focussable elements.
const skipBlockInserterTrigger = target.classList.contains(
Copy link
Contributor Author

@alexstine alexstine Mar 5, 2022

Choose a reason for hiding this comment

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

This has been something of a terrible pain. For some reason, hasAttribute/getAttribute throw errors on tests so I gave up and just used classList. I attempted to get it working with aria-label but for whatever reason, this is way easier in tests than it is in practice.

'block-editor-button-block-appender'
);
// Make sure focusElement is valid, form field, and in current ref.
if (
focusElement &&
isFormElement( focusElement ) &&
target.contains( focusElement ) &&
! skipBlockInserterTrigger
) {
focusElement.focus();
return;
}
}

setContentEditableWrapper( ref.current, false );
Copy link
Contributor Author

@alexstine alexstine Feb 16, 2022

Choose a reason for hiding this comment

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

@ellatrix I am not sure what this line is for right off hand, is it okay to have my code run before this or do I need to switch the order?


placeCaretAtHorizontalEdge( target, isReverse );
Expand Down
1 change: 1 addition & 0 deletions packages/block-editor/src/components/list-view/block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ function ListViewBlock( {
const selectEditorBlock = useCallback(
( event ) => {
selectBlock( event, clientId );
event.preventDefault();
Copy link
Contributor Author

@alexstine alexstine Mar 1, 2022

Choose a reason for hiding this comment

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

Had to add this because if focus was placed on a button, the button would get clicked causing an unprovoked action. E.g.

  1. Insert a Table block.
  2. Select it from the List View.
  3. The Create Table button gets clicked on block selection button click.
  4. Adding this stops the event after selection.

},
[ clientId, selectBlock ]
);
Expand Down
12 changes: 6 additions & 6 deletions packages/e2e-tests/specs/editor/various/__snapshots__/writing-flow.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,12 @@ exports[`Writing Flow should not prematurely multi-select 1`] = `
<!-- /wp:paragraph -->"
`;

exports[`Writing Flow should only consider the content as one tab stop 1`] = `
"<!-- wp:table -->
<figure class=\\"wp-block-table\\"><table><tbody><tr><td></td><td>2</td></tr><tr><td></td><td></td></tr></tbody></table></figure>
<!-- /wp:table -->"
`;

exports[`Writing Flow should preserve horizontal position when navigating vertically between blocks 1`] = `
"<!-- wp:paragraph -->
<p>abc</p>
Expand All @@ -297,9 +303,3 @@ exports[`Writing Flow should remember initial vertical position 1`] = `
<p><br>2</p>
<!-- /wp:paragraph -->"
`;

exports[`Writing Flow should only consider the content as one tab stop 1`] = `
"<!-- wp:table -->
<figure class=\\"wp-block-table\\"><table><tbody><tr><td></td><td>2</td></tr><tr><td></td><td></td></tr></tbody></table></figure>
<!-- /wp:table -->"
`;
1 change: 1 addition & 0 deletions packages/e2e-tests/specs/editor/various/block-deletion.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,7 @@ describe( 'deleting all blocks', () => {
// Add and remove a block.
await insertBlock( 'Image' );
await page.waitForSelector( 'figure[data-type="core/image"]' );
await page.keyboard.press( 'ArrowUp' );
Copy link
Contributor

@tellthemachines tellthemachines Feb 28, 2022

Choose a reason for hiding this comment

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

Note that Arrow Up to select the block wrapper won't work if 'Contain text cursor inside block' is enabled, so it will be much harder to delete a block immediately after insertion.

Copy link
Contributor Author

@alexstine alexstine Mar 1, 2022

Choose a reason for hiding this comment

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

@tellthemachines This didn't change though, right? If contains cursor option is selected, Up Arrow on latest trunk should not focus the wrapper either.

Copy link
Contributor

@tellthemachines tellthemachines Mar 1, 2022

Choose a reason for hiding this comment

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

Sure, but with these changes the wrapper won't be focused after insertion anymore. This is only relevant when you add a block, immediately realise it's the wrong block, and want to quickly delete it. It would be nice to preserve that ability, but we should be able to find a workaround for it.

Copy link
Contributor

@andrewserong andrewserong Mar 7, 2022

Choose a reason for hiding this comment

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

This is only relevant when you add a block, immediately realise it's the wrong block, and want to quickly delete it. It would be nice to preserve that ability, but we should be able to find a workaround for it.

I wonder if in a follow-up, we can look at in placeholder states that don't have an editable input field (e.g. the Columns placeholder state, but not the Twitter embed input field), and see if we can get backspace to still delete the block?

Copy link
Contributor

@tellthemachines tellthemachines Mar 15, 2022

Choose a reason for hiding this comment

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

That won't work for Table though, because the first focusable field is an input 😅
But we should be able to work something out. Definitely as a follow-up.

await page.keyboard.press( 'Backspace' );

// Verify there is no selected block.
Expand Down
23 changes: 23 additions & 0 deletions packages/e2e-tests/specs/editor/various/keyboard-navigable-blocks.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,4 +207,27 @@ describe( 'Order of block keyboard navigation', () => {
await page.keyboard.press( 'ArrowRight' );
await expect( await getActiveLabel() ).toBe( 'Move up' );
} );

it( 'allows the first element within a block to receive focus', async () => {
// Insert a image block.
await insertBlock( 'Image' );

// Make sure the upload button has focus.
const uploadButton = await page.waitForXPath(
'//button[contains( text(), "Upload" ) ]'
);
await expect( uploadButton ).toHaveFocus();

// Try to focus the image block wrapper.
await page.keyboard.press( 'ArrowUp' );
await expect( await getActiveLabel() ).toBe( 'Block: Image' );
} );

it( 'allows the block wrapper to gain focus for a group block instead of the first element', async () => {
// Insert a group block.
await insertBlock( 'Group' );

// If active label matches, that means focus did not change from group block wrapper.
await expect( await getActiveLabel() ).toBe( 'Block: Group' );
} );
} );
7 changes: 4 additions & 3 deletions packages/e2e-tests/specs/editor/various/list-view.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,12 +86,13 @@ describe( 'List view', () => {
// Select the image block in the canvas.
await page.keyboard.press( 'Enter' );

const canvasImageBlock = await page.waitForSelector(
'figure[aria-label="Block: Image"]'
const uploadButton = await page.waitForXPath(
'//button[contains( text(), "Upload" ) ]'
);
expect( canvasImageBlock ).toHaveFocus();
expect( uploadButton ).toHaveFocus();

// Delete the image block in the canvas.
await page.keyboard.press( 'ArrowUp' );
await page.keyboard.press( 'Backspace' );

// List view should have two rows.
Expand Down
1 change: 0 additions & 1 deletion packages/e2e-tests/specs/editor/various/multi-block-selection.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -655,7 +655,6 @@ describe( 'Multi-block selection', () => {
await page.keyboard.press( 'Enter' );
// Select two columns.
await page.keyboard.press( 'ArrowRight' );
await page.keyboard.press( 'ArrowRight' );
await page.keyboard.press( 'Enter' );
// Navigate to appender.
await page.keyboard.press( 'ArrowRight' );
Expand Down
25 changes: 10 additions & 15 deletions packages/e2e-tests/specs/editor/various/toolbar-roving-tabindex.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,18 +20,16 @@ async function expectLabelToHaveFocus( label ) {
).toBe( label );
}

async function testBlockToolbarKeyboardNavigation(
currentBlockLabel,
currentBlockTitle
) {
async function testBlockToolbarKeyboardNavigation( currentBlockTitle ) {
await focusBlockToolbar();
await expectLabelToHaveFocus( currentBlockTitle );
await page.keyboard.press( 'ArrowRight' );
await expectLabelToHaveFocus( 'Move up' );
await page.keyboard.press( 'Tab' );
await expectLabelToHaveFocus( currentBlockLabel );
Copy link
Contributor

@tellthemachines tellthemachines Feb 28, 2022

Choose a reason for hiding this comment

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

Is it a problem that we're now unable to focus the block wrapper on a new block? Since it has a "document" role I'd expect it to still be focusable.

The other thing to consider is we're introducing a change in how focus behaves only for newly-added blocks, so supposing we add a block, then move focus to another block or another part of the page, and then move back again, the behaviour will be different. This might create confusion, e.g. "why am I now able to focus the block wrapper when I wasn't before?"

Copy link
Contributor Author

@alexstine alexstine Mar 1, 2022

Choose a reason for hiding this comment

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

@tellthemachines In my testing, the block wrapper can still gain focus if you use Up Arrow from an element. Not sure why it doesn't work in some tests.

Copy link
Member

@ellatrix ellatrix Mar 10, 2022

Choose a reason for hiding this comment

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

Why do we have to remove this check? Can we still test that moving focus from the toolbar to the content is still possible?

Copy link
Contributor Author

@alexstine alexstine Mar 10, 2022

Choose a reason for hiding this comment

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

@ellatrix I changed this because landing on the first element inside the wrapper is no longer the aria-label of the block wrapper so the test failed. I tried to still ensure moving focus to toolbar and back worked but if you press Tab from toolbar, wrapper is skipped.

Copy link
Contributor Author

@alexstine alexstine Mar 10, 2022

Choose a reason for hiding this comment

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

@ellatrix Rewrote the test to be less confusing. Hopefully better structure and comment helps. This test is lacking on comments as it is. Something to improve in the future.

Copy link
Contributor Author

@alexstine alexstine Mar 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Another update. I could never make the tests pass. Is it fine to try to improve the tests in a follow-up PR? Essentially I need to implement some type of function for content editable blocks vs. non-content editable blocks. Content editable blocks can have the aria-label parsed directly but non-content editable blocks cannot since Up Arrow is now required to focus the block wrapper. In my opinion, these tests are still good since it checks to see if the toolbar is still accessible via Shift+Tab and Arrow keys.

This PR is getting far too big for me to keep good track of as well so I'd like to split this off soon.

Copy link
Contributor Author

@alexstine alexstine Mar 11, 2022

Choose a reason for hiding this comment

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

Oh, I think my E2E tests were fine but trunk is currently failing. 👎 I wish I would have checked that sooner. I wondered why they were passing locally... 😕

await pressKeyWithModifier( 'shift', 'Tab' );
await expectLabelToHaveFocus( 'Move up' );
await page.keyboard.press( 'ArrowLeft' );
await expectLabelToHaveFocus( currentBlockTitle );
}

async function wrapCurrentBlockWithGroup( currentBlockTitle ) {
Expand Down Expand Up @@ -64,53 +62,50 @@ describe( 'Toolbar roving tabindex', () => {
it( 'ensures paragraph block toolbar uses roving tabindex', async () => {
await insertBlock( 'Paragraph' );
await page.keyboard.type( 'Paragraph' );
await testBlockToolbarKeyboardNavigation(
'Paragraph block',
'Paragraph'
);
await testBlockToolbarKeyboardNavigation( 'Paragraph' );
await wrapCurrentBlockWithGroup( 'Paragraph' );
await testGroupKeyboardNavigation( 'Paragraph block', 'Paragraph' );
} );

it( 'ensures heading block toolbar uses roving tabindex', async () => {
await insertBlock( 'Heading' );
await page.keyboard.type( 'Heading' );
await testBlockToolbarKeyboardNavigation( 'Block: Heading', 'Heading' );
await testBlockToolbarKeyboardNavigation( 'Heading' );
await wrapCurrentBlockWithGroup( 'Heading' );
await testGroupKeyboardNavigation( 'Block: Heading', 'Heading' );
} );

it( 'ensures list block toolbar uses roving tabindex', async () => {
await insertBlock( 'List' );
await page.keyboard.type( 'List' );
await testBlockToolbarKeyboardNavigation( 'Block: List', 'List' );
await testBlockToolbarKeyboardNavigation( 'List' );
await wrapCurrentBlockWithGroup( 'List' );
await testGroupKeyboardNavigation( 'Block: List', 'List' );
} );

it( 'ensures image block toolbar uses roving tabindex', async () => {
await insertBlock( 'Image' );
await testBlockToolbarKeyboardNavigation( 'Block: Image', 'Image' );
await testBlockToolbarKeyboardNavigation( 'Image' );
await wrapCurrentBlockWithGroup( 'Image' );
await testGroupKeyboardNavigation( 'Block: Image', 'Image' );
} );

it( 'ensures table block toolbar uses roving tabindex', async () => {
await insertBlock( 'Table' );
await testBlockToolbarKeyboardNavigation( 'Block: Table', 'Table' );
await testBlockToolbarKeyboardNavigation( 'Table' );
// Move focus to the first toolbar item.
await page.keyboard.press( 'Home' );
Copy link
Contributor

@tellthemachines tellthemachines Feb 28, 2022

Choose a reason for hiding this comment

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

Does pressing the 'Home' key no longer take us to the beginning of the toolbar? I'm not able to test this PR on Windows as gutenberg.run doesn't seem to be working.

Copy link
Contributor Author

@alexstine alexstine Feb 28, 2022

Choose a reason for hiding this comment

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

@tellthemachines It did in my testing. That part should not be effected. The only things I changed around this were taking in to account not reading the aria-label from the block wrapper as focus was skipped.

await expectLabelToHaveFocus( 'Table' );
await page.click( '.blocks-table__placeholder-button' );
await page.keyboard.press( 'Tab' );
await testBlockToolbarKeyboardNavigation( 'Body cell text', 'Table' );
await expectLabelToHaveFocus( 'Body cell text' );
await wrapCurrentBlockWithGroup( 'Table' );
await testGroupKeyboardNavigation( 'Block: Table', 'Table' );
} );

it( 'ensures custom html block toolbar uses roving tabindex', async () => {
await insertBlock( 'Custom HTML' );
await testBlockToolbarKeyboardNavigation( 'HTML', 'Custom HTML' );
await testBlockToolbarKeyboardNavigation( 'Custom HTML' );
await wrapCurrentBlockWithGroup( 'Custom HTML' );
await testGroupKeyboardNavigation(
'Block: Custom HTML',
Expand Down
15 changes: 10 additions & 5 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 @@ -26,8 +26,8 @@ const addParagraphsAndColumnsDemo = async () => {
`//*[contains(@class, "components-autocomplete__result") and contains(@class, "is-selected") and contains(text(), 'Columns')]`
);
await page.keyboard.press( 'Enter' );
await page.click( ':focus [aria-label="Two columns; equal split"]' );
await page.click( ':focus .block-editor-button-block-appender' );
await page.click( 'button[aria-label="Two columns; equal split"]' );
await page.click( '.block-editor-button-block-appender' );
await page.waitForSelector( '.block-editor-inserter__search input:focus' );
await page.keyboard.type( 'Paragraph' );
await pressKeyTimes( 'Tab', 2 ); // Tab to paragraph result.
Expand Down Expand Up @@ -628,9 +628,12 @@ describe( 'Writing Flow', () => {
`//button[contains(@class,'components-dropdown-menu__menu-item')]//span[contains(text(), 'Wide width')]`
);
await wideButton.click();
// Focus the block content
await page.keyboard.press( 'Tab' );

// Select the previous block.
await page.keyboard.press( 'ArrowUp' );
await page.keyboard.press( 'ArrowUp' );

// Confirm correct setup.
expect( await getEditedPostContent() ).toMatchSnapshot();
Expand All @@ -650,13 +653,17 @@ describe( 'Writing Flow', () => {
const inserter = await page.$(
'.block-editor-block-list__insertion-point'
);
// Find the space between the inserter and the image block.
const inserterRect = await inserter.boundingBox();
const lowerInserterY = inserterRect.y + ( 2 * inserterRect.height ) / 3;

// Clicking that in-between space should select the image block.
await page.mouse.click( x, lowerInserterY );

const type = await page.evaluate( () =>
document.activeElement.getAttribute( 'data-type' )
document.activeElement
.closest( '[data-block]' )
.getAttribute( 'data-type' )
);

expect( type ).toBe( 'core/image' );
Expand All @@ -666,8 +673,6 @@ describe( 'Writing Flow', () => {
await page.keyboard.press( 'Enter' );
await page.keyboard.type( '/table' );
await page.keyboard.press( 'Enter' );
// Move into the placeholder UI.
await page.keyboard.press( 'ArrowDown' );
// Tab to the "Create table" button.
await page.keyboard.press( 'Tab' );
await page.keyboard.press( 'Tab' );
Expand Down