search

ampproject / amp-wp

Enable AMP on your WordPress site, the WordPress way.
https://wordpress.org/plugins/amp/
GNU General Public License v2.0
1.79k stars 383 forks source link

The 16/9 ratio chosen as constant for featured image is not ideal for generating the other two resolutions ( 1x1, 4x3 ) #5611

Closed naveen17797 closed 3 years ago

naveen17797 commented 3 years ago

Bug Description

The minimum featured image size ratio is set to 16 x 9 in the block editor warning which is not correct for generating other two resolution images.

export const getMinimumFeaturedImageDimensions = () => {
    const width = MINIMUM_FEATURED_IMAGE_WIDTH;

    const height = width * ( 9 / 16 );

    return { width, height };
};

For example if we chose the 16 / 9 ratio, we would have image with dimensions 1200 x 675. To generate the 1 x 1 ratio from this ratio we need to stretch the image to obtain 1200 px on height, which would lead to poor quality.

Expected Behaviour

The ratio should be set to 1 x 1 or a way to modify the ratio with filter.

Steps to reproduce

  1. Open block editor
  2. Navigate to featured image metabox
  3. You could see the warning for the image resolution

Screenshots

Screen Shot 2020-11-16 at 1 26 01 PM

Additional context

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation brief

QA testing instructions

Demo

Changelog entry

westonruter commented 3 years ago

The 16/9 ratio comes from Google Search's documentation for Article structured data: https://developers.google.com/search/docs/data-types/article

image

Images should be at least 1200 wide. So if you supply a featured image that is 1200x1200 then this will satisfy the check, no?

It's not only about the ratio: it's about the resolution.

ziodave commented 3 years ago

Exactly, editors are confused because they are tricked into believing that 1.200x675 is the viable requirement whereas Google recommends to provide 3 ratios: 1x1, 4x3 and 16x9; minimum width 1.200; minimum surface 800k pixels.

Which means that 1.200x1.200 is the minimum viable requirement.

And we can use the source image minimum 1.200x1.200 to create the 16x9 and 4x3 versions in WordPress as per Google recommendations.

westonruter commented 3 years ago

I don't understand. 1200x675 is a 16/9 aspect ratio. How is it not viable? This selected image's full size s is used by the plugin in the Schema.org metadata:

https://github.com/ampproject/amp-wp/blob/06c9b9531874174facfa805cad683c73617360a8/includes/amp-helper-functions.php#L1651-L1660

ziodave commented 3 years ago

Sorry, let me explain better. These are the relevant Google requirements for AMP Articles [1]:

These are the examples [2]:

Example 1, 2:

     ...
      "image": [
        "https://example.com/photos/1x1/photo.jpg",
        "https://example.com/photos/4x3/photo.jpg",
        "https://example.com/photos/16x9/photo.jpg"
       ],
     ...

Example 3:

{
  "@context": "https://schema.org",
  "@type": "NewsArticle",
  "image": [
    "https://example.com/photos/1x1/photo.jpg",
    "https://example.com/photos/4x3/photo.jpg",
    "https://example.com/photos/16x9/photo.jpg"
  ]
}

Hence it is recommended to provide 3 ratios: 1x1, 4x3, 16x9 and the minimum width is 1.200 pixels. Hence the minimum requirements should be for a 1.200x1.200 image (ratio 1x1).

As a secondary requirement, by using add_image_size [3] we can have WordPress generate the 4x3 and 16x9 images out of the minimum viable requirement of 1.200x1.200 pixels.

[1] https://developers.google.com/search/docs/data-types/article#article-types [2] https://developers.google.com/search/docs/data-types/article#examples [3] https://developer.wordpress.org/reference/functions/add_image_size/

westonruter commented 3 years ago

So on your site you want to add all three aspect ratios even though the AMP plugin currently only provides one, and you are generating the other aspect ratios via add_image_size()? You're then supplying those via the amp_schemaorg_metadata filter, or by means of your own Schema.org metadata? And to support this, you need authors to upload images that are at least 1200x1200?

ziodave commented 3 years ago

So on your site you want to add all three aspect ratios even though the AMP plugin currently only provides one, and you are generating the other aspect ratios via add_image_size()?

Correct. IMO the AMP plugin should provide the 3 image sizes (1.200x1.200, 1.200x900 and 1.200x675).

You're then supplying those via the amp_schemaorg_metadata filter, or by means of your own Schema.org metadata?

Own Schema.org metadata. But it should apply to the AMP SD as well.

And to support this, you need authors to upload images that are at least 1200x1200?

Correct.

-- As I see this as a general AMP Article requirement [1] I think that the minimum requirement should be set to 1200x1200 and that amp-wp itself will provide the recommended image sizes.

There are guidelines for images, e.g. (courtesy of kalicube.pro) [2]: image

We're of course willing to contribute in case.

