Improve story page background media with image srcset, reduced image size, and a11y text #3006

westonruter · 2019-08-11T07:09:25Z

Changes amp_story_page image size to 1280px high instead of 1440px high.
Instead of adding an amp-img for the amp-story-page background image element, this PR adds an img similar to how the core/image block does, including the important wp-image-{attachment_id} class. This ensures that wp_make_content_images_responsive() is able to supply a srcset for the image (whereas currently there is none), potentially allowing reduced bandwidth when more appropriate image sizes are available; this allows for plugins like Jetpack to supply dynamically-generated image sizes. The image sanitizer converts the img to an amp-img. The inclusion of srcset also means this logic can be replaced with just const mediaUrl = media.url:

amp-wp/assets/src/stories-editor/blocks/amp-story-page/edit.js

Line 137 in 186c83e

    
           const mediaUrl = has( media, [ 'sizes', MAX_IMAGE_SIZE_SLUG, 'url' ] ) ? media.sizes[ MAX_IMAGE_SIZE_SLUG ].url : media.url;

Improves accessibility by adding an alt attribute to the background image, and an aria-label to the background video.
Uses the object-position attribute instead of the object-position style. This reduces the amount of CSS included style[amp-custom] and avoids the opaque amp-wp-... class name.

Still needing to be done:

Add block deprecation logic.
Add 1800px-tall image size for desktop stories?
Simplify logic in \AMP_Story_Media::add_new_max_image_size()? See c330ce7.

Rendered Output Before

...
<amp-story-grid-layer template="fill">
	<amp-img
		src="https://wordpressdev.lndo.site/content/uploads/2019/08/American_bison_k5680-1-2-2208x1280.jpg"
		layout="fill"
		class="amp-wp-896316c"
	></amp-img>
</amp-story-grid-layer>
...

Rendered Output After

...
<amp-story-grid-layer template="fill">
	<amp-img
		src="https://wordpressdev.lndo.site/content/uploads/2019/08/American_bison_k5680-1-2.jpg"
		layout="fill"
		alt="American Bison"
		class="wp-image-2911 amp-wp-enforced-sizes"
		object-position="85% 50%"
		srcset="https://wordpressdev.lndo.site/content/uploads/2019/08/American_bison_k5680-1-2-1963x1280.jpg 1963w, https://wordpressdev.lndo.site/content/uploads/2019/08/American_bison_k5680-1-2-300x196.jpg 300w, https://wordpressdev.lndo.site/content/uploads/2019/08/American_bison_k5680-1-2-1024x668.jpg 1024w, https://wordpressdev.lndo.site/content/uploads/2019/08/American_bison_k5680-1-2-768x501.jpg 768w, https://wordpressdev.lndo.site/content/uploads/2019/08/American_bison_k5680-1-2-1200x783.jpg 1200w, https://wordpressdev.lndo.site/content/uploads/2019/08/American_bison_k5680-1-2-1400x913.jpg 1400w"
	></amp-img>
</amp-story-grid-layer>
...

Supply wp-image-X class name so that srcset is supplied. Also use object-position attribute instead of object-position inline style.

westonruter · 2019-08-11T07:12:48Z

I'm somewhat out of my depth for the deprecated handling.

/cc @swissspidy

…t size

westonruter · 2019-08-11T21:18:53Z

assets/src/stories-editor/blocks/amp-story-page/edit.js

 			mediaType = media.type;
 		}

-		const mediaUrl = has( media, [ 'sizes', MAX_IMAGE_SIZE_SLUG, 'url' ] ) ? media.sizes[ MAX_IMAGE_SIZE_SLUG ].url : media.url;


@kienstra This appears no longer to be necessary when the srcset is present.

Originated in 0bd76bc

Ah, that makes sense.

swissspidy · 2019-08-12T12:24:06Z

@miina Mind taking a look at adding the block deprecation for the changed img tag?

miina · 2019-08-12T13:30:17Z

Mind taking a look at adding the block deprecation for the changed img tag?

Sure, checking now.

googlebot · 2019-08-12T13:54:17Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

miina · 2019-08-12T13:55:22Z

@googlebot I consent.

googlebot · 2019-08-12T13:55:26Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

miina · 2019-08-12T13:57:32Z

Added the deprecation,

Templates need an update as well,

will check that, too, to be able to test the deprecation better.

westonruter · 2019-08-12T21:29:02Z

Eliminate now-unnecessary unsetting of $image_sizes['full'] in \AMP_Story_Media::add_new_max_image_size()? See c330ce7.

@kienstra Thoughts on how much of this is still relevant now?

kienstra · 2019-08-13T00:01:28Z

Hi @westonruter,

Eliminate now-unnecessary unsetting of $image_sizes['full'] in \AMP_Story_Media::add_new_max_image_size()? See c330ce7.

@kienstra Thoughts on how much of this is still relevant now?

Good question.

I think the elseif block that unsets the full image may still be relevant, but the first if block probably isn't.

1. First `if` block

amp-wp/includes/class-amp-story-media.php

Lines 223 to 224 in f17f597

    
           if ( isset( $_POST['action'] ) && ( 'query-attachments' === $_POST['action'] || 'upload-attachment' === $_POST['action'] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Missing 
        
           	$image_sizes[ self::MAX_IMAGE_SIZE_SLUG ] = $full_size_name;

^ This probably isn't needed anymore. It had 2 purposes:

a) On clicking 'Background Media' > 'Select Media' and uploading an image, it adds self:: MAX_IMAGE_SIZE_SLUG to the available image sizes

b) In the Image block, on clicking 'Media Library,' it adds the self:: MAX_IMAGE_SIZE_SLUG to the available image sizes in the Media Library

But removing that if block doesn't seem to have an effect. Like you mentioned, in this PR the Background Media doesn't look at media.sizes, it uses the media.url.

2. elseif block

amp-wp/includes/class-amp-story-media.php

Lines 225 to 228 in f17f597

    
           } elseif ( get_post_type() && AMP_Story_Post_Type::POST_TYPE_SLUG === get_post_type() ) { 
        
           	$image_sizes[ self::MAX_IMAGE_SIZE_SLUG ] = $full_size_name; 
        
           	unset( $image_sizes['full'] ); 
        
           }

Deleting this conditional did have an effect, and I'm not sure if it'd be good to delete it now.

To recap what it does, in the Image block's 'Image Size' <select>, it removes the 'Full Size', and adds 'Story Max Size':

Without that code above, a user can select a full size image. And it looks like the srcset doesn't reduce the image size enough.

Though the user would have to select the full size image, large is still the default.

For example:

Checkout this PR's branch and delete this line:

amp-wp/includes/class-amp-story-media.php

Line 99 in f17f597

add_filter( 'image_size_names_choose', [ __CLASS__, 'add_new_max_image_size' ] );
Create an Image block
Click 'Media Library'
Upload a 4000 x 4000 image
In the 'Image Size' <select>, choose 'Full Size'
Publish the story, and go to the front-end
Expected: the srcset produces a currentSrc at least a little smaller than the full 4000 x 4000 size
Actual: the currentSrc is 4000 x 4000

This seems to be the case even with smaller image sizes, like 2000 x 2000.

With this PR's branch as is, without deleting that line, it's a little better:

…lable image size

westonruter · 2019-08-19T22:02:52Z

@kienstra Thanks a lot for the thorough response. I did some testing as well and it turns out that when doing device emulation (simulating Pixel 2, for example) the srcset causes the full size to be reduced to the dimensions of the story page automatically. The srcset is not properly calculated when looking at a non-landscape story on a desktop viewport. So this being the case, I think it's fine actually to leave the full size. Done in 9e96dfe.

westonruter · 2019-08-19T22:03:14Z

I think we can defer adding a new image size for desktop for a future release.

westonruter · 2019-08-20T05:11:03Z

@miina Could you resolve the conflicts with the templates?

westonruter · 2019-08-20T17:06:15Z

Humm, E2E failures?

miina · 2019-08-20T17:10:01Z

Hmph, there's a test that doesn't pass reliably, will restart the tests, should look into improving this test again. Thought that it got fixed already but apparently not.

miina · 2019-08-20T17:19:31Z

Or actually, this time it looks like it's another test. Looking into this now.

includes/templates/story-templates/travel-vertical.html

miina · 2019-08-20T17:47:24Z

A different test is failing every time. Hard to debug as well since locally not failing.

It might be related to the issue in this version of Puppeteer that page.click doesn't seem to work reliably and instead page.evaluate should be used for consistent results, e.g.:

        await page.evaluate( ( selector ) => {
		document.querySelector( selector ).click();
	}, btnSelector );

The upstream helpers use the page.click, perhaps we should try replacing those and then try. @swissspidy Any other ideas?

swissspidy · 2019-08-20T18:19:03Z

Looking at the tests now (tests/e2e/specs/stories-editor/cta.test.js failed), but what I can see is that there are block validation errors. Mind checking that, @miina?

This reverts commit 16d5aae.

swissspidy · 2019-08-20T19:09:58Z

Skipped that flaky test for now; can be investigated separately.

miina · 2019-08-20T21:17:06Z

Will look into that CTA test.

westonruter · 2019-08-20T22:14:35Z

Opened Jetpack PR to reduce Photon-sized images from 1440px to 1280px: Automattic/jetpack#13278

