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

Enter editing mode via Enter or Spacebar #58795

Merged
merged 12 commits into from
Feb 15, 2024
Merged

Enter editing mode via Enter or Spacebar #58795

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
030b6ea
Enter editing mode via Enter or Spacebar
jeryj Feb 7, 2024
75a8d55
Document tests. Solidify the iframe check. Remove flaky shift+tab fro…
jeryj Feb 13, 2024
4316b4a
Remove command palette focus tests
jeryj Feb 13, 2024
8cc9253
Make tabbing long distances less flakey
jeryj Feb 13, 2024
08c81eb
Extract tabTo logic to util
jeryj Feb 13, 2024
d8e988e
Don't return anything from tabToX utils
jeryj Feb 13, 2024
ad788fc
Remove extra forward slash
jeryj Feb 14, 2024
39b0a10
Find editor canvas by button role
jeryj Feb 14, 2024
0ef75fa
Remove tabToNode. Use iframe button role.
jeryj Feb 14, 2024
e9a2861
Use times param directly instead of as object
jeryj Feb 15, 2024
7441ad2
Rely on default times value
jeryj Feb 15, 2024
fea790d
Rely on default times value
jeryj Feb 15, 2024
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
5 changes: 4 additions & 1 deletion packages/block-editor/src/components/iframe/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,13 +273,16 @@ function Iframe( {
src={ src }
title={ __( 'Editor canvas' ) }
onKeyDown={ ( event ) => {
if ( props.onKeyDown ) {
props.onKeyDown( event );
}
// If the event originates from inside the iframe, it means
// it bubbled through the portal, but only with React
// events. We need to to bubble native events as well,
// though by doing so we also trigger another React event,
// so we need to stop the propagation of this event to avoid
// duplication.
if (
else if (
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't make any sense. When you have multiple keydown handlers, all of them should execute

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only time the if statement above it runs is when a keydown listener is passed to the iframe, which is only in the view mode of the site editor. The shortcuts shouldn't be enabled at that point since we're not within the editor.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, but that's not clear from reading this. Also there might be other iframe instances that pass onKeydown and do want shortcuts disabled. Using onKeydown to disable/enable shortcuts seems a bit strange, imo that should be done through a separate prop. Maybe reuse isPreviewMode?

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'm not sure how all of that should be structured. The only thing important to me in this PR was fixing a bug where you couldn't use the keyboard to enter into Edit mode from View mode. This onKeydown was not allowed to run.

Very open to fixing this another way. I wasn't sold on this solution.

Copy link
Member

Choose a reason for hiding this comment

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

No, this is fine, all I'm saying is that the else shouldn't have been added?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh - That's fine with me: #59474

event.currentTarget.ownerDocument !==
event.target.ownerDocument
) {
Expand Down
80 changes: 80 additions & 0 deletions test/e2e/specs/site-editor/navigation.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
/**
* WordPress dependencies
*/
const { test, expect } = require( '@wordpress/e2e-test-utils-playwright' );

test.describe( 'Site editor navigation', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test feels quite big and I worry that it's going to need a lot of work to maintain it as the editor continues to evolve. I think it would be better if we could find a way to just test the spacebar press on its own.

test.beforeAll( async ( { requestUtils } ) => {
await requestUtils.activateTheme( 'emptytheme' );
} );

test.afterAll( async ( { requestUtils } ) => {
await requestUtils.activateTheme( 'twentytwentyone' );
} );

test( 'Can use keyboard to navigate the site editor', async ( {
Copy link
Contributor

Choose a reason for hiding this comment

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

'Can use keyboard to navigate the site editor sounds like the whole battery of tests instead of a single test - and looking at the many expectations it's kinda true.

admin,
page,
pageUtils,
} ) => {
await admin.visitSiteEditor();

// Navigate to the iframe
await pageUtils.pressKeys( 'Tab', { times: 3 } );
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels quite fragile. Is there a way to focus the iframe directly without relying on the number of tab presses?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is correct because we should be aware we introduced a new tab stop?

// Open the command palette via button press
await expect( page.getByLabel( 'Open command palette' ) ).toBeFocused();
await pageUtils.pressKeys( 'Enter' );
await expect(
page.getByPlaceholder( 'Search for commands' )
).toBeFocused();
// Return focus to the button
await pageUtils.pressKeys( 'Escape' );
await expect( page.getByLabel( 'Open command palette' ) ).toBeFocused();
// Open it again with Command + K
await pageUtils.pressKeys( 'primary+k' );
await expect(
page.getByPlaceholder( 'Search for commands' )
).toBeFocused();
await pageUtils.pressKeys( 'Escape' );
await expect( page.getByLabel( 'Open command palette' ) ).toBeFocused();
// Go to the Pages button
await pageUtils.pressKeys( 'Tab', { times: 4 } );
await expect(
page.getByRole( 'button', { name: 'Pages' } )
).toBeFocused();
await pageUtils.pressKeys( 'Enter' );
// We should be in the Pages sidebar
await expect(
page.getByRole( 'button', { name: 'Back', exact: true } )
).toBeFocused();
await pageUtils.pressKeys( 'Enter' );
// Go back to the main navigation
await expect(
page.getByRole( 'button', { name: 'Pages' } )
).toBeFocused();
await pageUtils.pressKeys( 'Tab', { times: 6 } );
// Getting the actual iframe as a cleaner locator was suprisingly tricky,
// so we're using a css selector with .is-focused which should be present when the iframe has focus.
await expect(
page.locator( 'iframe[name="editor-canvas"].is-focused' )
).toBeFocused();
jeryj marked this conversation as resolved.
Show resolved Hide resolved

// Enter into editing mode
await pageUtils.pressKeys( 'Enter' );

// We should now be in editing mode
await pageUtils.pressKeys( 'Shift+Tab', { times: 9 } );

// Open the sidebar again
await expect(
page.getByRole( 'button', {
name: 'Open Navigation',
exact: true,
} )
).toBeFocused();
await pageUtils.pressKeys( 'Enter' );
await expect(
page.getByLabel( 'Go to the Dashboard' ).first()
).toBeFocused();
} );
} );
Loading