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

Testing: Add an e2e test to check the interactions in write mode #65819

Merged
merged 3 commits into from
Oct 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions packages/block-editor/src/components/tool-selector/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ function ToolSelector( props, ref ) {
</>
),
info: __( 'Focus on content.' ),
'aria-label': __( 'Write' ),
},
{
value: 'edit',
Expand All @@ -88,6 +89,7 @@ function ToolSelector( props, ref ) {
</>
),
info: __( 'Edit layout and styles.' ),
'aria-label': __( 'Design' ),
},
] }
/>
Expand Down
3 changes: 3 additions & 0 deletions packages/e2e-test-utils-playwright/src/editor/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import { saveSiteEditorEntities } from './site-editor';
import { setIsFixedToolbar } from './set-is-fixed-toolbar';
import { switchToLegacyCanvas } from './switch-to-legacy-canvas';
import { transformBlockTo } from './transform-block-to';
import { switchEditorTool } from './switch-editor-tool';

type EditorConstructorProps = {
page: Page;
Expand Down Expand Up @@ -84,6 +85,8 @@ export class Editor {
/** @borrows setIsFixedToolbar as this.setIsFixedToolbar */
setIsFixedToolbar: typeof setIsFixedToolbar =
setIsFixedToolbar.bind( this );
/** @borrows switchEditorTool as this.switchEditorTool */
switchEditorTool: typeof switchEditorTool = switchEditorTool.bind( this );
/** @borrows switchToLegacyCanvas as this.switchToLegacyCanvas */
switchToLegacyCanvas: typeof switchToLegacyCanvas =
switchToLegacyCanvas.bind( this );
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/**
* Internal dependencies
*/
import type { Editor } from './index';

/**
* Switch the editor tool being used.
*
* @param this
* @param label The text string of the button label.
*/
export async function switchEditorTool( this: Editor, label: string ) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Prefer using unions of string literals in types since we only have two options for now. Something like 'Design' | 'Write'.

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 hesitant, someone could decide to test another language.

Copy link
Member

Choose a reason for hiding this comment

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

They're only types so I guess they can always bypass that. We can also add | string to the end to allow arbitrary strings.

const toolsToolbar = this.page.getByRole( 'toolbar', {
name: 'Document tools',
} );
await toolsToolbar
.getByRole( 'button', {
name: 'Tools',
} )
.click();
const menu = this.page.getByRole( 'menu', {
name: 'Tools',
} );
await menu
.getByRole( 'menuitemradio', {
name: label,
} )
.click();
await toolsToolbar
.getByRole( 'button', {
name: 'Tools',
} )
.click();
}
116 changes: 116 additions & 0 deletions test/e2e/specs/editor/various/write-design-mode.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
/**
* WordPress dependencies
*/
const { test, expect } = require( '@wordpress/e2e-test-utils-playwright' );

test.describe( 'Write/Design mode', () => {
test.beforeEach( async ( { admin, editor } ) => {
await admin.createNewPost();
await expect(
editor.canvas.getByRole( 'textbox', { name: 'Add title' } )
).toBeFocused();
} );

test.afterAll( async ( { requestUtils } ) => {
await requestUtils.deleteAllPosts();
} );

test( 'Should prevent selecting intermediary blocks', async ( {
editor,
page,
} ) => {
// Insert a section with a nested block and an editable block.
await editor.insertBlock( {
name: 'core/group',
attributes: {
style: {
spacing: {
padding: '20px',
},
color: {
background: 'darkgray',
},
},
},
innerBlocks: [
{
name: 'core/group',
attributes: {
style: {
spacing: {
padding: '20px',
},
color: {
background: 'lightgray',
},
},
},
innerBlocks: [
{
name: 'core/paragraph',
attributes: {
content: 'Something',
},
},
],
},
],
} );

// Switch to write mode.
await editor.switchEditorTool( 'Write' );

const sectionBlock = editor.canvas
.getByRole( 'document', {
name: 'Block: Group',
} )
.nth( 0 );
const sectionClientId = await sectionBlock.getAttribute( 'data-block' );
const nestedGroupBlock = sectionBlock.getByRole( 'document', {
name: 'Block: Group',
} );
const paragraph = nestedGroupBlock.getByRole( 'document', {
name: 'Block: Paragraph',
} );
const paragraphClientId = await paragraph.getAttribute( 'data-block' );

// We should not be able to select the intermediary group block.
// if we try to click on it (the padding area)
// The selection should land on the top level block.
const nestedGroupPosition = await nestedGroupBlock.boundingBox();
await page.mouse.click(
nestedGroupPosition.x + 5,
nestedGroupPosition.y + 5
);

const getSelectedBlock = async () =>
await page.evaluate( () =>
window.wp.data
.select( 'core/block-editor' )
.getSelectedBlockClientId()
);

expect( await getSelectedBlock() ).toEqual( sectionClientId );

// We should be able to select the paragraph block and write in it.
await paragraph.click();
await page.keyboard.type( ' something' );
expect( await getSelectedBlock() ).toEqual( paragraphClientId );
await expect( paragraph ).toHaveText( 'Something something' );

// Check that the inspector still shows the group block with the content panel.
await editor.openDocumentSettingsSidebar();
const editorSettings = page.getByRole( 'region', {
name: 'Editor settings',
} );
await expect(
// Ideally we should be using CSS selectors
Copy link
Member

Choose a reason for hiding this comment

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

"not" be using CSS selectors? 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

haha :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's fixed in trunk.

// but in this case there's no easy role/label
// to retrieve the "selected block title"
editorSettings.locator( '.block-editor-block-card__title' )
Copy link
Member

Choose a reason for hiding this comment

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

Using class name selectors is not recommended. Could we instead select it by getByRole and optionally with a level field here?

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 know but for me, this is not easy to pinpoint and avoid random failures when we add headings... I prefer the selector here.

Copy link
Member

Choose a reason for hiding this comment

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

I think there's already a heading in place? What do you mean by pinpointing random failures? I'd appreciate if we could follow the established best practices whenever possible and add comments if we could not. Thanks! 🙏

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 h2 is not enough to say that I'm targeting the "title" of the inspector panel. There could be more h2 in the inspector panel, in fact there are already. (content).

We could theoretically say h2 > eq( 0 ) but that's not very stable, any random design change can break that. (moving titles around).

So for me, in this case, I prefer a non ambiguous assertion rather than a generic one.

I agree on following best practices, but we need to understand what's important and why they exist.

I'm happy to update If there's a less ambiguous approach using that doesn't use selectors.

Copy link
Member

Choose a reason for hiding this comment

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

A common workaround practice throughout the codebase is using expect( getByRole('heading', {name: 'Group', level: 2}) ).toBeVisible().

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd echo Riad about until fixing the a11y issue, we should stick with that classname for now and unblock this PR. Let's create an issue for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

No need to block this PR. But we should raise a followup Issue as suggested with the appropriate labels to get Accessibility feedback.

Copy link
Member

Choose a reason for hiding this comment

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

IMO, getByRole is always preferred, in cases when it doesn't apply, we should still fallback to alternatives like getByTestId than class name selectors. I don't understand the argument of using class name selectors being more "solid". TBH, this has happened too many times that this is becoming frustrating to me. Could we at least add a comment above indicating that this is an edge case so that people won't accidentally follow the practice? I don't think it should block this PR, but I think this type of issue is still important, especially given that it just flagged a potential accessibility concern here by trying to avoid the class name selectors.

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'll add a comment. I'm not sure we used test ids before.

Copy link
Contributor

@getdave getdave Oct 7, 2024

Choose a reason for hiding this comment

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

Thanks for the comment.

Nit: the wording isn't quite accurate and should read (emphasis added for clarity):

// Ideally we should **_NOT_** be using CSS selectors

).toHaveText( 'Group' );
await expect(
editorSettings.getByRole( 'button', { name: 'Content' } )
).toBeVisible();
} );
} );
Loading