* tag '1.2.1': (434 commits) Bump 1.2.1 Adjust block inserter style. (#3075) Update dependency eslint-plugin-jest to v22.15.2 Fix flaky CTA test (#3057) Add more resizing handles (#3023) Bump version to 1.2.1-RC1 Improve story page background media with image srcset, reduced image size, and a11y text (#3006) Wait until content loaded before calculating font size. (#3052) Harden logic for normalizing image metadata before adding story images (#3049) Update dependency uuid to v3.3.3 (#3046) Inline color support (#3033) Update dependency webpack-cli to v3.3.7 (#3040) Tiny prop-types fix Update dependency babel-jest to v24.9.0 (#3037) Update e2e test setup (#3031) Include amp-script among dynamic_element_selectors in tree shaking Another try to fix tests. Ensure selecting the first page before removing the block. Move setting input selection to the end to helpers. Fix editor test. ...

westonruter added 3 commits August 10, 2019 23:00

Use img instead of amp-img for amp-story-page background image

ea3d577

Supply wp-image-X class name so that srcset is supplied. Also use object-position attribute instead of object-position inline style.

Add media alt/title to amp-story-page background media

32831f5

Allow object-position and layout attributes on img elements for Kses

bea38a4

westonruter added this to the v1.2.1 milestone Aug 11, 2019

googlebot added the cla: yes Signed the Google CLA label Aug 11, 2019

westonruter changed the title ~~Add srcset and alt attributes to amp-story-page background images~~ Improve story page background image and video with srcset, reduced size, and alt Aug 11, 2019

westonruter added 2 commits August 11, 2019 14:08

Reduce amp_story_page image size height from 1440 to 1280

28fc9be

Eliminate manual image size selection since srcset now picks the righ…

ec8226e

…t size

westonruter commented Aug 11, 2019

View reviewed changes

Add deprecation for amp-story-page block.

4fc5d7a

googlebot added cla: no Has not signed the Google CLA and removed cla: yes Signed the Google CLA labels Aug 12, 2019

googlebot added cla: yes Signed the Google CLA and removed cla: no Has not signed the Google CLA labels Aug 12, 2019

miina added 2 commits August 12, 2019 17:02

Switch src attribute's position for causing less changes in templates.

a8a5d1b

Update templates.

48caf6b

Rely on srcset to select best image size; only add Story Page as avai…

9e96dfe

…lable image size

westonruter marked this pull request as ready for review August 19, 2019 22:03

westonruter requested a review from swissspidy August 19, 2019 22:42

Merge develop & resolve conflicts.

b6e60ec

westonruter added the AMP Stories label Aug 20, 2019

swissspidy reviewed Aug 20, 2019

View reviewed changes

includes/templates/story-templates/travel-vertical.html Show resolved Hide resolved

Bump template version.

1302353

westonruter changed the title ~~Improve story page background image and video with srcset, reduced size, and alt~~ Improve story page background media with image srcset, reduced image size, and a11y text Aug 20, 2019

swissspidy added 4 commits August 20, 2019 20:33

Fix block validation issues in templates

0640536

Use XPath for CTA block test

16d5aae

Revert "Use XPath for CTA block test"

ee8393f

This reverts commit 16d5aae.

Temporarily skip flaky test

7745774

swissspidy approved these changes Aug 20, 2019

View reviewed changes

swissspidy merged commit f5cbc6f into develop Aug 20, 2019

swissspidy deleted the add/amp-story-page-background-image-srcset branch August 20, 2019 19:11

westonruter mentioned this pull request Aug 20, 2019

Reduce Photon-resized AMP Story image max height from 1440 to 1280 Automattic/jetpack#13278

Merged

westonruter mentioned this pull request Sep 3, 2019

Identify areas where alt text is missing, currently, for images/video. #2983

Closed

Improve story page background media with image srcset, reduced image size, and a11y text #3006

Improve story page background media with image srcset, reduced image size, and a11y text #3006

Uh oh!

Conversation

westonruter commented Aug 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rendered Output Before

Rendered Output After

Uh oh!

westonruter commented Aug 11, 2019

Uh oh!

westonruter Aug 11, 2019

Choose a reason for hiding this comment

Uh oh!

kienstra Aug 12, 2019

Choose a reason for hiding this comment

Uh oh!

swissspidy commented Aug 12, 2019

Uh oh!

miina commented Aug 12, 2019

Uh oh!

googlebot commented Aug 12, 2019

Uh oh!

miina commented Aug 12, 2019

Uh oh!

googlebot commented Aug 12, 2019

Uh oh!

miina commented Aug 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

westonruter commented Aug 12, 2019

Uh oh!

kienstra commented Aug 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1. First if block

2. elseif block

Uh oh!

westonruter commented Aug 19, 2019

Uh oh!

westonruter commented Aug 19, 2019

Uh oh!

westonruter commented Aug 20, 2019

Uh oh!

westonruter commented Aug 20, 2019

Uh oh!

miina commented Aug 20, 2019

Uh oh!

miina commented Aug 20, 2019

Uh oh!

Uh oh!

miina commented Aug 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swissspidy commented Aug 20, 2019

Uh oh!

swissspidy commented Aug 20, 2019

Uh oh!

miina commented Aug 20, 2019

Uh oh!

westonruter commented Aug 20, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

westonruter commented Aug 11, 2019 •

edited

Loading

miina commented Aug 12, 2019 •

edited

Loading

kienstra commented Aug 13, 2019 •

edited

Loading

1. First `if` block

miina commented Aug 20, 2019 •

edited

Loading