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 nested cover block bug #28114

Merged
merged 2 commits into from
Jan 12, 2021
Merged

Conversation

jffng
Copy link
Contributor

@jffng jffng commented Jan 11, 2021

This PR removes height: 100% from the default styles of the cover block.

I'm not certain, but I think this line causes more issues than the uses cases that make it worth keeping. In particular it causes a bug when a cover block appears inside of a flex container (like a columns block), causing the height the cover to take up 100% of a container which may contain other blocks — pushing those blocks outside of the document flow:

Master This PR
Screen Shot 2021-01-11 at 3 09 10 PM Screen Shot 2021-01-11 at 3 09 44 PM

Another example of this bug: Automattic/themes#2976 (comment)

How to test

  • GB master using the Twenty Twenty-One theme
  • Add a columns block and a cover inside it (you can use this block markup)
  • Verify the issue occers
  • Check out this PR and verify the issue is resolved

@scruffian
Copy link
Contributor

This also fixes a bug with resizing the height of a cover block. When inside a columns block the resize handles don't work on master but they do with this PR. 🚢

This may also fix #17921 and #17919.

Copy link
Contributor

@scruffian scruffian left a comment

Choose a reason for hiding this comment

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

You might want to also get a review from @jasmussen or @kjellr.

@jasmussen
Copy link
Contributor

Copy link
Member

@aristath aristath left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@senadir
Copy link
Contributor

senadir commented Jan 12, 2021

I can't really remember why I introduced that (it was a long time ago), my best guess was to ensure compatibility with the resizableBox component

@jasmussen
Copy link
Contributor

Seems like there are a lot of benefits to trying it. Let's keep an eye out for any bugs that might ensue.

@jffng
Copy link
Contributor Author

jffng commented Jan 12, 2021

There is a failing e2e test that may be related:

FAIL specs/editor/various/typewriter.test.js (14.409 s)

I will try looking into this.

@jasmussen
Copy link
Contributor

If the test turns particularly gnarly, you can try npm run test-e2e -- --puppeteer-interactive, which should show you a small Chrome window running the particular test. That command runs all the tests, that takes a while, but you can also point to the particular failing test.

@scruffian scruffian merged commit 3de1114 into WordPress:master Jan 12, 2021
@github-actions github-actions bot added this to the Gutenberg 9.8 milestone Jan 12, 2021
@jasmussen jasmussen mentioned this pull request Jan 18, 2021
@ockham
Copy link
Contributor

ockham commented Jan 18, 2021

Noting that this seems to have caused #28242. Tentative fix by @jasmussen here: #28287. (Thanks Joen!)

@jasmussen
Copy link
Contributor

I had to restore the height: 100%; in #28350 because as it turned out, it was necessary for the focal point picker to work.

An interim fix, adding overflow: hidden; in its place, had its own issue, it cropped the resize handle and focus style.

If you can provide some demo content that produces the issue you see with a cover block inside a column, I'd be happy to try and look for an alternate fix.

ockham added a commit that referenced this pull request Jan 21, 2021
This reverts commit 1796afc.

Reason: Visually broken on the frontend

This basically gets us back to the state of Cover block styling prior to #28114.
ockham added a commit that referenced this pull request Jan 21, 2021
This reverts commit 1796afc.

Reason: Visually broken on the frontend

This basically gets us back to the state of Cover block styling prior to #28114.
@scruffian
Copy link
Contributor

Thanks @jasmussen. This is the post content that causes the issue:

<!-- wp:jetpack/layout-grid {"column1DesktopSpan":12,"column1TabletSpan":8,"column1MobileSpan":4} -->
<div class="wp-block-jetpack-layout-grid alignfull column1-desktop-grid__span-12 column1-desktop-grid__row-1 column1-tablet-grid__span-8 column1-tablet-grid__row-1 column1-mobile-grid__span-4 column1-mobile-grid__row-1"><!-- wp:jetpack/layout-grid-column -->
<div class="wp-block-jetpack-layout-grid-column wp-block-jetpack-layout-grid__padding-none"><!-- wp:cover {"url":"https://scruffiansimple.files.wordpress.com/2020/08/cropped-3d2cb-mountaindawn.jpg","id":1528} -->
<div class="wp-block-cover has-background-dim" style="background-image:url(https://scruffiansimple.files.wordpress.com/2020/08/cropped-3d2cb-mountaindawn.jpg)"><div class="wp-block-cover__inner-container"><!-- wp:paragraph {"align":"center","placeholder":"Write title…","fontSize":"large"} -->
<p class="has-text-align-center has-large-font-size">cover</p>
<!-- /wp:paragraph --></div></div>
<!-- /wp:cover -->

<!-- wp:media-text {"mediaId":1221,"mediaLink":"https://scruffiansimple.wordpress.com/placeholder-image-10/","mediaType":"image"} -->
<div class="wp-block-media-text alignwide is-stacked-on-mobile"><figure class="wp-block-media-text__media"><img src="https://scruffiansimple.files.wordpress.com/2020/09/maywood-logo.png" alt="" class="wp-image-1221 size-full"/></figure><div class="wp-block-media-text__content"><!-- wp:paragraph {"placeholder":"Content…","fontSize":"large"} -->
<p class="has-large-font-size">media + text</p>
<!-- /wp:paragraph --></div></div>
<!-- /wp:media-text --></div>
<!-- /wp:jetpack/layout-grid-column --></div>
<!-- /wp:jetpack/layout-grid -->

You'll need a Jetpack site to test it on. There are more details here: Automattic/themes#2976

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants