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.

Sign up for GitHub

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

Use data-hero-candidate attribute in DetermineHeroImages #5934

Merged
merged 25 commits into from
Mar 19, 2021
Merged

Use data-hero-candidate attribute in DetermineHeroImages #5934

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
8b84aaa
Switch to data-hero-candidate branch for amp-toolbox
schlessera Feb 27, 2021
d727a36
Use data-hero-candidate attribute in DetermineHeroImages transformer
schlessera Feb 27, 2021
77d5f99
Refactor transformer to get rid of code duplication
schlessera Feb 27, 2021
c490443
Adapt tests
schlessera Feb 27, 2021
48b9524
Update Composer lock file - 2020-03-08
schlessera Mar 8, 2021
b126bc3
Update amp-toolbox-php dependency to v0.2.0
schlessera Mar 18, 2021
b685b0e
Use switch instead of variable methods
schlessera Mar 18, 2021
7d96d8a
Improve XPath to target custom header images
schlessera Mar 18, 2021
cdaa8c4
Use new Encoding::AMP constant
schlessera Mar 18, 2021
223b008
Ignore PHPStan error for PHP7+ isBuiltIn() method
schlessera Mar 18, 2021
3d6ff8b
Merge branch 'fix/5824-use-data-hero-candidate-attribute' of https://…
schlessera Mar 18, 2021
e13d93d
Remove DetermineHeroImages from default transformers
schlessera Mar 18, 2021
1d99a1e
Revert "Ignore PHPStan error for PHP7+ isBuiltIn() method"
schlessera Mar 18, 2021
02d1b90
Use centralized default options for Dom\Document
schlessera Mar 18, 2021
68b961c
Keep matching the custom header class
schlessera Mar 18, 2021
025a6f0
Change the way the amp carousel test is set up
schlessera Mar 18, 2021
b5e45f1
Fix assignment alignment for phpcs; add phpdoc type for static analysis
westonruter Mar 19, 2021
cadc98f
Fix detecting hero image candidate in Cover Block
westonruter Mar 19, 2021
2cac676
Only consider cover block in initial position of first entry content
westonruter Mar 19, 2021
834cca5
Test detection of cover block inside initial group block
westonruter Mar 19, 2021
e42f1b3
Add prerendering for image blocks in initial position if first entry …
westonruter Mar 19, 2021
8513aba
Enable DetermineHeroImages transformer by default in core themes
westonruter Mar 19, 2021
25ec074
Remove obsolete logic for candidates as array
westonruter Mar 19, 2021
65b0e05
Add covers annotations for DetermineHeroImages tests
schlessera Mar 19, 2021
bcb50e5
Use constant for config key
schlessera Mar 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
"ext-json": "*",
"ext-libxml": "*",
"ext-spl": "*",
"ampproject/amp-toolbox": "^0.1",
"ampproject/amp-toolbox": "^0.2",
"cweagans/composer-patches": "1.7.0",
"fasterimage/fasterimage": "1.5.0",
"sabberworm/php-css-parser": "dev-master#bfdd976"
Expand Down
138 changes: 70 additions & 68 deletions composer.lock
View file
Open in desktop

Large diffs are not rendered by default.

5 changes: 2 additions & 3 deletions includes/class-amp-theme-support.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
*/

use AmpProject\Amp;
use AmpProject\AmpWP\Dom\Options;
use AmpProject\AmpWP\ExtraThemeAndPluginHeaders;
use AmpProject\AmpWP\Option;
use AmpProject\AmpWP\QueryVar;
Expand Down Expand Up @@ -1867,7 +1868,7 @@ public static function prepare_response( $response, $args = [] ) {
*/
do_action( 'amp_server_timing_start', 'amp_dom_parse', '', [], true );

$dom = Document::fromHtml( $response );
$dom = Document::fromHtml( $response, Options::DEFAULTS );

if ( AMP_Validation_Manager::$is_validate_request ) {
AMP_Validation_Manager::remove_illegal_source_stack_comments( $dom );
Expand Down Expand Up @@ -2117,8 +2118,6 @@ private static function get_optimizer_configuration( $args ) {
Optimizer\Transformer\TransformedIdentifier::class,
]
);
} else {
array_unshift( $transformers, Transformer\DetermineHeroImages::class );
}

