Refactored e2e keyboard navigation util functions / tests #16707
Conversation
…s (Issue WordPress#12392) using bug resolved in WordPress#11773 as the basis for the steps
…ThroughBlockMoverControl` and `tabThroughBlockToolbar` to the parent scope. Using pressKeyWithModifier within navigateToContentEditorTop.
…'s toolbar buttons
…abThroughBlockToolbar to e2e-test-utils
…5-add-e2e-keyboard-navigation-util-functions # Conflicts: # packages/e2e-tests/specs/keyboard-navigable-blocks.test.js
…olbar, tab-through-block-mover-control, keyboard-navigable-blocks-test. Add some detail to JSDocs
…-functions # Conflicts: # packages/e2e-tests/specs/keyboard-navigable-blocks.test.js
tjnicolaides
requested review from
aduth,
ajitbohra,
gziolo,
nerrad,
ntwb,
talldan and
youknowriad
as code owners
gziolo
added
[Type] Automated Testing
|
This is great work ❤️ I hope to find some time to give it a spin and perform in-depth testing later this week. This PR is quite big so it might take some time to land it unless you have some idea how to extract some chunk into smaller PRs. |
|
I'm glad to hear you say that - I'd let enough time pass that I was worried I had lost sight of what we really wanted in this PR. It's okay if this takes longer to get in - I'll keep it updated while it's in the review process. I see we already have a conflict, so I'll get on that. Thanks! |
…-functions # Conflicts: # packages/e2e-tests/specs/keyboard-navigable-blocks.test.js
| await page.keyboard.type( content ); | ||
|
|
||
| // if there are more contenteditable elements, select and populate them too: | ||
| const blocks = await textContentAreas( { empty: true } ); |
There was a problem hiding this comment.
This should populate other content areas within the same block, is that correct? Yet I noticed when running keyboard-navigable-blocks.test.js in interactive mode that the citation field in the Quote block did not get populated. Shouldn't it have done so?
| '.wp-block.is-selected [contenteditable]', | ||
| '.wp-block.is-typing [contenteditable]', | ||
| ].map( ( selector ) => { | ||
| return empty ? selector + '[data-is-placeholder-visible="true"]' : selector; |
There was a problem hiding this comment.
Where is data-is-placeholder-visible expected to appear? I can't find it anywhere in the codebase.
There was a problem hiding this comment.
Hey, sorry to just now be getting on these changes. data-is-placeholder-visible appears to be a tinymce thing. The project had some snapshot tests that displayed those attributes at one point: ad182a5
There was a problem hiding this comment.
Yup, it's not being used. None of the files in that commit even exist anymore. So if you inspect the rich text elements with your browser dev tools, you can confirm that data-is-placeholder-visible never appears on any element.
That's why the blocks variable here is always an empty array, so the for loop underneath never runs, so the citation in the quote block doesn't get populated.
There was a problem hiding this comment.
OK, done! Apart from the comments I left, my only general observation is it's not great to depend too much on static classnames to target elements in the e2e tests, as changing a class by itself should not cause a test to fail. But there are instances of that sprinkled throughout the tests, so not something to be fixed in this PR 🙂
|
|
||
| describe( 'Order of block keyboard navigation', () => { | ||
| beforeEach( async () => { | ||
| await createNewPost(); | ||
| } ); | ||
| it( 'permits tabbing through blocks in the expected order', async () => { |
There was a problem hiding this comment.
It would be better to break this up into a test for each block; possibly even a describe section for each block and a separate test for each part of the block. We should aim to have as few assertions as possible in each test.
As it stands, if I cause the test to fail by changing the classname for one of the block-mover__controls, it tells me nothing about which block the failure happened in, or if it failed for all the blocks, so it won't be very useful in identifying where the problem is.
There was a problem hiding this comment.
I hear what you're saying, and after starting to look at this this evening, here's what I think I am moving towards for this PR. Please let me know if I'm misunderstanding your recommendation at all, or if I have any opportunities to consolidate Jest logic:
describe( 'Order of heading block keyboard navigation', () => {
beforeEach( async () => {
await createNewPost();
} );
it( 'permits tabbing through heading blocks in the expected order', async () => {
await insertAndPopulateBlock( 'Heading', 'Heading Block Content' );
await navigateToContentEditorTop();
await tabThroughTextBlock( 'core/heading', 'Heading Block Content' );
} );
} );
describe( 'Order of quote block keyboard navigation', () => {
beforeEach( async () => {
await createNewPost();
} );
it( 'permits tabbing through quote blocks in the expected order', async () => {
await insertAndPopulateBlock( 'Quote', 'Quote Block Content' );
await insertAndPopulateBlock( 'Quote', 'Quote Block Content 2' );
await navigateToContentEditorTop();
await tabThroughTextBlock( 'core/quote', 'Quote Block Content' );
} );
} );
describe( 'Order of list block keyboard navigation', () => {
beforeEach( async () => {
await createNewPost();
} );
it( 'permits tabbing through list blocks in the expected order', async () => {
await insertAndPopulateBlock( 'List', 'List Block Content' );
await insertAndPopulateBlock( 'List', 'List Block Content 2' );
await navigateToContentEditorTop();
await tabThroughTextBlock( 'core/list', 'List Block Content' );
} );
} );
// ..... etc., etc.
There was a problem hiding this comment.
How about:
describe( 'Order of block keyboard navigation', () => {
beforeEach( async () => {
await createNewPost();
} );
// ...
it( 'permits tabbing through quote blocks in the expected order', async () => {
await insertAndPopulateBlock( 'Quote', 'Quote Block Content' );
await insertAndPopulateBlock( 'Quote', 'Quote Block Content 2' );
await navigateToContentEditorTop();
await tabThroughTextBlock( 'core/quote', 'Quote Block Content' );
} );
it( 'permits tabbing through list blocks in the expected order', async () => {
await insertAndPopulateBlock( 'List', 'List Block Content' );
await insertAndPopulateBlock( 'List', 'List Block Content 2' );
await navigateToContentEditorTop();
await tabThroughTextBlock( 'core/list', 'List Block Content' );
} );
} );This way you won't have to add several describe and beforeEach sections.
There was a problem hiding this comment.
Update: I went the rest of the way and made the change here. Looking forward to seeing how this reports out in travis. https://github.com/WordPress/gutenberg/pull/16707/files#diff-448359ec0ae4a4e7ee9545bf9f9b43fc
There was a problem hiding this comment.
Thank you both for the feedback - this should be looking closer to what you had in mind, at this point: https://github.com/WordPress/gutenberg/pull/16707/files#diff-448359ec0ae4a4e7ee9545bf9f9b43fc
| export async function textContentAreasHaveFocus( content ) { | ||
| const blocks = await textContentAreas( { empty: false } ); | ||
| const isFocusedTextContentArea = await page.evaluate( () => document.activeElement.contentEditable ); | ||
| const textContentAreaContent = await page.evaluate( () => document.activeElement.innerHTML ); |
There was a problem hiding this comment.
isFocusedTextContentArea and textContentAreaContent need to be moved inside the loop, under the if block, otherwise their value won't be updated as you cycle through the content areas.
There was a problem hiding this comment.
(Doing so will cause the tests to fail, which confirms my earlier observation that the citation in the Quote block isn't being populated.)
There was a problem hiding this comment.
I'll have a closer look at this, but when I'm looking at the tests run, I'm seeing the citation field of the quote block being populated with text as expected. It should be entering "Quote Block Content" into the main content area, as well as the lighter grey, smaller font size citation content area. Is this correct?
There was a problem hiding this comment.
It should be entering "Quote Block Content" into the main content area, as well as the lighter grey, smaller font size citation content area.
That's what I was expecting it to do, but when I ran the tests on this branch locally the citation wasn't populated. And after making the change to the loop I mentioned above, the tests failed, which is consistent with the citation field being empty.
…re returning promise objects. Removing await from simple assertions that do not need it
…re returning promise objects.
…re returning promise objects.
…r remainder of the new e2e-test-utils JSDocs
…n each for block to verify the assertions are being made about the expected elements.
…isolated 'describe' / 'it' tests
packages/e2e-tests/specs/keyboard-navigable-content-editor.test.js
Outdated
Show resolved
Hide resolved
|
|
||
| describe( 'Order of block keyboard navigation', () => { | ||
| beforeEach( async () => { | ||
| await createNewPost(); | ||
| } ); | ||
| it( 'permits tabbing through blocks in the expected order', async () => { |
There was a problem hiding this comment.
How about:
describe( 'Order of block keyboard navigation', () => {
beforeEach( async () => {
await createNewPost();
} );
// ...
it( 'permits tabbing through quote blocks in the expected order', async () => {
await insertAndPopulateBlock( 'Quote', 'Quote Block Content' );
await insertAndPopulateBlock( 'Quote', 'Quote Block Content 2' );
await navigateToContentEditorTop();
await tabThroughTextBlock( 'core/quote', 'Quote Block Content' );
} );
it( 'permits tabbing through list blocks in the expected order', async () => {
await insertAndPopulateBlock( 'List', 'List Block Content' );
await insertAndPopulateBlock( 'List', 'List Block Content 2' );
await navigateToContentEditorTop();
await tabThroughTextBlock( 'core/list', 'List Block Content' );
} );
} );This way you won't have to add several describe and beforeEach sections.
|
This PR is getting closer to be ready to merge, great iterations 👍 I added mostly nitpicks to ensure we only expose general-purpose helpers which are named in a way where it's easier to identify what they do when located outside of the existing tests. |
…overControls to tabThroughBlockMovers. Changing the name of one describe block, and removing unnecessary nesting added to keyboard-navigable-blocks.test.js
…TextContentAreas. Updating e2e-test-utils/README
…-functions # Conflicts: # packages/e2e-test-utils/README.md # packages/e2e-test-utils/src/index.js
…tion-util-functions
…tion-util-functions
|
In the mean time, we moved from puppeteer to playwright so if this PR still need to be merged, it will need to be redone entirely. So I'm just closing for now. Thanks for the efforts. |
Description
Per @gziolo's suggestion, refactored the keyboard navigability tests to make the individual assertions easier to reuse and to make the tab orders less brittle. There is now an additional test that checks the keyboard navigation tab order of many different types of content blocks.
Related PR: #13455
How has this been tested?
The test is passing when running
npm run test-e2e.It also returns no errors or warnings when running
npm run lint-js.Screenshots
This recording of the new test running in non-headless mode shows the script adding blocks and tabbing through them:
Types of changes
keyboard-navigable-content-editor.test.jstabThroughTextBlock, orinsertAndPopulateBlock, and assertions (such asinserterToggleHasFocusortextContentAreasHaveFocus) into e2e-test-utilsChecklist: