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

Invisible cover block background colors #7471

Closed
akmetin opened this issue Feb 23, 2023 · 14 comments · Fixed by #7481
Closed

Invisible cover block background colors #7471

akmetin opened this issue Feb 23, 2023 · 14 comments · Fixed by #7481
Labels
Changelogged Whether the issue/PR has been added to release notes.
Milestone

Comments

@akmetin
Copy link

akmetin commented Feb 23, 2023

Bug Description

Cover block background colors do not work as expected.

Expected Behaviour

Should render as in the editor.

Screenshots

Screenshot at Feb 23 17-37-57

Screenshot at Feb 23 17-37-57

PHP Version

8.1

Plugin Version

2.4.0

AMP plugin template mode

Reader

WordPress Version

6.1.1

Site Health

No issues

Gutenberg Version

No response

OS(s) Affected

No response

Browser(s) Affected

No response

Device(s) Affected

No response

Acceptance Criteria

No response

Implementation Brief

No response

QA Testing Instructions

No response

Demo

No response

Changelog Entry

No response

@westonruter
Copy link
Member

@deepnees what primary (non-AMP) theme are you using?

We did seek to address this previously in #6039.

@thelovekesh
Copy link
Collaborator

I think a block-based theme is being used here.

Blocks-based themes rely on theme.json and our logic for determining styles, and the color palette is based on theme_support

foreach ( array_keys( self::SUPPORTED_FEATURES ) as $feature_key ) {
$feature_value = current( (array) get_theme_support( $feature_key ) );
if ( ! is_array( $feature_value ) || empty( $feature_value ) ) {
continue;
}

Also, check this note. According to this we will need to get editor-color-palette from theme.json's color palette.

@milindmore22
Copy link
Collaborator

Yes, I tested it with TT1 theme
https://amp-support.rt.gw/2023/03/02/cover-background-color-test/?amp=1

@westonruter
Copy link
Member

Looks like the way to get the theme.json data is via wp_get_global_settings( [], 'theme' ).

@westonruter
Copy link
Member

And more specifically: wp_get_global_settings( [ 'color', 'palette' ], 'theme' )

@thelovekesh
Copy link
Collaborator

@westonruter Should we take into account the fluid font sizes that WordPress 6.1 offers?

@westonruter
Copy link
Member

I'm not very familiar with them. If not taken into account, would the AMP version seem broken?

@thelovekesh
Copy link
Collaborator

Fluid font sizes are all about using clamp() and more of a responsive utility. So, it will not break AMP in any way.

It is being used in theme.json like:

"fontSizes": [
	{
		"fluid": {
			"min": "0.875rem",
			"max": "1rem"
		},
		"size": "1rem",
		"slug": "small"
	},
]

and being outputted as:

--wp--preset--font-size--small: clamp(0.875rem, 0.875rem + ((1vw - 0.48rem) * 0.24), 1rem);

If fluid typography is not used, then it will use font size from default/theme presets:

--wp--preset--font-size--small: 13px;

@westonruter
Copy link
Member

Got it. At the moment, these will be broken entirely due to php-css-parser failing to parse such complex CSS values. This was just raised in a support topic which we had already reported in #7291. I'm going to re-open the issue and think about a workaround.

@westonruter
Copy link
Member

But yes, we should preserve fluid font sizes from the main theme when in Reader mode, same as we try to preserve the color palette. But the color palette is more important since lack of support can mean the text is impossible to read due to being white-on-white or black-on-black.

@pavanpatil1
Copy link

Cross-checked the issue and the fix is working fine. background-color is now visible and text is also visible properly in the reader mode. However, in the cover block, the background color for the paragraph inside the cover is visible outside the container.

image

@thelovekesh
Copy link
Collaborator

#7496 aims to fix the above issue. It happened because of p.has-background CSS which was implemented for every p.has-background selector instead of .amp-wp-article-content > p.has-background.

@pavanpatil1
Copy link

QA Passed ✅

Verified the fix. It is working as expected. The block style is visible properly.

image

@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Mar 22, 2023
@westonruter
Copy link
Member

@deepnees This is now released on the WordPress.org Plugin Directory.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelogged Whether the issue/PR has been added to release notes.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants