Improve hero image determination #6062

westonruter · 2021-04-09T21:30:42Z

Summary

Fixes #6061

Enable DetermineHeroImages transformer by default, instead of just for the core themes.
Introduce HeroCandidateFiltering service which uses WP filters to automatically add data-hero-candidate attribute to images output via get_custom_logo(), get_header_image_tag(), and the wp_get_attachment_image() when the attachment image is the featured image of the queried object or first the post in the loop.
Remove featured image and (mostly) custom logo handling from DetermineHeroImages since better handled now by HeroCandidateFiltering.
Update DetermineHeroImages to identify all images before main or the first .entry-content as being header images, but omit images which were originally marked as loading=lazy. Include even tiny images which have logo in the class name.
Extend prerendering of first-content blocks to include images in blocks other than Cover block and Image block (e.g. YouTube embed placeholders).

Depends on:

Checklist

My code is tested and passes existing tests.
My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

codecov · 2021-04-10T04:10:05Z

Codecov Report

Merging #6062 (be3dba3) into develop (905ca69) will increase coverage by 0.08%.
The diff coverage is 97.10%.

❗ Current head be3dba3 differs from pull request most recent head 67a2f91. Consider uploading reports for the commit 67a2f91 to get more accurate results

@@              Coverage Diff              @@
##             develop    #6062      +/-   ##
=============================================
+ Coverage      74.81%   74.89%   +0.08%     
- Complexity      5771     5782      +11     
=============================================
  Files            230      231       +1     
  Lines          17492    17505      +13     
=============================================
+ Hits           13087    13111      +24     
+ Misses          4405     4394      -11

