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

Fix e2e test: 'inserts a default block on bottom padding click' #65138

Closed
stokesman opened this issue Sep 8, 2024 · 2 comments
Closed

Fix e2e test: 'inserts a default block on bottom padding click' #65138

stokesman opened this issue Sep 8, 2024 · 2 comments
Labels
[Package] E2E Tests /packages/e2e-tests [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests.

Comments

@stokesman
Copy link
Contributor

stokesman commented Sep 8, 2024

So it looks like #64639 regressed the click below to add block (there should really have been some e2e tests).

There is one but apparently it doesn’t fail:

test( 'inserts a default block on bottom padding click', async ( {

Originally posted by @stokesman in #64992 (comment)

@stokesman stokesman added [Type] Bug An existing feature does not function as intended [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Package] E2E Tests /packages/e2e-tests and removed [Type] Bug An existing feature does not function as intended labels Sep 8, 2024
@t-hamano
Copy link
Contributor

t-hamano commented Sep 9, 2024

After investigating this issue, I found that this test hadn't failed before because it was being tested with TT1.

In classic themes such as TT1, the 8px padding is applied to the document:

html :where(.editor-styles-wrapper) {
padding: 8px;

This satisfies the conditional statement below, allowing the block to be inserted:

if ( ! paddingBottom ) {
return;
}

In block themes, however, this padding doesn't exist, so the above conditional statement is met and the early return prevents the block from being inserted.

Perhaps the conditional statement itself is unnecessary?

@stokesman
Copy link
Contributor Author

stokesman commented Sep 10, 2024

Thanks for investigating. It looks like you’re right about why it wasn’t failing. So had it been ran against a block-based theme it would have failed with #64639.

The test does fail now if I break the hook. So I guess we can close this issue. I had not thought to quickly check that before making this and I just didn’t want to forget to follow up.

Perhaps the conditional statement itself is unnecessary?

As best I can tell the purpose of the early return in that case is to effectively disable the hook when editing in a context where the “typewriter” 40vh space isn’t being added. I think it would be better to truly disable the hook based on the same condition for adding the space. Then the hook doesn’t have try detecting the space and the condition can be removed.

I have a branch like that and am meaning to make it a PR to follow up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] E2E Tests /packages/e2e-tests [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests.
Projects
None yet
Development

No branches or pull requests

2 participants