[1] https://developers.google.com/search/docs/data-types/article#article-types [2] https://wordlift.io/blog/en/top-stories-carousel-checklist/

westonruter commented 3 years ago

IMO the AMP plugin should provide the 3 image sizes (1.200x1.200, 1.200x900 and 1.200x675).

The problem is that automatic crops are often poor. If the photo is a picture of a person, for example, a 1200x1200 photo could result in a 1200x900 crop that cuts out the person's eyes and mouth. This is why we do not automatically provide the other automatic aspect ratios in the AMP plugin.

ziodave commented 3 years ago

Yes, it adds complexity. Maybe we can split the issue in 2 parts:

  1. Allow 3rd parties to change the minimum image size requirements using a WP filter, so that we can still set the requirement to 1.200x1.200 if it fits the web site editorial workflow.
  2. Define a workflow for the generation of the 4x3 and 16x9 versions, starting from the default (centered) based on the guidelines below and which would allow the editor to make a different selection via the Web UI.

Guidelines:

image

The workflow could work similarly to the edit media screen of WordPress, while blocking the ratios, so that the editor could click somewhere "Edit Ratios", the media edit screen would open with the 3 preset ratios 1x1, 4x3 and 16x9 automatically set as centered. The user could then shrink/enlarge/move the fixed ratio selection to change the image focus.

image
westonruter commented 3 years ago

Adding a filter to customize the minimum required dimensions seems plausible to me.

Adding a workflow to art-direct the crops for the other ratios would be out of scope for the AMP plugin, I think.

naveen17797 commented 3 years ago

I think adding the filter on the server side would be good than adding it via js hooks, i would be happy to provide a PR when you make the decision.

ziodave commented 3 years ago

Ok, so we could add a wp.hooks call here:

https://github.com/ampproject/amp-wp/blob/7460a0e32e33172ef5c42b4a6a24dcb38dfbe970/assets/src/common/helpers/index.js#L62

Something like:

export const getMinimumFeaturedImageDimensions = () => {
    const width = MINIMUM_FEATURED_IMAGE_WIDTH;

    const height = width * ( 9 / 16 );

    return applyFilters( 'amp-wp/minimumFeaturedImageDimensions', { width, height } );
};

@westonruter @naveen17797 does it make sense?

swissspidy commented 3 years ago

I would probably simply add the filter on the PHP side, not using wp.hooks. Just my 2 cents :)

ziodave commented 3 years ago

@swissspidy I couldn't spot it in the PHP side: the 16:9 ratio seems to be hardcoded in JavaScript - const height = width * ( 9 / 16 );. Can you point us to the code in PHP?

swissspidy commented 3 years ago

@ziodave There isn't. I was merely suggesting adding apply_filters() in PHP rather than wp.hooks.applyFilters() in JavaScript

ziodave commented 3 years ago

Ah gotcha, so we should:

  1. move this MINIMUM_FEATURED_IMAGE_WIDTH from constants.js to the PHP side
  2. add a MINIMUM_FEATURED_IMAGE_RATIO constant
  3. a PHP filter such as 
    apply_filters( 'amp_minimum_featured_image', array(
    'width' => MINIMUM_FEATURED_IMAGE_WIDTH,
    'ratio' => MINIMUM_FEATURED_IMAGE_RATIO,
) );
  4. export them to the edit post screen using wp_localize_script
  5. update getMinimumFeaturedImageDimensions to use the settings provided by wp_localize_script

Does it sound right?

westonruter commented 3 years ago

Yes, that sounds right. However, I'd recommend using wp_add_inline_script() (with wp_json_encode()) rather than wp_localize_script() since the latter casts values to strings.

Also, should the filtered value not be width and height instead of width and ratio?

Maybe the filtered value should be an array with just two numbers:

list( $minimum_width, $minimum_height ) = apply_filters( 'amp_minimum_featured_image_dimensions', [ 1200, 675 ] );
ziodave commented 3 years ago

@naveen17797 I think we have enough specs to contribute here.

naveen17797 commented 3 years ago

yes, i am working on it.

naveen17797 commented 3 years ago

when i try to run tests with

npm run test:php as mentioned on the engineering guidelines i get phpunit does not exist error.

when i checked package.json. it seems to be using globally installed phpunit

"test:php": "phpunit",

but i also found phpunit in vendor directory after installing dependencies with composer.

Whats the correct way to do run the tests, is test suite compatible with latest version of phpunit ?

@westonruter

swissspidy commented 3 years ago

You should be able to run them using vendor/bin/phpunit

westonruter commented 3 years ago

You should be able to run them using vendor/bin/phpunit

Yes, I noticed this as well. I have this fixed in the branch I'm currently working on (#5558):

https://github.com/ampproject/amp-wp/blob/517c2802bf6b03f0879960a428ae3d2f4f486540/package.json#L141

pierlon commented 3 years ago

The Engineering Guidelines will also have to be reviewed and updated.

pierlon commented 3 years ago

QA passed