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

[WIP] Fix reading of small SVG images to determine dimensions #1807

Closed
wants to merge 1 commit into from

Conversation

westonruter
Copy link
Member

This issue came up via this support topic: https://wordpress.org/support/topic/how-to-show-emoji-show-share-button-and-hide-powered-by-on-amp-page/

With this raw post content:

<p>Image: <img class="emoji loaded" draggable="false" src="https://s.w.org/images/core/emoji/11/svg/1f642.svg" alt="🙂"></p>

<p>Character: 🙂</p>

In HTML non-AMP this gets rendered as:

image

Note that the emoji character is being converted into an image by WordPress's emoji handling. In AMP this is disabled, so the character passes through as a character but the image is incorrectly rendered in AMP:

image

The underlying HTML is:

<p>Image: <amp-img class="emoji loaded amp-wp-unknown-size amp-wp-unknown-width amp-wp-unknown-height amp-wp-enforced-sizes" draggable="false" src="https://s.w.org/images/core/emoji/11/svg/1f642.svg" alt="🙂" width="525" height="400" layout="intrinsic"></amp-img></p>

<p>Character: 🙂</p>

The problem turns out to be an issue with FasterImage and how it was modified in #1150 to allow extracting pixel dimensions from SVG images. The problem is that it is attempting to read 1024 bytes of data from the image, but the above SVG is only 525 bytes long. This results in a WillWashburn\Stream\StreamBufferTooSmallException exception being thrown, and the image size failing to be obtained, and thus the fallback image size is being used (which is obviously too big).

So this PR fixes that problem by allowing small SVG images to be read without throwing an error.

Work Outstanding

Before

image

After

image

While this is way better, it is still not perfect. The SVG image has a viewbox of 36x36 so that is what is assigned as the width & height of the amp-img. Nevertheless, the emojis are supposed to be 16x16. Or rather, they are supposed to be 1em by 1em, so it should look like this:

image

So to full fix the case of inline SVG emojis we potentially need to either replace the img.emoji back with the underlying character (which is found in the alt attribute). This would not be ideal because we'd miss out on the improvements that WordPress's emoji handling was introduced for.

A full fix seems to be two-fold:

  • Make emoji SVG images get sizes at 1em somehow. This is a challenge because amp-img doesn't allow em units.
  • Make an AMP-compatible version of WordPress's emoji handling, by adding a sanitizer that does in PHP what core does in JS. In the AMP plugin we disable the emoji handling in AMP:

remove_action( 'wp_head', 'print_emoji_detection_script', 7 );
remove_action( 'wp_print_styles', 'print_emoji_styles' );

These may be out of scope for this PR.

Other outstanding work:

  • Fix unit test.
  • Replace cURL with WP HTTP API (which would also help with mocking the uint tests).

Fixes #204.

@googlebot googlebot added the cla: yes Signed the Google CLA label Jan 11, 2019
@westonruter
Copy link
Member Author

Closing in favor of willwashburn/fasterimage#14

@westonruter westonruter closed this Mar 6, 2019
@westonruter westonruter deleted the fix/svg-image-sizing branch March 6, 2019 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Signed the Google CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants