Fix cover matrix alignment #28404

ajlende · 2021-01-21T18:32:14Z

Description

Fixes cover block positioning when the text align matrix is used. Similar to #28365, but I was still having some issues on save with positioning without adding the height.

I collapsed the fix that was used in edit into save to resolve that positioning issue.

How has this been tested?

Use cover block with matrix alignment
Try different block alignments
Try different combinations of repeat and fixed

Screenshots

Edit Before

Edit After

Save Before

Save After

Types of changes

Bug fix

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR.

jasmussen · 2021-01-21T18:47:36Z

Thanks for looking at this! This fix reminds me a lot of the fix i tried yesterday: #28361

That is, it seemed like a good approach to me, but @ockham discovered some issues with it on the frontend.

ockham · 2021-01-21T19:00:48Z

Thanks @ajlende! This is looking good to me; I can confirm that I'm not seeing the glitch I was seeing with #28361, nor the one I was seeing with #28365.

I'll do some more testing...

jasmussen · 2021-01-21T19:02:47Z

Looking at the code, I like this fix more than mine. And forgive me for stating the obvious, but we should only merge one of the two 😅

ockham · 2021-01-21T19:03:03Z

packages/block-library/src/cover/style.scss

+	// Extra specificity for in edit mode where other styles would override it otherwise.
+	img.wp-block-cover__image-background,
+	video.wp-block-cover__video-background {


Do we need the img and video element-based selectors? 😅 I was hoping that moving the selectors into .wp-block-cover would be specific enough.

Suggested change

// Extra specificity for in edit mode where other styles would override it otherwise.

img.wp-block-cover__image-background,

video.wp-block-cover__video-background {

.wp-block-cover__image-background,

.wp-block-cover__video-background {

It wasn't working for me without it, unfortunately. Without it, the specificity is the same and these styles don't get applied.

edit: for the record, the selector that I needed to override was .editor-styles-wrapper img

Since the extra specificity is only needed in the editor, I could duplicate these styles in edit.scss with the extra specificity and move these out to where they were before. But, I figured it would be better to avoid duplication between edit and save wherever possible.

ajlende · 2021-01-21T19:03:34Z

@ockham Thanks for testing, I'm also trying a few more cases. You may come across a visual glitch with repeated backgrounds, and I already have a fix for that one in #28362

ockham · 2021-01-21T19:10:28Z

Some visual differences on the frontend with Twenty Twenty.

This PR:

#28365:

ajlende · 2021-01-21T19:11:47Z

I also found an issue with left and right alignment

ockham · 2021-01-21T19:13:47Z

I also found an issue with left and right alignment

Ohh indeed. Twenty Twenty, frontend, right aligned:

(Edit, the horizontal lines are part of 2020's post layout.)

ockham · 2021-01-21T19:18:04Z

In the editor, for reference:

ajlende · 2021-01-21T19:27:32Z

That last commit seemed to fix the positioning in Twenty Twenty-One

Edit:

Save:

ajlende · 2021-01-21T19:57:00Z

Think I got the height fixed for Twenty Twenty

Edit:

Save:

ockham · 2021-01-21T19:58:12Z

I think we're getting closer. Editor and frontend are starting to look good in most of the Twenties, with different alignments.

Full width in TT1 still isn't quite working on the frontend:

Editor:

(Edit, pasted wrong screenshot first.)

ajlende · 2021-01-21T20:01:27Z

I think my last change may have fixed that too. Give it a try

ockham · 2021-01-21T20:03:20Z

I think my last change may have fixed that too. Give it a try

Oh wow, looks like! 🎉

ockham · 2021-01-21T20:07:01Z

I think I should start creating a test matrix to keep track of the tricky ones:

Test the Twenties from 2021 backwards.
Set the Cover blocks' content position.
Test all possible alignments (left, center, right) (2019-2021: also wide and full width) of the Cover block.
Always test both editor and frontend.

Known problematic cases:

2021, frontend, full width. (#)
2020, frontend, right alignment. (#)
2014, frontend, center. (#)
2011, both editor and frontend, left aligned. (#)

@ajlende Am I missing anything?

ajlende · 2021-01-21T20:09:32Z

My testing hasn't made it back past Twenty Nineteen yet, but for the things that I have tested so far, that's a complete list

ockham · 2021-01-21T20:29:24Z

I've covered all the Twenties now (although I didn't go through all alignments for each of them), and things are looking good 🎉

It's getting late here, and I'd like to have @jasmussen take a final look at this to weigh in on the CSS changes (and maybe give it some more testing). If all goes well, I'll merge tomorrow (and will release 9.8.1).

ajlende · 2021-01-21T20:37:19Z

I'm sitting here in GMT-10 so I have a lot of daylight left. I'll have something ready for you and Joen in the morning.

Found one bug with the focal point picker in Twenty Twenty ~~(works fine in Twenty Twenty-One)~~, so that might be worth adding to the list of things to check.

edit: It's not a bug with the theme, ~~it appears to be a bug with center alignment applied~~.

edit2: Now the bug is showing up with other alignments as well. Maybe a JS thing, more investigation is needed.

stokesman · 2021-01-21T21:12:25Z

edit2: Now the bug is showing up with other alignments as well. Maybe a JS thing, more investigation is needed.

I don't know if it is the same thing you're seeing but I just made #28406 to fix an issue I've been seeing with the Focal point picker.

ajlende · 2021-01-21T21:32:03Z

@stokesman That's exactly the issue. I was wondering if it might be related because I moved a piece of code that said, "This explicit height rule is necessary for the focal point picker to work."

https://github.com/WordPress/gutenberg/pull/28404/files#diff-9e449d4b8bd69c9771de8e20c9a1d78798a6934f99e3bfc432df6ceb23def7f4L8

Based on the other CSS I have it doesn't seem like that line is useful, so I might remove it and then test a bunch to confirm.

jasmussen · 2021-01-22T06:44:47Z

@stokesman and @ajlende, I added that line:

Based on the other CSS I have it doesn't seem like that line is useful, so I might remove it and then test a bunch to confirm.

I added it when I restored the height: 100%; after the focal point picker had regressed. However the height rule was initially removed from the Cover block to mitigate issues stemming from the Cover being used inside flex based boxes. So if it's possible to remove that rule in a safe way, it would be great. CC: @jffng

jasmussen · 2021-01-22T07:33:29Z

packages/block-library/src/cover/style.scss

@@ -4,8 +4,6 @@
 	background-size: cover;
 	background-position: center center;
 	min-height: 430px;
-	width: 100%;


This rule was moved from style.scss to editor.scss, which makes me nervous. In general, unless it's explicitly for block editor specific shenanigans, like extra divs, adjacent markup, or visuals that appear only in the editor, my instinct is to keep as much of the CSS as possible in style.scss. That way any visual issues you fix on the frontend, you also fix in the editor.

I might move this back, but I need to test a little bit more first.

jasmussen · 2021-01-22T07:47:48Z

Alright, I'm taking a deep dive this morning, and already now I have a few high level thoughts to share:

First off, this PR is good and short. We can potentially make it simpler, I left a comment, but the basics seem correct.

In testing the various twenties, it seems additionally clear that the change from a CSS background to an img element not only was the primary cause of the issue, but likely that there are additional adjacent issues. Specifically, many themes style the img, and that bleeds into the cover now.

Because of those adjacent issues, we might want to strengthen some selectors and add additional rules. But I'm not sure whether that's something for this PR or something best to keep separate.

Specifically, Twenty Eleven is a great theme to test, because it has pretty elaborate rules that target the img directly:

Behold:

A 1px white gray border, padding, and more.

This PR gets closer in addressing that, but it doesn't get all the way:

As far as I'm aware, the only way to prevent this CSS bleed is to use !important, and target every extra selector we encounter. Something like this:

[the crazy cover selector] {
    border: none !important;
    max-width: none !important;
    padding: 0 !important;
    margin: 0 !important;
    width: 100% !important;
    height: 100% !important;
    position: absolute !important;
    top: 0 !important;
    left: 0 !important;
    right: 0 !important;
    bottom: 0 !important;
    object-fit: cover !important;
}

What other things might a theme do? Box-shadow? Outlines? CSS filters? Is there ever an edgecase where we would want to allow a theme's image styles to bleed into the cover, or do we have to treat the img as purely a dumb element?

I'm nervous about going down this nuclear road, but it seems clear from testing that it's an arms race now that the img element is in play. So we kind of have to make a decision where to go.

jasmussen · 2021-01-22T13:12:17Z

Alright, I pushed a change to move the height rule back to the style.scss file, and to add a bunch of extra properties to unset, trying to pre-empt what might bleed in from a theme.

That effectively makes this PR identical to #28412, except softer: the rules have high specificity, but not "nuclear" (there are no !importants).

And it solves the TwentyEleven theme case:

I think this is good enough to merge as a fix, and we can always look at whether we need to increase the specificity here.

ockham · 2021-01-22T13:23:53Z

2020 frontend, right aligned, is back to this:

Prior to 21ad5ef, it was like this:

(Which isn't perfect, but better?)

jasmussen · 2021-01-22T13:26:04Z

Let me take a look at that one. Maybe it was the moving of the 100% height, after all.

jasmussen · 2021-01-22T13:34:45Z

Yeah, it appears that height: 100%; needs to be in the editor only. Even there, I'd still like to remove it, and #28406 might be our opportunity to do that. But that's for another day.

Before:

After:

Also rebased.

ockham · 2021-01-22T13:35:34Z

Thanks Joen! ❤️

jasmussen · 2021-01-22T13:37:12Z

Okay I'm not sure I rebased that right, but it wasn't strictly necessary, and the branch appears to work as intended still.

ockham · 2021-01-22T13:42:09Z

Seeing one editor/frontend inconsistency in 2021 (center alignment):

Editor:

Frontend:

Disclaimer, might be pre-existing. My testing is probably overly paranoid now.

jasmussen · 2021-01-22T13:59:00Z

Wow that's a good catch!

That's a pre-existing issue, and appears to be a bug with the demo content, no less. Specifically, the demo content has both align center, and align wide applied, which takes on the frontend but not in the editor.

If you delete the cover block from the demo content, and create a fresh one, then align it wide or center in however direction you want to go, it works fine, both in the editor and published.

ockham · 2021-01-22T14:08:32Z

Wow that's a good catch!

That's a pre-existing issue, and appears to be a bug with the demo content, no less. Specifically, the demo content has both align center, and align wide applied, which takes on the frontend but not in the editor.

If you delete the cover block from the demo content, and create a fresh one, then align it wide or center in however direction you want to go, it works fine, both in the editor and published.

Hmm, interesting. So it seems like the block has the alignwide className set when first loading it:

<!-- wp:cover {"url":"https://cldup.com/Fz-ASbo2s3.jpg","className":"alignwide"} -->
<div class="wp-block-cover has-background-dim alignwide"><img class="wp-block-cover__image-background" alt="" src="https://cldup.com/Fz-ASbo2s3.jpg" data-object-fit="cover"/><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">Of Mountains &amp; Printing Presses</p>
<!-- /wp:paragraph --></div></div>
<!-- /wp:cover -->

Then, when I set the alignment to center:

<!-- wp:cover {"url":"https://cldup.com/Fz-ASbo2s3.jpg","align":"center","className":"alignwide"} -->
<div class="wp-block-cover aligncenter has-background-dim alignwide"><img class="wp-block-cover__image-background" alt="" src="https://cldup.com/Fz-ASbo2s3.jpg" data-object-fit="cover"/><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">Of Mountains &amp; Printing Presses</p>
<!-- /wp:paragraph --></div></div>
<!-- /wp:cover -->

So now we have "align":"center", but the wide alignment is also kept in className.

Not sure this is a bug with the demo content though. It looks a bit like it's a block migration that's not working properly maybe?

(Anyway, not directly related to this PR. Just noting here FTR and a later fix.)

jasmussen · 2021-01-22T14:14:02Z

Yep, there's some weirdness there, but all of that I reproduced in the main branch, so no blockers for this one, I'd say.

ockham

Okay, I've given another round of testing to the known troublemakers, and they're looking good now 👍

Co-authored-by: jasmussen <joen@automattic.com>

ockham · 2021-01-22T14:24:18Z

Cherry-picked to release/9.8.

ajlende added [Type] Bug An existing feature does not function as intended [Block] Cover Affects the Cover Block - used to display content laid over a background image labels Jan 21, 2021

ajlende requested review from ockham and jasmussen January 21, 2021 18:32

ajlende self-assigned this Jan 21, 2021

ajlende requested a review from ajitbohra as a code owner January 21, 2021 18:32

ajlende mentioned this pull request Jan 21, 2021

Revert "Cover: Fix matrix alignment issue." #28364

Merged

ajlende force-pushed the fix/cover-matrix-alignment branch from a599338 to 21dfbb7 Compare January 21, 2021 18:41

ockham reviewed Jan 21, 2021

View reviewed changes

ockham mentioned this pull request Jan 21, 2021

Try: Alternate cover fix. #28365

Closed

jasmussen reviewed Jan 22, 2021

View reviewed changes

jasmussen mentioned this pull request Jan 22, 2021

Try: Cover block image rules, very high specificity. #28412

Closed

ajlende and others added 6 commits January 22, 2021 14:31

Fix cover matrix alignment

e9e1ab8

Consolidate editor and save styles

729d653

Override img margin/padding from .wp-block-cover.alignright > *

8b698ab

Fix cover block height in Twenty Twenty

ad72dcf

Add more rules.

e4dee72

Move height back.

2543962

jasmussen force-pushed the fix/cover-matrix-alignment branch from c501fb3 to 2543962 Compare January 22, 2021 13:36

ockham approved these changes Jan 22, 2021

View reviewed changes

ockham merged commit 8cf37d4 into WordPress:master Jan 22, 2021

github-actions bot added this to the Gutenberg 9.9 milestone Jan 22, 2021

ockham pushed a commit that referenced this pull request Jan 22, 2021

Fix cover matrix alignment (#28404)

1ea8dce

Co-authored-by: jasmussen <joen@automattic.com>

ockham modified the milestones: Gutenberg 9.9, Gutenberg 9.8 Jan 22, 2021

This was referenced Jan 25, 2021

Demo content cover block alignment not respected. Migration needed? #28476

Closed

Add visual diffs #28477

Open

Try: Visual regression test coverage for Cover block #28554

Open

Fix cover matrix alignment #28404

Fix cover matrix alignment #28404

Conversation

ajlende commented Jan 21, 2021 • edited Loading

Description

How has this been tested?

Screenshots

Edit Before

Edit After

Save Before

Save After

Types of changes

Checklist:

jasmussen commented Jan 21, 2021

ockham commented Jan 21, 2021

jasmussen commented Jan 21, 2021

ockham Jan 21, 2021 • edited Loading

Choose a reason for hiding this comment

ajlende Jan 21, 2021 • edited Loading

Choose a reason for hiding this comment

ajlende Jan 21, 2021

Choose a reason for hiding this comment

ajlende commented Jan 21, 2021

ockham commented Jan 21, 2021

ajlende commented Jan 21, 2021

ockham commented Jan 21, 2021 • edited Loading

ockham commented Jan 21, 2021

ajlende commented Jan 21, 2021

ajlende commented Jan 21, 2021

ockham commented Jan 21, 2021 • edited Loading

ajlende commented Jan 21, 2021

ockham commented Jan 21, 2021

ockham commented Jan 21, 2021 • edited Loading

ajlende commented Jan 21, 2021

ockham commented Jan 21, 2021 • edited Loading

ajlende commented Jan 21, 2021 • edited Loading

stokesman commented Jan 21, 2021

ajlende commented Jan 21, 2021

jasmussen commented Jan 22, 2021

jasmussen Jan 22, 2021

Choose a reason for hiding this comment

jasmussen commented Jan 22, 2021

jasmussen commented Jan 22, 2021

ockham commented Jan 22, 2021

jasmussen commented Jan 22, 2021

jasmussen commented Jan 22, 2021

ockham commented Jan 22, 2021

jasmussen commented Jan 22, 2021

ockham commented Jan 22, 2021

jasmussen commented Jan 22, 2021 • edited Loading

ockham commented Jan 22, 2021

jasmussen commented Jan 22, 2021

ockham left a comment

Choose a reason for hiding this comment

ockham commented Jan 22, 2021

ajlende commented Jan 21, 2021 •

edited

Loading

ockham Jan 21, 2021 •

edited

Loading

ajlende Jan 21, 2021 •

edited

Loading

ockham commented Jan 21, 2021 •

edited

Loading

ockham commented Jan 21, 2021 •

edited

Loading

ockham commented Jan 21, 2021 •

edited

Loading

ockham commented Jan 21, 2021 •

edited

Loading

ajlende commented Jan 21, 2021 •

edited

Loading

jasmussen commented Jan 22, 2021 •

edited

Loading