array_unshift( $transformers, Transformer\AmpSchemaOrgMetadata::class );
Expand Down
2 changes: 1 addition & 1 deletion includes/sanitizers/class-amp-meta-sanitizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,7 +295,7 @@ protected function create_charset_element() {
$this->dom,
Tag::META,
[
Attribute::CHARSET => Document::AMP_ENCODING,
Attribute::CHARSET => Document\Encoding::AMP,
]
);
}
Expand Down
20 changes: 16 additions & 4 deletions includes/utils/class-amp-dom-utils.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@
* @package AMP
*/

use AmpProject\AmpWP\Dom\Options;
use AmpProject\Dom\Document;
use AmpProject\Dom\Element;
use AmpProject\Tag;

/**
Expand Down Expand Up @@ -65,7 +67,14 @@ class AMP_DOM_Utils {
*/
public static function get_dom( $document, $encoding = null ) {
_deprecated_function( __METHOD__, '1.5.0', 'AmpProject\Dom\Document::fromHtml()' );
return Document::fromHtml( $document, $encoding );

$options = Options::DEFAULTS;

if ( null !== $encoding ) {
$options[ Document\Option::ENCODING ] = $encoding;
}

return Document::fromHtml( $document, $options );
}

/**
Expand Down Expand Up @@ -172,7 +181,10 @@ public static function get_dom_from_content( $content, $encoding = null ) {
*/
$document = "<html><head></head><body>{$content}</body></html>";

return Document::fromHtml( $document, $encoding );
$options = Options::DEFAULTS;
$options[ Document\Option::ENCODING ] = $encoding;

return Document::fromHtml( $document, $options );
}

