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.

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

Parse width presets for use in sizes values #1251

Closed
Tracked by #760 ...
mukeshpanchal27 opened this issue May 28, 2024 · 5 comments · Fixed by #1252
Closed
Tracked by #760 ...

Parse width presets for use in sizes values #1251

mukeshpanchal27 opened this issue May 28, 2024 · 5 comments · Fixed by #1252
Assignees
Labels
[Plugin] Auto Sizes Issues for the Auto Sizes plugin [Type] Enhancement A suggestion for improvement of an existing feature

Comments

@mukeshpanchal27
Copy link
Member

mukeshpanchal27 commented May 28, 2024

As described in #1187 (comment), If the image is smaller than the content or wide size, it returns the wrong (bigger) size. In such cases, we need to return the smaller value among the image width, content size, or wide size.

The different widths used in theme.json for content and wide size.

  • 620px
  • min(640px, 90vw)
  • 90vw

We should parse the width so we can use proper width in sizes to get proper image size.

@mukeshpanchal27
Copy link
Member Author

For initial work:

  • Only return a smaller image size if the content or wide size is larger than the image size.
    • If content size is 620px and content use thumbnail the the size will use thumbnail 150px image size instead of 620px.
  • Parse the width value for "px"; for other types( vw, min() ), return the image width according to the default WordPress behaviour.

cc. @joemcgill

@joemcgill
Copy link
Member

joemcgill commented Jun 11, 2024

Thanks for raising this issue, @mukeshpanchal27. As I've been reviewing #1252, I noticed that even when a smaller image is used, WP includes the following default CSS that will make a smaller image stretch to the wide and full alignment container, regardless of its intrinsic width:

.wp-block-image.alignfull img,.wp-block-image.alignwide img{
  height:auto;
  width:100%;
}

Usually, WordPress should be using the wide or full alignment size to populate the sizes attribute in those cases, but instead is using the value from the width attribute instead.

The only use cases where the sizes improvements in our plugin should respect a smaller intrinsic width of the image is when it is using default, center, left, or right alignments. For wide and full alignments, the images get stretched to fill the width of the container, so our plugin should update the sizes attribute to use the layout width instead. The same use cases apply when an image is manually resized in the editor. However, in those cases, the image block controls the layout width by applying an inline style attribute, like style="width:400px", which doesn't get reflected in the width attribute added to the img, nor the default sizes attribute that WP generates.

Another thing I realized while reviewing your PR is that we should be able to use the image width that is already added to the image, rather than needing to call wp_get_attachment_image_src(), which will result a DB query for each image. Generally the width attribute is only added dynamically to image blocks during wp_filter_content_tags, which avoids the extra DB queries by scanning all of the content for images and running _prime_post_caches on all the ids (ref). We will need to update the approach for improving the sizes attribute so that it runs as part of that process, rather than on render_block filters.

Summarizing next steps

  • Move processing on sizes attributes from render_block filters, to wp_filter_content_tags.
  • Update sizes for default, center, left, and right aligned images to respect smaller image widths.
    • Account for manually resized images, when considering smaller image layout widths.
  • Ensure sizes for wide/full alignments always use the layout width, regardless of image size.
@mukeshpanchal27
Copy link
Member Author

mukeshpanchal27 commented Jun 13, 2024

Another thing I realized while reviewing your PR is that we should be able to use the image width that is already added to the image, rather than needing to call wp_get_attachment_image_src(), which will result a DB query for each image. Generally the width attribute is only added dynamically to image blocks during wp_filter_content_tags, which avoids the extra DB queries by scanning all of the content for images and running _prime_post_caches on all the ids (ref). We will need to update the approach for improving the sizes attribute so that it runs as part of that process, rather than on render_block filters.

@joemcgill If we use wp_content_img_tag image filter we don't get the alignment options so we can't update sizes through it. Correct me if i miss anything.

@joemcgill
Copy link
Member

Correct me if i miss anything

True. those block attributes get rendered as CSS classes on the figure element that wraps each image, but those don't get passed to the wp_content_img_tag filter. It would really be best if we could prime these attachment caches prior to parsing blocks so we didn't have to update the rendered content after the fact.

One way that I can see to do that is to filter the block_parser_class and implement our own WP_Block_Parser class that extends WP_Block_Parser so we can prime all image ids in the content during the parse() method prior to actually running the parser. Here's some pseudocode to explain what I mean.

class WPP_Block_Parser extends WP_Block_Parser {
  private static $attachment_ids;

  public function parse( $document ) {
    // parse the blocks.
    $blocks = parent::parse()

    // Loop through blocks to get all of the attachment IDs.
    foreach ( $blocks as $block ) {
      // Look for ID and add to the $attachment_ids array.
    }

    _prime_post_caches( self::$attachment_ids, false, true );

    return $blocks;
  }
}

This would probably be much easier to do if https://github.com/WordPress/wordpress-develop/pull/6533 gets merged, because then we could just filter the whole rendered block template content.

Alternately, we could add the alignment to the img during block parsing and remove it again when we actually use that alignment to update the sizes attribute.

@joemcgill
Copy link
Member

Marking this as fixed by #1252.

@westonruter westonruter added this to the auto-sizes 1.1.0 milestone Jul 10, 2024
@westonruter westonruter added the [Type] Enhancement A suggestion for improvement of an existing feature label Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Plugin] Auto Sizes Issues for the Auto Sizes plugin [Type] Enhancement A suggestion for improvement of an existing feature
4 participants