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

Cover block: Restore default overlay background #26569

Merged
merged 5 commits into from
Nov 2, 2020

Conversation

sirreal
Copy link
Member

@sirreal sirreal commented Oct 29, 2020

Description

The default Cover block overlay background was removed. This changed the
style of existing blocks on existing sites.

Restore the default background in such a way that it does not conflict
with theme-provided background-color styles and there is no need to
artificially add extra specificity.

Fixes #26545
Closes #26564 (superseded)

How has this been tested?

Try to reproduce this issue: #26545

Be sure that the issue described in #25290 is no longer present.

There appear to be problems with the Docker registry so I'm unable to easily test this with wp-env. I did manually apply the changes via developer tools and they seem to fix the issue.

Types of changes

Bug fix

Fix a regression where the Cover block lost its default background overlay color (#26545).

The default Cover block overlay background was removed. This changed the
style of existing blocks on existing sites.

Restore the default background in such a way that it does not conflict
with theme-provided background-color styles and there is no need to
artificially add extra specificity.

Fix regression: #26545
@jasmussen
Copy link
Contributor

This is working well for me:

cover

I dig this approach. It seems to thread the needle! Curious about @jorgefilipecosta and @nosolosw but otherwise, 🚢

@github-actions
Copy link

github-actions bot commented Oct 29, 2020

Size Change: +119 B (0%)

Total Size: 1.21 MB

Filename Size Change
build/block-library/style-rtl.css 7.86 kB +26 B (0%)
build/block-library/style.css 7.86 kB +23 B (0%)
build/components/index.js 172 kB +70 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.78 kB 0 B
build/api-fetch/index.js 3.45 kB 0 B
build/autop/index.js 2.84 kB 0 B
build/blob/index.js 665 B 0 B
build/block-directory/index.js 8.72 kB 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/index.js 130 kB 0 B
build/block-editor/style-rtl.css 11.1 kB 0 B
build/block-editor/style.css 11.1 kB 0 B
build/block-library/editor-rtl.css 8.98 kB 0 B
build/block-library/editor.css 8.98 kB 0 B
build/block-library/index.js 146 kB 0 B
build/block-library/theme-rtl.css 837 B 0 B
build/block-library/theme.css 838 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.1 kB 0 B
build/components/style-rtl.css 15.2 kB 0 B
build/components/style.css 15.2 kB 0 B
build/compose/index.js 9.81 kB 0 B
build/core-data/index.js 12.3 kB 0 B
build/data-controls/index.js 772 B 0 B
build/data/index.js 8.77 kB 0 B
build/date/index.js 31.8 kB 0 B
build/deprecated/index.js 768 B 0 B
build/dom-ready/index.js 571 B 0 B
build/dom/index.js 4.46 kB 0 B
build/edit-navigation/index.js 11.2 kB 0 B
build/edit-navigation/style-rtl.css 881 B 0 B
build/edit-navigation/style.css 885 B 0 B
build/edit-post/index.js 306 kB 0 B
build/edit-post/style-rtl.css 6.38 kB 0 B
build/edit-post/style.css 6.37 kB 0 B
build/edit-site/index.js 22.1 kB 0 B
build/edit-site/style-rtl.css 3.86 kB 0 B
build/edit-site/style.css 3.85 kB 0 B
build/edit-widgets/index.js 26.4 kB 0 B
build/edit-widgets/style-rtl.css 3.1 kB 0 B
build/edit-widgets/style.css 3.1 kB 0 B
build/editor/editor-styles-rtl.css 480 B 0 B
build/editor/editor-styles.css 482 B 0 B
build/editor/index.js 43.1 kB 0 B
build/editor/style-rtl.css 3.85 kB 0 B
build/editor/style.css 3.85 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 735 B 0 B
build/format-library/index.js 7.7 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 623 B 0 B
build/i18n/index.js 3.57 kB 0 B
build/is-shallow-equal/index.js 712 B 0 B
build/keyboard-shortcuts/index.js 2.52 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.11 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.34 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.42 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.43 kB 0 B
build/priority-queue/index.js 791 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/reusable-blocks/index.js 3.06 kB 0 B
build/rich-text/index.js 13.2 kB 0 B
build/server-side-render/index.js 2.77 kB 0 B
build/shortcode/index.js 1.69 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.22 kB 0 B

compressed-size-action

@sirreal sirreal marked this pull request as ready for review October 29, 2020 12:18
@sirreal sirreal added [Block] Cover Affects the Cover Block - used to display content laid over a background image [Package] Block library /packages/block-library [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release labels Oct 29, 2020
@sirreal sirreal self-assigned this Oct 29, 2020
@stokesman
Copy link
Contributor

stokesman commented Oct 29, 2020

Nice @sirreal. I'd tried the same approach and found it workable. The only thing this leaves as questionable is the opacity control now doesn't show for the pseudo default black color.

@jasmussen
Copy link
Contributor

Fair point, but potentially something that global styles can augment.

Comment on lines +47 to +49
&.has-background-dim:not([class*="-background-color"]) {
background-color: $black;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it'd be a (likely imperceptible) performance optimization to do this instead:

:not([class*="-background-color"]) {
	&.wp-block-cover-image,
	&.wp-block-cover {
		background-color: $black;
	}
}

Except that goes as a separate top-level block (not nested). The gist is the slow selector is [class*="-background-color"]) goes on the left of simpler class selectors so that the browser can more quickly determine when the selector doesn't apply. I didn't include in the selector has-background-dim because the cover block always has it. But maybe that wasn't the case for the older wp-block-cover-image so it might be safer to include.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you feel strongly about this? Your suggestion does likely have better performance characteristics but I also suspect they're imperceptible. I'd prefer to keep the rule in the same SASS wrapper block and next to the inherit rules. It's a coherent place and easy to see.

Copy link
Contributor

Choose a reason for hiding this comment

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

No strong feelings on that.

@stokesman
Copy link
Contributor

It's a simple change to restore the older logic for when the opacity control displays.
Remove this line:

const canDim = !! url && ( overlayColor.color || gradientValue );

On this line, change canDim to !! url

@sirreal
Copy link
Member Author

sirreal commented Oct 29, 2020

It's a simple change to restore the older logic for when the opacity control displays …

👍 Thanks, I've pushed a commit with that change. Essentially, this is a rollback of part of #26133, right?

@sirreal
Copy link
Member Author

sirreal commented Oct 29, 2020

On second thought, I've reverted the opacity controls changes.

I'd like to keep this laser focused on shipping a fix for sites that are experiencing issues in production. I'd expect a Gutenberg 9.2 patch release to include this fix.

There is time before the next Gutenberg release to include related fixes or to clean up the opacity controls for WordPress 5.6, but those are beyond the critical regression this seeks to address.

I'd appreciate if folks can review this with that mindset 🙇

@stokesman
Copy link
Contributor

stokesman commented Oct 29, 2020

this is a rollback of part of #26133, right?

Yes, just a rollback. Specifically of ee85a7d. But I like your thinking on keeping the laser focus of this PR.

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

The regression is fixed with this change 👍 I verified covers without explicit overlay color with and without explicit opacities are kept as before.

Although not directed related to this PR, why do we allow opacity when we have an explicit overlay color but not with the default color? It feels strange that a user before changed an opacity and after an update can not change the value that was previously changed and is forced to keep the same value. I can even go to the code editor and change the opacity value and things work. Why remove the UI for that case?

@tellthemachines tellthemachines added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Nov 2, 2020
@sirreal sirreal merged commit 716884d into master Nov 2, 2020
@sirreal sirreal deleted the fix/26545-missing-cover-overlay branch November 2, 2020 09:50
@github-actions github-actions bot added this to the Gutenberg 9.3 milestone Nov 2, 2020
@sirreal sirreal mentioned this pull request Nov 2, 2020
6 tasks
@sirreal
Copy link
Member Author

sirreal commented Nov 2, 2020

I've created #26625 to restore the opacity controls as suggested.

tellthemachines pushed a commit that referenced this pull request Nov 11, 2020
* Restore default Cover overlay background

The default Cover block overlay background was removed. This changed the
style of existing blocks on existing sites.

Restore the default background in such a way that it does not conflict
with theme-provided background-color styles and there is no need to
artificially add extra specificity.

Fix regression: #26545
tellthemachines added a commit that referenced this pull request Nov 12, 2020
* Cover block: Restore default overlay background (#26569)

* Restore default Cover overlay background

The default Cover block overlay background was removed. This changed the
style of existing blocks on existing sites.

Restore the default background in such a way that it does not conflict
with theme-provided background-color styles and there is no need to
artificially add extra specificity.

Fix regression: #26545

* Limit the interface skeleton to a max width of 100% to prevent some blocks managing to exceed the width (#26552)

* Cover Block: Restore opacity controls (#26625)

* Fix image block centering when resizing to a smaller size (#26376)

* Fix image block centering when resizing to a smaller size

* Revert previous 100% width fix

* Remove display: table when image block is resized to avoid centering of block

* Match frontend classes for alignment in editor

* Gallery: Remove caption fallback for alt attribute (#26676)

* Fix quote block default alignment (#26680)

* Gallery: Remove obsolete deprecation entry (#26736)

* Do not apply text color if it is being overriden (#24626)

* Fix: Preset colors don't work on button block style outline (#26707)

* Tests: Add fixture for Column deprecation (#26774)

* Fix/column width units (#26757)

* Fix issues with different units in column widths

* Columns with fixed width should keep width on recalculation

* Address review feedback

* fix undefined index notice in Social Link Block (#25663)

* fix undefined index notice

* use isset instead of array_key_exists check

* Update packages/block-library/src/social-link/index.php

Co-authored-by: Greg Ziółkowski <grzegorz@gziolo.pl>

Co-authored-by: Greg Ziółkowski <grzegorz@gziolo.pl>

* Adds in missing styling for toolbar when using text-only setting (#26769)

* Adds in missing styling for toolbar when using text-only setting

* Increases margin

* Moves style to correct file

* Inserter: Fix 'Browse All' in Quick Inserter (#26443)

* Inserter: Fix 'Browse All' in Quick Inserter

Maintain the insertion point in @wordpress/block-editor state when
moving from the Quick Inserter to the Inserter.

This fixes a bug where the insertion point is wrong after clicking a
block appender and selecting 'Browse All'.

Care is taken to not regress a previous bug where the insertion point is
wrong after clicking an inline insertion button and selecting 'Browse
ALl'.

* Inserter: Fix typo

Co-authored-by: Daniel Richards <daniel.richards@automattic.com>

* Call getBlockInsertionPoint once

* Add useSelect dependencies

* Make setInsertionPoint unstable

* Avoid setting `isVisible` state when `SET_INSERTION_POINT` is triggered

* Make index required and clarify rootClientId usage

* Split insertionPoint into two reducers

* Fix getInsertionPoint selector that expects default state of reducer to be null

* Avoid resetting insertionPoint state on HIDE_INSERTION_POINT

Co-authored-by: Daniel Richards <daniel.richards@automattic.com>

Co-authored-by: Jon Surrell <jon.surrell@automattic.com>
Co-authored-by: Jacopo Tomasone <Copons@users.noreply.github.com>
Co-authored-by: Daniel Richards <daniel.richards@automattic.com>
Co-authored-by: Bernie Reiter <ockham@raz.or.at>
Co-authored-by: Greg Ziółkowski <grzegorz@gziolo.pl>
Co-authored-by: Oliver Juhas <webmandesign@users.noreply.github.com>
Co-authored-by: Jorge Costa <jorge.costa@automattic.com>
Co-authored-by: Fabian Kägy <mail@fabian-kaegy.de>
Co-authored-by: Tammie Lister <tammie@automattic.com>
Co-authored-by: Robert Anderson <robert@noisysocks.com>
@tellthemachines tellthemachines removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Nov 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Cover Affects the Cover Block - used to display content laid over a background image [Package] Block library /packages/block-library [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cover Block: Overlay background possible regression
5 participants