Skip to content

Commit

Permalink
Fix display of Custom Logo in Twenty Twenty
Browse files Browse the repository at this point in the history
  • Loading branch information
westonruter committed Mar 23, 2020
1 parent 4b58751 commit d59b24a
Show file tree
Hide file tree
Showing 2 changed files with 84 additions and 2 deletions.
60 changes: 58 additions & 2 deletions includes/sanitizers/class-amp-core-theme-sanitizer.php
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ protected static function get_theme_features_config( $theme_slug ) {
'add_nav_menu_styles' => [],
'add_twentytwenty_masthead_styles' => [],
'add_img_display_block_fix' => [],
'add_twentytwenty_custom_logo_fix' => [],
'add_twentytwenty_current_page_awareness' => [],
];

Expand Down Expand Up @@ -720,6 +721,58 @@ static function() {
);
}

/**
* Fix display of Custom Logo in Twenty Twenty.
*
* This is required because width:auto on the site-logo amp-img does not preserve the proportional width in the same
* way as the same styles applied to an img.
*
* @since 1.5
* @link https://github.com/ampproject/amp-wp/issues/4418
* @link https://codepen.io/westonruter/pen/rNVqadv
*/
public static function add_twentytwenty_custom_logo_fix() {
$method = __METHOD__;
add_filter(
'get_custom_logo',
static function( $html ) use ( $method ) {

This comment has been minimized.

Copy link
@schlessera

schlessera Mar 24, 2020

Collaborator

I don't think you need to pass $method in as a bound variable here.

__METHOD__ should be a compile-time constant, so it should just work even within the callback.

This comment has been minimized.

Copy link
@westonruter

westonruter Mar 24, 2020

Author Member

It was getting output as {closure}.

This comment has been minimized.

Copy link
@westonruter

westonruter Mar 24, 2020

Author Member

This comment has been minimized.

Copy link
@schlessera

schlessera Mar 24, 2020

Collaborator

Oh, interesting. So the closure is being considered for __METHOD__. TIL!

This comment has been minimized.

Copy link
@schlessera

schlessera Mar 24, 2020

Collaborator

Unrelated, but just noticed now: The whitespace is off here:
function( $html ) should actually be function ($html).
function is not the name of the function that is followed by its arguments. For a normal function declaration, it would be function <name>( <arguments ). As we have an anonymous function here, function is still just the normal keyword, and in this case the name is empty.

This comment has been minimized.

Copy link
@westonruter

westonruter Mar 24, 2020

Author Member

Why isn't this being enforced by phpcs?

This comment has been minimized.

Copy link
@schlessera

schlessera Mar 24, 2020

Collaborator

Good question. I can look into that...

// Pattern sourced from AMP_Base_Embed_Handler::match_element_attributes().
$pattern = sprintf(
'/<img%s/',
implode(
'',
array_map(
function ( $attr_name ) {
return sprintf( '(?=[^>]*?%1$s="(?P<%1$s>\d+)")?', preg_quote( $attr_name, '/' ) );
},
[ 'width', 'height' ]
)
)
);
if ( preg_match( $pattern, $html, $matches ) && isset( $matches['width'] ) && isset( $matches['height'] ) ) {
$width = (int) $matches['width'];
$height = (int) $matches['height'];

$desktop_height = 9; // in rem; see <https://github.com/WordPress/wordpress-develop/blob/ad8d01a7e9e13144d1676b8e6d70c3e81ef703af/src/wp-content/themes/twentytwenty/style.css#L4887>.
$mobile_height = 6; // in rem; see <https://github.com/WordPress/wordpress-develop/blob/ad8d01a7e9e13144d1676b8e6d70c3e81ef703af/src/wp-content/themes/twentytwenty/style.css#L1424>.

$desktop_width = $desktop_height * ( $width / $height );
$mobile_width = $mobile_height * ( $width / $height );

$html .= sprintf(
'<style data-src="%s">.site-logo amp-img { width: %frem; } @media (min-width: 700px) { .site-logo amp-img { width: %frem; } }</style>',
esc_attr( $method ),
$mobile_width,
$desktop_width
);

}
return $html;
},
PHP_INT_MAX
);
}

/**
* Add style rule with a selector of higher specificity than just `img` to make `amp-img` have `display:block` rather than `display:inline-block`.
*
Expand All @@ -728,15 +781,18 @@ static function() {
* resulting in larger margins in AMP than expected.
*
* @since 1.5
* @link https://github.com/ampproject/amp-wp/issues/4419
*/
public static function add_img_display_block_fix() {
$method = __METHOD__;
// Note that wp_add_inline_style() is not used because this stylesheet needs to be added _before_ style.css so
// that any subsequent style rules for images will continue to override.
add_action(
'wp_print_styles',
static function() {
static function() use ( $method ) {
printf(
'<style data-src="AMP_Core_Theme_Sanitizer::add_img_display_block_fix">%s</style>',
'<style data-src="%s">%s</style>',
esc_attr( $method ),
// The selector is targeting an attribute that can never appear. It is purely present to increase specificity.
'amp-img:not([_]) { display: block }'
);
Expand Down
26 changes: 26 additions & 0 deletions tests/php/test-class-amp-core-theme-sanitizer.php
Original file line number Diff line number Diff line change
Expand Up @@ -251,4 +251,30 @@ public function test_add_img_display_block_fix() {
$output = ob_get_clean();
$this->assertRegExp( '/amp-img.+display.+block/s', $output );
}

/**
* Tests add_twentytwenty_custom_logo_fix.
*
* @covers AMP_Core_Theme_Sanitizer::add_twentytwenty_custom_logo_fix()
*/
public function test_add_twentytwenty_custom_logo_fix() {
add_filter(
'get_custom_logo',
static function () {
return '<img src="https://example.com/logo.jpg" width="100" height="200">';
}
);

AMP_Core_Theme_Sanitizer::add_twentytwenty_custom_logo_fix();
$logo = get_custom_logo();

$needle = '.site-logo amp-img { width: 3.000000rem; } @media (min-width: 700px) { .site-logo amp-img { width: 4.500000rem; } }';

// @todo Make use of AssertContainsCompatibility trait.
if ( method_exists( $this, 'assertStringContainsString' ) ) {
$this->assertStringContainsString( $needle, $logo );
} else {
$this->assertContains( $needle, $logo );
}
}
}

0 comments on commit d59b24a

Please sign in to comment.