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

Incorrect assumption of hex input by ReaderThemeSupportFeatures::get_relative_luminance_from_hex() #7285

Closed
westonruter opened this issue Oct 4, 2022 · 1 comment · Fixed by #7286
Labels
Bug Something isn't working Changelogged Whether the issue/PR has been added to release notes.
Milestone

Comments

@westonruter
Copy link
Member

Bug Description

See support forum topic.

The ReaderThemeSupportFeatures::get_relative_luminance_from_hex() method incorrectly assumes hex input:

public function get_relative_luminance_from_hex( $hex ) {
// Remove the "#" symbol from the beginning of the color.
$hex = ltrim( $hex, '#' );
// Make sure there are 6 digits for the below calculations.
if ( 3 === strlen( $hex ) ) {
$hex = $hex[0] . $hex[0] . $hex[1] . $hex[1] . $hex[2] . $hex[2];
}
// Get red, green, blue.
$red = hexdec( substr( $hex, 0, 2 ) );
$green = hexdec( substr( $hex, 2, 2 ) );
$blue = hexdec( substr( $hex, 4, 2 ) );
// Calculate the luminance.
$lum = ( 0.2126 * $red ) + ( 0.7152 * $green ) + ( 0.0722 * $blue );
return (int) round( $lum );
}
}

It turns out that in some cases, a theme may be passing a CSS variable like var(--base-2), or presumably rgba(...) could also be passed.

To account for this, the print_editor_color_palette_styles method should skip passing the color value into get_relative_luminance_from_hex when it is not a valid hex.

This could be achieved by adding:

if ( ! preg_match( '/^[0-9a-f]{3}[0-9a-f]{3}?$/i', ltrim( $color_option[ self::KEY_COLOR ], '#' ) ) ) {
    continue;
}

After the following condition (or as part of it):

private function print_editor_color_palette_styles( array $color_palette ) {
echo '<style id="amp-wp-theme-support-editor-color-palette">';
foreach ( $color_palette as $color_option ) {
if ( ! $this->has_required_feature_props( self::FEATURE_EDITOR_COLOR_PALETTE, $color_option ) ) {
continue;
}

Expected Behaviour

If a non-hex color is used, no deprecated error should be raised from hexdec() here:

$red = hexdec( substr( $hex, 0, 2 ) );
$green = hexdec( substr( $hex, 2, 2 ) );
$blue = hexdec( substr( $hex, 4, 2 ) );

Screenshots

No response

PHP Version

No response

Plugin Version

2.3.0

AMP plugin template mode

Reader

WordPress Version

No response

Site Health

No response

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

@pavanpatil1
Copy link

QA Passed ✅

Cross-checked the issue with the below snippet and the fix is working as expected.

add_action(
	'after_setup_theme',
	static function() {
		add_theme_support( 'editor-color-palette', array(
			array(
				'name'  => 'bar',
				'slug'  => 'bar',
				'color' => 'var(--bar)',
			)
		) );
	}
);

Before:

debuglog.mp4

After fix:

debuglog1.mp4

@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Changelogged Whether the issue/PR has been added to release notes.
Projects
None yet
3 participants