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

Try: Remove canvas padding. #22213

Merged
merged 3 commits into from
Jun 22, 2020
Merged

Try: Remove canvas padding. #22213

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4370b73
Try: Remove canvas padding.
jasmussen May 8, 2020
32d0b0c
Update snapshot.
jasmussen Jun 22, 2020
6e0ff30
Try: Fix test.
jasmussen Jun 22, 2020
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
6 changes: 6 additions & 0 deletions packages/e2e-tests/specs/editor/various/typewriter.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ describe( 'TypeWriter', () => {
// Create first block.
await page.keyboard.press( 'Enter' );

// Create second block.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ellatrix Not urgent, but when you have a moment I'd love your take on why this specific commit makes the tests pass.

For context, this PR changes the padding of the editing canvas to be smaller. This leaves more room for themes that don't add this padding themselves, i.e. 2020 which is being used for these tests.

The tests failing were

TypeWriter › should maintain caret position
TypeWriter › should maintain caret position after leaving last editable

I tested both of those using npm run test-e2e packages/e2e-tests/specs/edi tor/various/typewriter.test.js -- --puppeteer-interactive, passing on master but failing on this branch prior to this commit.

My guess is that with the padding as it was, there was enough room on the canvas for the blocks to fit without the typewriter effect setting in. This would explain why simply adding some enter-key presses fixed the test for me.

But it does suggest that this particular test is very sensitive to things style properties like padding, margin, line-height and font size. For example if TwentyTwenty updates the editor style to add this padding back, or to change a block margin, this test will likely fail again. Is there a way to make it more resilient than just adding more blocks? Or conversely, is simply adding more blocks an okay way to fix this test?

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

const initialPosition = await getCaretPosition();

// The page shouldn't be scrolled when it's being filled.
Expand Down Expand Up @@ -118,8 +121,11 @@ describe( 'TypeWriter', () => {
await page.keyboard.press( 'Enter' );
// Create second block.
await page.keyboard.press( 'Enter' );
// Create third block.
await page.keyboard.press( 'Enter' );
// Move to first block.
await page.keyboard.press( 'ArrowUp' );
await page.keyboard.press( 'ArrowUp' );

const initialPosition = await getCaretPosition();

Expand Down