Skip to content

Use more robust HTML Tag Processor for Image Placeholders #1477

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

Jump to bottom
Merged
westonruter merged 9 commits into from
Aug 26, 2024
Merged

Use more robust HTML Tag Processor for Image Placeholders #1477

westonruter merged 9 commits into from
Aug 26, 2024

Conversation

@sagarnasit
Copy link
Contributor

@sagarnasit sagarnasit commented Aug 16, 2024

Summary

Fixes #1475

  • Use more robust HTML Tag Processor for Image Placeholder.

@sagarnasit sagarnasit requested a review from pbearne as a code owner August 16, 2024 13:18
@github-actions
Copy link

github-actions bot commented Aug 16, 2024 •
edited
Loading

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: sagarnasit <[email protected]>
Co-authored-by: westonruter <[email protected]>
Co-authored-by: felixarntz <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@sagarnasit
Copy link
Contributor Author

sagarnasit commented Aug 16, 2024 •
edited
Loading

Output for Image with picture element turned off:

<img
  data-dominant-color="788d7e"
  data-has-transparency="false"
  style="--dominant-color: #788d7e;"
  fetchpriority="high"
  decoding="async"
  width="1024"
  height="711"
  src="http://localhost:8888/wp-content/uploads/2024/08/IMG-1-jpeg.webp"
  alt=""
  class="wp-image-13 not-transparent"
  srcset="
    http://localhost:8888/wp-content/uploads/2024/08/IMG-1-jpeg.webp 1024w,
    http://localhost:8888/wp-content/uploads/2024/08/IMG-1-300x208-jpeg.webp 300w,
    http://localhost:8888/wp-content/uploads/2024/08/IMG-1-768x533-jpeg.webp 768w"
  sizes="(max-width: 620px) 100vw, 620px"
>

With the picture element turned on:

<picture
  class="wp-picture-13"
  style="display: contents;">
  <source
   type="image/webp"
   srcset="
    http://localhost:8888/wp-content/uploads/2024/08/IMG-1-jpeg.webp 1024w,
    http://localhost:8888/wp-content/uploads/2024/08/IMG-1-300x208-jpeg.webp 300w,
    http://localhost:8888/wp-content/uploads/2024/08/IMG-1-768x533-jpeg.webp 768w"
   sizes="(max-width: 620px) 100vw, 620px">
  <img
   data-dominant-color="788d7e"
   data-has-transparency="false"
   style="--dominant-color: #788d7e;"
   fetchpriority="high"
   decoding="async"
   width="1024"
   height="711"
   src="http://localhost:8888/wp-content/uploads/2024/08/IMG-1.jpeg"
   alt=""
   class="wp-image-13 not-transparent"
   srcset="
    http://localhost:8888/wp-content/uploads/2024/08/IMG-1.jpeg 1024w,
    http://localhost:8888/wp-content/uploads/2024/08/IMG-1-300x208.jpeg 300w,
    http://localhost:8888/wp-content/uploads/2024/08/IMG-1-768x533.jpeg 768w"
   sizes="(max-width: 620px) 100vw, 620px"
></picture>

After these changes style with --dominant-color is now on the IMG tag instead of previously on the PICTURE tag.

