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.

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
5 changes: 4 additions & 1 deletion packages/block-editor/src/components/iframe/index.js
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
119 changes: 119 additions & 0 deletions test/e2e/specs/site-editor/navigation.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
/**
* WordPress dependencies
*/
const { test, expect } = require( '@wordpress/e2e-test-utils-playwright' );

test.use( {
editorNavigationUtils: async ( { page, pageUtils }, use ) => {
await use( new EditorNavigationUtils( { page, pageUtils } ) );
},
} );

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,
editorNavigationUtils,
page,
pageUtils,
} ) => {
await admin.visitSiteEditor();

// Test: Can navigate to a sidebar item and into its subnavigation frame without losing focus
// Go to the Pages button

await editorNavigationUtils.tabToLabel( 'Pages' );

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();

// Test: Can navigate into the iframe using the keyboard
await editorNavigationUtils.tabToLabel( 'Editor Canvas' );
const editorCanvasButton = page.getByRole( 'button', {
name: 'Editor Canvas',
} );
await expect( editorCanvasButton ).toBeFocused();
// Enter into the site editor frame
await pageUtils.pressKeys( 'Enter' );
// Focus should be on the iframe without the button role
await expect(
page.locator( 'iframe[name="editor-canvas"]' )
).toBeFocused();
// The button role should have been removed from the iframe.
await expect( editorCanvasButton ).toBeHidden();

// Test to make sure a Tab keypress works as expected.
// As of this writing, we are in select mode and a tab
// keypress will reveal the header template select mode
// button. This test is not documenting that we _want_
// that action, but checking that we are within the site
// editor and keypresses work as intened.
await pageUtils.pressKeys( 'Tab' );
await expect(
page.getByRole( 'button', {
name: 'Template Part Block. Row 1. header',
} )
).toBeFocused();

// Test: We can go back to the main navigation from the editor frame
// Move to the document toolbar
await pageUtils.pressKeys( 'alt+F10' );
// Go to the open navigation button
await pageUtils.pressKeys( 'shift+Tab' );

// 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();
// We should have our editor canvas button back
await expect( editorCanvasButton ).toBeVisible();
} );
} );

class EditorNavigationUtils {
constructor( { page, pageUtils } ) {
this.page = page;
this.pageUtils = pageUtils;
}

async tabToLabel( label, times = 10 ) {
for ( let i = 0; i < times; i++ ) {
await this.pageUtils.pressKeys( 'Tab' );
const activeLabel = await this.page.evaluate( () => {
return (
document.activeElement.getAttribute( 'aria-label' ) ||
document.activeElement.textContent
);
} );
if ( activeLabel === label ) {
return;
}
}
}
}
Loading