/**
Expand Down Expand Up @@ -375,8 +387,8 @@ public static function has_class( DOMElement $element, $class ) {
*
* @deprecated Use AmpProject\Dom\Document::getElementId() instead.
*
* @param DOMElement $element Element to get the ID for.
* @param string $prefix Optional. Defaults to 'amp-wp-id'.
* @param DOMElement|Element $element Element to get the ID for.
Copy link
Member

Choose a reason for hiding this comment

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

Rather than having to do this for static analysis, the getElementId method could indicate the $element is a DOMElement or an Element.

* @param string $prefix Optional. Defaults to 'amp-wp-id'.
* @return string ID to use.
*/
public static function get_element_id( $element, $prefix = 'amp-wp-id' ) {
Expand Down
22 changes: 22 additions & 0 deletions src/Dom/Options.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
<?php
/**
* Class Options.
*
* @package AmpProject\AmpWP
*/

namespace AmpProject\AmpWP\Dom;

use AmpProject\Dom\Document;

interface Options {

/**
* Default options to use for the Dom.
*
* @var array
*/
const DEFAULTS = [
Document\Option::AMP_BIND_SYNTAX => Document\Option::AMP_BIND_SYNTAX_DATA_ATTRIBUTE,
];
}
90 changes: 53 additions & 37 deletions src/Transformer/DetermineHeroImages.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,25 +31,25 @@
final class DetermineHeroImages implements Transformer {

/**
* XPath query to find the existing hero images (elements with data/hero attribute).
* XPath query to find the custom logo.
*
* @var string
*/
const EXISTING_HERO_IMAGES_XPATH_QUERY = './/*[@data-hero]';
const CUSTOM_HEADER_XPATH_QUERY = ".//*[ @id = 'wp-custom-header' or @id = 'masthead' or @id = 'site-header' or contains( concat( ' ', normalize-space( @class ), ' ' ), ' wp-custom-header ' ) ]//*[ ( self::img or self::amp-img ) and not( @data-hero ) and not( contains( concat( ' ', normalize-space( @class ), ' ' ), ' custom-logo ' ) ) ]";

/**
* XPath query to find the custom logo.
*
* @var string
*/
const CUSTOM_LOGO_XPATH_QUERY = ".//a[ contains( concat( ' ', normalize-space( @class ), ' ' ), ' custom-logo-link ' ) ]//*[ contains( concat( ' ', normalize-space( @class ), ' ' ), ' custom-logo ' ) ][ not( @data-hero ) ]";
const CUSTOM_LOGO_XPATH_QUERY = ".//a[ contains( concat( ' ', normalize-space( @class ), ' ' ), ' custom-logo-link ' ) ]//*[ ( self::img or self::amp-img ) and contains( concat( ' ', normalize-space( @class ), ' ' ), ' custom-logo ' ) ][ not( @data-hero ) ]";

/**
* XPath query to find the featured image.
*
* @var string
*/
const FEATURED_IMAGE_XPATH_QUERY = ".//*[ contains( concat( ' ', normalize-space( @class ), ' ' ), ' wp-post-image ' ) ][ not( @data-hero ) ]";
const FEATURED_IMAGE_XPATH_QUERY = ".//*[ ( self::img or self::amp-img ) and contains( concat( ' ', normalize-space( @class ), ' ' ), ' wp-post-image ' ) ][ not( @data-hero ) ]";

/**
* XPath query to find the cover blocks.
Expand All @@ -68,44 +68,60 @@ final class DetermineHeroImages implements Transformer {
* @return void
*/
public function transform( Document $document, ErrorCollection $errors ) { // phpcs:ignore VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable
$existing_hero_images_count = $document->xpath->query(
self::EXISTING_HERO_IMAGES_XPATH_QUERY,
$document->body
)->length;

$available_hero_image_slots = max(
PreloadHeroImage::DATA_HERO_MAX - $existing_hero_images_count,
0
);

$hero_image_elements = [];

if ( count( $hero_image_elements ) < $available_hero_image_slots ) {
$custom_logo = $this->get_custom_logo( $document );
if ( null !== $custom_logo ) {
$hero_image_elements[] = $custom_logo;
foreach ( [ 'custom_header', 'custom_logo', 'featured_image', 'cover_blocks' ] as $hero_image_source ) {
if ( count( $hero_image_elements ) < PreloadHeroImage::DATA_HERO_MAX ) {
$candidates = [];

switch ( $hero_image_source ) {
case 'custom_header':
$candidates = $this->get_custom_header( $document );
break;
case 'custom_logo':
$candidates = $this->get_custom_logo( $document );
break;
case 'featured_image':
$candidates = $this->get_featured_image( $document );
break;
case 'cover_blocks':
$candidates = $this->get_cover_blocks( $document );
break;
}

if ( empty( $candidates ) ) {
continue;
}

if ( is_array( $candidates ) ) {
$hero_image_elements = array_merge( $hero_image_elements, array_filter( $candidates ) );
} else {
$hero_image_elements[] = $candidates;
}
}
}

if ( count( $hero_image_elements ) < $available_hero_image_slots ) {
$featured_image = $this->get_featured_image( $document );
if ( null !== $featured_image ) {
$hero_image_elements[] = $featured_image;
}
}

if ( count( $hero_image_elements ) < $available_hero_image_slots ) {
$hero_image_elements = array_merge(
$hero_image_elements,
array_filter(
$this->get_cover_blocks( $document )
)
);
}
$this->add_data_hero_candidate_attribute(
array_slice( $hero_image_elements, 0, PreloadHeroImage::DATA_HERO_MAX )
);
}

$this->add_data_hero_attribute(
array_slice( $hero_image_elements, 0, $available_hero_image_slots )
/**
* Retrieve the element that represents the custom header.
*
* @param Document $document Document to retrieve the custom header from.
* @return DOMElement|null Element that represents the custom header, or null
* if not found.
*/
private function get_custom_header( Document $document ) {
$elements = $document->xpath->query(
self::CUSTOM_HEADER_XPATH_QUERY,
$document->body
);
Comment on lines +130 to 133
Copy link
Member

Choose a reason for hiding this comment

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

An alternative way to identify the header image which may be more robust as opposed to relying on element IDs is to obtain the header image URL and then query for image elements that have that src:

Suggested change
$elements = $document->xpath->query(
self::CUSTOM_HEADER_XPATH_QUERY,
$document->body
);
$header_image = get_header_image();
if ( ! $header_image ) {
return null;
}
$elements = $document->xpath->query(
sprintf(
'//*[ ( self::img or self::amp-img ) and @src = "%s" ]',
addcslashes( $custom_header->url, '"\\' )
),
$document->body
);

But this is going to have other problems, namely because get_header_image_tag() allows for the markup to be filtered, meaning the image URL could be replaced with one that is pointing to a CDN.


$custom_header = $elements->item( 0 );

return $custom_header instanceof DOMElement ? $custom_header : null;
}

/**
Expand Down Expand Up @@ -165,9 +181,9 @@ private function get_cover_blocks( Document $document ) {
* @param DOMElement[] $hero_image_elements Elements that are viable hero
* images.
*/
private function add_data_hero_attribute( $hero_image_elements ) {
private function add_data_hero_candidate_attribute( $hero_image_elements ) {
foreach ( $hero_image_elements as $hero_image_element ) {
$hero_image_element->setAttribute( Attribute::DATA_HERO, null );
$hero_image_element->setAttribute( Attribute::DATA_HERO_CANDIDATE, null );
}
}
}
Loading