westonruter
westonruter reviewed Aug 16, 2024
View reviewed changes
westonruter
westonruter reviewed Aug 16, 2024
View reviewed changes
westonruter
westonruter reviewed Aug 16, 2024
View reviewed changes
plugins/dominant-color-images/hooks.php Outdated
$filtered_image = str_replace( ' style="', ' style="--dominant-color: #' . esc_attr( $image_meta['dominant_color'] ) . '; ', $filtered_image );
} else {
$filtered_image = str_replace( '<img ', '<img style="--dominant-color: #' . esc_attr( $image_meta['dominant_color'] ) . ';" ', $filtered_image );
$style_attribute = '--dominant-color: #' . esc_attr( $image_meta['dominant_color'] ) . '; ';
Copy link
Member

@westonruter westonruter Aug 16, 2024

Choose a reason for hiding this comment

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

No need to use esc_attr() with tag processor as this is handled for you.

Copy link
Member

@westonruter westonruter Aug 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
$style_attribute = '--dominant-color: #' . esc_attr( $image_meta['dominant_color'] ) . '; ';
$style_attribute = "--dominant-color: #{$image_meta['dominant_color']}; ";

westonruter
westonruter reviewed Aug 16, 2024
View reviewed changes
plugins/dominant-color-images/hooks.php Outdated

if ( ! empty( $image_meta['dominant_color'] ) ) {
$data .= sprintf( 'data-dominant-color="%s" ', esc_attr( $image_meta['dominant_color'] ) );
$processor->set_attribute( 'data-dominant-color', esc_attr( $image_meta['dominant_color'] ) );
Copy link
Member

@westonruter westonruter Aug 16, 2024

Choose a reason for hiding this comment

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

Suggested change
$processor->set_attribute( 'data-dominant-color', esc_attr( $image_meta['dominant_color'] ) );
$processor->set_attribute( 'data-dominant-color', $image_meta['dominant_color'] );

westonruter
westonruter reviewed Aug 16, 2024
View reviewed changes
sagarnasit
sagarnasit commented Aug 16, 2024
View reviewed changes
plugins/dominant-color-images/tests/test-dominant-color.php
Copy link
Contributor Author

@sagarnasit sagarnasit Aug 16, 2024

Choose a reason for hiding this comment

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

Using an HTML Tag Processor allows including single quotes and escaped double quotes for the image placeholder.

westonruter
westonruter reviewed Aug 16, 2024
View reviewed changes
westonruter
westonruter reviewed Aug 16, 2024
View reviewed changes
plugins/dominant-color-images/tests/test-dominant-color.php Outdated
Comment on lines 194 to 197
'escaped double quotes' => array(
'image' => '<img src=\"%s\">',
'expected' => false,
'expected' => true,
),
Copy link
Member

@westonruter westonruter Aug 16, 2024

Choose a reason for hiding this comment

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

This is a strange test case to begin with. If the string is <img src=\"http://example.com/foo.jpg\">, the HTML Tag Processor would parse the src as having the value of \"http://example.com/foo.jpg\". I guess this doesn't even matter because we're not doing anything with the src anyway. I just wonder what value this test case has in the first place.

Copy link
Member

@felixarntz felixarntz Aug 26, 2024

Choose a reason for hiding this comment

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

I think we should delete this test case, I agree it doesn't make sense.

@westonruter westonruter added [Type] Enhancement A suggestion for improvement of an existing feature [Plugin] Image Placeholders Issues for the Image Placeholders plugin (formerly Dominant Color Images) labels Aug 17, 2024
westonruter
westonruter approved these changes Aug 19, 2024
View reviewed changes
Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

LGTM!

felixarntz
felixarntz approved these changes Aug 26, 2024
View reviewed changes
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

LGTM, just I think it's worth deleting the one pointless test.

plugins/dominant-color-images/tests/test-dominant-color.php Outdated
Comment on lines 194 to 197
'escaped double quotes' => array(
'image' => '<img src=\"%s\">',
'expected' => false,
'expected' => true,
),
Copy link
Member

@felixarntz felixarntz Aug 26, 2024

Choose a reason for hiding this comment

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

I think we should delete this test case, I agree it doesn't make sense.

@sagarnasit
Copy link
Contributor Author

sagarnasit commented Aug 26, 2024

@felixarntz I have removed the test case.

@westonruter westonruter merged commit d2cdfe9 into WordPress:trunk Aug 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@westonruter westonruter westonruter approved these changes

@felixarntz felixarntz felixarntz approved these changes

@pbearne pbearne Awaiting requested review from pbearne pbearne is a code owner

Assignees

No one assigned

Labels

[Plugin] Image Placeholders Issues for the Image Placeholders plugin (formerly Dominant Color Images) [Type] Enhancement A suggestion for improvement of an existing feature

Projects

None yet

Milestone

dominant-color-images 1.1.2

Development

Successfully merging this pull request may close these issues.

Image Placeholders can be hardened with HTML Tag Processor

3 participants

@sagarnasit @westonruter @felixarntz