Flag	Coverage Δ	Complexity Δ
javascript	`79.84% <ø> (ø)`	`0.00 <ø> (ø)`
php	`74.67% <97.10%> (+0.08%)`	`5782.00 <25.00> (+11.00)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ	Complexity Δ
...udes/sanitizers/class-amp-core-theme-sanitizer.php	`36.54% <0.00%> (+0.26%)`	`227.00 <0.00> (-1.00)`	⬆️
src/AmpWpPlugin.php	`100.00% <ø> (ø)`	`9.00 <0.00> (ø)`
src/Optimizer/Transformer/DetermineHeroImages.php	`98.07% <97.05%> (+7.03%)`	`24.00 <12.00> (-1.00)`	⬆️
src/Optimizer/AmpWPConfiguration.php	`100.00% <100.00%> (ø)`	`5.00 <0.00> (ø)`
src/Optimizer/HeroCandidateFiltering.php	`100.00% <100.00%> (ø)`	`13.00 <13.00> (?)`
src/Infrastructure/ServiceBasedPlugin.php	`86.20% <0.00%> (+0.68%)`	`56.00% <0.00%> (ø%)`

westonruter · 2021-04-10T05:49:10Z

src/Transformer/DetermineHeroImages.php

@@ -109,50 +100,64 @@ public function transform( Document $document, ErrorCollection $errors ) { // ph
 				}

 				if ( $candidate instanceof DOMElement ) {
-					$hero_image_elements[] = $candidate;
+					$hero_image_elements[ spl_object_hash( $candidate ) ] = $candidate;


The use of spl_object_hash() allows for the removal of duplicates.

src/Transformer/DetermineHeroImages.php

…de hero candidate

westonruter · 2021-04-11T20:00:50Z

src/Transformer/DetermineHeroImages.php

 					case 'featured_image':
 						$candidate = $this->get_featured_image( $document );
 						break;


There's an issue here with the featured image. Namely, if there is a template with multiple posts on it, and the first post lacks a featured image but the second post has one, then this featured image will get prerendered:

This issue is mitigated by preserving the loading=lazy on the original img as done in ampproject/amp-toolbox-php#149 but it is not ideal. Consider this worse case where there is a content image in the first post, but a featured image in the second. The featured image from the second post will win out over the content image in the first:

See also an e-commerce example (apparently using Astra and WooCommerce) in which the hero image is not marked up as a featured image, nor is it inside of .entry-content. Rather, it has a selector main > div.ast-woocommerce-container > div.product-section > div > div > div:nth-child(1) > div > amp-img.

This makes me think that we should perhaps eliminate the featured image check, in favor of getting the first images from the main element as hero candidates, or if no main then the first .hentry.

westonruter · 2021-04-12T19:49:47Z

Importantly, there is now a maxHeroImageCount parameter in the OptimizeHeroImages to override the default max of 2: ampproject/amphtml#30442 (comment). This depends on ampproject/amp-toolbox-php#55.

In a WordPress context, I think we should go ahead and SSR every image that has loading=lazy if that attribute is persisted on the amp-img[i-amphtml-ssr]>img.

We could limit SSR of hero images to 2 if they lack loading=lazy.

westonruter · 2021-04-13T05:47:14Z

I think we need to augment the transformer with regular WordPress hooks. Namely, we should add data-hero-candidate via PHP filters for header image, custom logo, and the featured image of the queried object and/or first post in the loop. This will be a more reliable way of detecting these hero images by having insider knowledge from WordPress. This would run in addition to the logic currently in DetermineHeroImages, so we'd identify images using filters and then anything that slipped past the filters could be identified with XPath queries.

…image-candidate-determination * 'develop' of github.com:ampproject/amp-wp: (31 commits) Update snapshots and fix tests Remove references to AMP 'page' since it may be post; harmonize strings Use boolean return type for isDevToolsEnabled selector Add missing name prop for PluginDocumentSettingPanel in AMPDocumentSettingPanel Show AMP Validation sidebar button when all issues have been reviewed Omit AMPToolbarButton when AMP is not enabled Make entry path regex match simpler Show AMP toggle when Dev Tools are disabled Remove redundant build:css script Improve JS test coverage Ignore block editor plugins from test coverage Ignore block editor plugins in unit test Add more unit tests Fix broken E2E test Do not generate empty JS chunks for stylesheets Omit DependencyExtractionWebpackPlugin for CSS Add `build:css` back so that CI continues to work Move the entrypoint config so that it is in alphabetical order Handle CSS build process with Webpack Add unit tests to AMPDocumentStatusNotification ...

…image-candidate-determination * 'develop' of github.com:ampproject/amp-wp: (27 commits) Remove unused $matches Avoid failing validation requests when response is redirect Undo erroneous addition of is_callable() check in d0d2137 Improve static analysis and formatting Catch UnknownConfigurationClass exception when transformer has no config Avoid alignment using tabs Remove double blank lines Fix test failure caused by new amp-onerror script Fix code style issue Revert sorting the services array Add transformer config command Add transformer list command Fix code style issues Use injected optimizer service Complete docblock Add optimizer command Refactor registration of CLI commands to be integrated with the service container Refactor validation command into PSR-4 service-based architecture Test whether optimizer gets caching and fallback downloads Add AmpWPConfiguration test ...

…php#154

…ters

…mage a hero candidate

github-actions · 2021-04-15T00:04:18Z

Plugin builds for 67a2f91 are ready 🛎️!

Download development build
Download production build

.phpstorm.meta.php

pierlon · 2021-04-15T01:33:10Z

includes/sanitizers/class-amp-core-theme-sanitizer.php

 			// Twenty Ten.
 			case 'twentyten':
-				return [
-					'enable_determine_hero_images_transformer' => [],
-				];
+				return [];


I suppose the twentyten case can be removed then?

I tried removing but then I got warnings:

[15-Apr-2021 03:16:16 UTC] PHP Warning: array_merge(): Expected parameter 2 to be an array, null given in /app/public/content/plugins/amp/includes/sanitizers/class-amp-core-theme-sanitizer.php on line 495 [15-Apr-2021 03:16:16 UTC] PHP Warning: Invalid argument supplied for foreach() in /app/public/content/plugins/amp/includes/sanitizers/class-amp-core-theme-sanitizer.php on line 499

So it's currently needed, due to how that array is used in the get_theme_features method.

src/Optimizer/AmpWPConfiguration.php

src/Optimizer/HeroCandidateFiltering.php

Co-authored-by: Pierre Gordon <16200219+pierlon@users.noreply.github.com>

src/Optimizer/HeroCandidateFiltering.php

pierlon · 2021-04-15T04:44:01Z

tests/php/src/Optimizer/HeroCandidateFilteringTest.php

+	}
+
+	/** @covers ::add_data_hero_candidate_attribute()  */
+	public function add_data_hero_candidate_attribute() {


This test was not being executed.

Suggested change

public function add_data_hero_candidate_attribute() {

public function test_add_data_hero_candidate_attribute() {

Oops! Committed your suggestion in this commit: 67a2f91

pierlon · 2021-04-15T04:44:09Z

tests/php/src/Optimizer/HeroCandidateFilteringTest.php

+	/** @covers ::add_data_hero_candidate_attribute()  */
+	public function add_data_hero_candidate_attribute() {
+		$this->assertEquals(
+			$array_with_attr,


$array_with_attr is undefined here.

😊 Fixed in 67a2f91

Co-authored-by: Pierre Gordon <16200219+pierlon@users.noreply.github.com>

pierlon

Quite an impressive feat achieved here, LGTM!

schlessera · 2021-04-15T08:11:54Z

src/Optimizer/HeroCandidateFiltering.php

+	 * @return bool Whether the conditional object is needed.
+	 */
+	public static function is_needed() {
+		return amp_is_request();


I'll have to pay attention to this one. It means we need to ensure in the CLI context that we need to ensure this evaluates to true, otherwise these filters will be missing.

Good point. It will always be false in a WP-CLI context currently:

amp-wp/includes/amp-helper-functions.php

Lines 327 to 329 in 1e7ea02

// Short-circuit for cron, CLI, admin requests or requests to non-frontend pages.

if ( wp_doing_cron() || ( defined( 'WP_CLI' ) && WP_CLI ) || is_admin() || in_array( $pagenow, [ 'wp-login.php', 'wp-signup.php', 'wp-activate.php', 'repair.php' ], true ) ) {

return false;

westonruter added this to the v2.1 milestone Apr 9, 2021

westonruter commented Apr 10, 2021

View reviewed changes

westonruter requested a review from schlessera April 10, 2021 05:49

westonruter commented Apr 10, 2021

View reviewed changes

src/Transformer/DetermineHeroImages.php Outdated Show resolved Hide resolved

westonruter force-pushed the fix/hero-image-candidate-determination branch from 731b690 to e8ea9c0 Compare April 11, 2021 05:44

westonruter added 7 commits April 10, 2021 22:54

Include DetermineHeroImages on non-core themes as well

2e40142

Improve identification of images in header

0521582

Gather custom logo along with other header images

9a65ecc

Update tests for DetermineHeroImages

f9bff05

Expand prerendering to any initial block containing an image

0549326

Use Element instead of DOMElement

f27dab7

Add test to ensure tiny image at beginning of entry-content is not ma…

0ce975a

…de hero candidate

westonruter force-pushed the fix/hero-image-candidate-determination branch from e8ea9c0 to cb443cc Compare April 11, 2021 05:58

Update amp-toolbox-php to ampproject/amp-toolbox-php#149

5100cd4

westonruter force-pushed the fix/hero-image-candidate-determination branch from cb443cc to 5100cd4 Compare April 11, 2021 06:46

westonruter commented Apr 11, 2021

View reviewed changes

westonruter mentioned this pull request Apr 14, 2021

Reduce impact of third party code ampproject/amphtml#33405

Closed

westonruter added 7 commits April 13, 2021 22:53

Update amp-toolbox-php to 0.4.0

23f1d7b

Update amp-toolbox-php to dev-main to include ampproject/amp-toolbox-…

08d2e83

…php#154

Add HeroCandidateFiltering service for injecting attribute via WP fil…

65794bc

…ters

Clarify that HeroCandidateFiltering only makes the primary featured i…

01b24b2

…mage a hero candidate

Update phpdoc for DetermineHeroImages

87472ca

westonruter marked this pull request as ready for review April 14, 2021 23:59

westonruter requested a review from pierlon April 15, 2021 00:00

westonruter added 2 commits April 14, 2021 17:43

Make test_filter_attachment_image_attributes determinate

2235614

Fix test_filter_attachment_image_attributes in WP≤5.1

8bd9855

pierlon suggested changes Apr 15, 2021

View reviewed changes

westonruter and others added 2 commits April 14, 2021 20:14

Add missing entries to PHPSTORM_META

e292be3

Combine array_unshift() calls

09942f9

Co-authored-by: Pierre Gordon <16200219+pierlon@users.noreply.github.com>

westonruter requested a review from pierlon April 15, 2021 03:45

westonruter added Optimizer Performance labels Apr 15, 2021

westonruter enabled auto-merge April 15, 2021 04:10

westonruter added 2 commits April 14, 2021 21:16

Omit amp-toolbox/.gitattributes from dist

92d7baa

Remove unnecessary optional chaining and since not available in Node 12

334bb46

pierlon suggested changes Apr 15, 2021

View reviewed changes

Fix add_data_hero_candidate_attribute test

67a2f91

Co-authored-by: Pierre Gordon <16200219+pierlon@users.noreply.github.com>

pierlon approved these changes Apr 15, 2021

View reviewed changes

westonruter merged commit a1bc12f into develop Apr 15, 2021

westonruter deleted the fix/hero-image-candidate-determination branch April 15, 2021 04:58

schlessera reviewed Apr 15, 2021

View reviewed changes

westonruter mentioned this pull request Jun 30, 2021

Ensure first entry content in DetermineHeroImages matches amp-wp-article-content in legacy Reader templates #6440

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve hero image determination #6062

Improve hero image determination #6062

westonruter commented Apr 9, 2021 •

edited

Loading

codecov bot commented Apr 10, 2021 •

edited

Loading

westonruter Apr 10, 2021

westonruter Apr 11, 2021

westonruter Apr 11, 2021

westonruter commented Apr 12, 2021

westonruter commented Apr 13, 2021

github-actions bot commented Apr 15, 2021 •

edited

Loading

pierlon Apr 15, 2021

westonruter Apr 15, 2021

pierlon Apr 15, 2021

westonruter Apr 15, 2021

pierlon Apr 15, 2021

westonruter Apr 15, 2021

pierlon left a comment

schlessera Apr 15, 2021

westonruter Apr 15, 2021

	public function add_data_hero_candidate_attribute() {
	public function test_add_data_hero_candidate_attribute() {

	// Short-circuit for cron, CLI, admin requests or requests to non-frontend pages.
	if ( wp_doing_cron() \|\| ( defined( 'WP_CLI' ) && WP_CLI ) \|\| is_admin() \|\| in_array( $pagenow, [ 'wp-login.php', 'wp-signup.php', 'wp-activate.php', 'repair.php' ], true ) ) {
	return false;

Improve hero image determination #6062

Improve hero image determination #6062

Conversation

westonruter commented Apr 9, 2021 • edited Loading

Summary

Checklist

codecov bot commented Apr 10, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter commented Apr 12, 2021

westonruter commented Apr 13, 2021

github-actions bot commented Apr 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierlon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter commented Apr 9, 2021 •

edited

Loading

codecov bot commented Apr 10, 2021 •

edited

Loading

github-actions bot commented Apr 15, 2021 •

edited

Loading