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.

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

Photon: Do not remove width and height attributes when known #8196

Merged
merged 4 commits into from
Nov 18, 2017
Merged

Photon: Do not remove width and height attributes when known #8196

merged 4 commits into from
Nov 18, 2017

Conversation

ebinnion
Copy link
Contributor

@ebinnion ebinnion commented Nov 16, 2017 •
edited
Loading

Fixes #8191

Until now, when filtering content, Photon has removed width and height attributes from img tags. This makes sense in certain cases since we optimize and resize the image on WordPress.com. If we resize the image, but left the width and height attributes the same, it could cause distortion.

But, it also causes issues, which are described in #8191.

To address the issue, I am proposing that instead of always removing the width and height attributes, that we update the width and height instead.

To test:

  • Checkout branch
  • Run tests
  • Place various images in posts/pages. Observe that when there are height/width attributes for the image, that we don't strip the height/width tags. Ensure that you're "viewing source" as oppose to using dev tools.

@ebinnion ebinnion added [Feature] Photon aka "Image CDN". Feature developed in the Image CDN package and shipped in multiple plugins [Status] In Progress labels Nov 16, 2017
@ebinnion ebinnion added this to the 5.6 milestone Nov 16, 2017
@ebinnion ebinnion self-assigned this Nov 16, 2017
@ebinnion ebinnion requested a review from a team as a code owner November 16, 2017 21:02
@jeherve
Copy link
Member

jeherve commented Nov 16, 2017

Awesome to see you working on this!

I just wanted to add a link to #529 (comment) , since we've discussed issues with devicepx-jetpack and those size attributes in the past; it seems things can get tricky depending on the device you're using and the zoom level that's applied.

@ebinnion ebinnion changed the title Photon: First pass at keeping with height when both known Photon: Do not remove width and height attributes when known Nov 16, 2017
@ebinnion ebinnion added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Type] Bug When a feature is broken and / or not performing as intended and removed [Status] In Progress labels Nov 16, 2017
@ebinnion
Copy link
Contributor Author

Looking at the fieldguide I found a bit of information about how devicepx is supposed to work:

Images on files.wordpress.com with w or h but not crop in their queries have those query arguments rewritten.

Based on that, I would not expect devicepx to resize when resize, fit, or lb are present, but when w or h are present.

In testing, that seemed to be the case. Here are some examples from my site with this branch checked out:

screen shot 2017-11-16 at 4 29 19 pm

screen shot 2017-11-16 at 4 33 36 pm

gravityrail
gravityrail previously approved these changes Nov 16, 2017
View reviewed changes
Copy link
Contributor

@gravityrail gravityrail left a comment

Choose a reason for hiding this comment

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

I have confirmed that this fixes my issues with Photon + Lazy Images, and that width and height appear to be correct for my photon images.

@ebinnion
Copy link
Contributor Author

I added a couple of clarification comments, but have not changed functionality. Thank you for the review @gravityrail!

gravityrail
gravityrail reviewed Nov 16, 2017
View reviewed changes
class.photon.php
$new_tag = preg_replace( '#(?<=\s)(width|height)=["|\']?[\d%]+["|\']?\s?#i', '', $new_tag );
// If we are not transforming the image with resize, fit, or letterbox (lb), then we should remove
// the width and height arguments from the image to prevent distortion. Even if $args['w'] and $args['h']
// are present, Photon does not crop to those dimensions. Instead, it appears to favor height.
Copy link
Contributor

Choose a reason for hiding this comment

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

If photon favours height, could we keep the height attribute and leave out the width attribute for this case? This might go some way towards alleviating the issues with pages reflowing over and over while infinite-scrolling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. I'll make that change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After looking into this some more, I don't think this will work how we want.

In testing, it seems to work when the image is treated as a block level element. For example, if a paragraph immediately comes after the image, then the proper height will be reserved. But, if the image or paragraph is floated, then reflowing will still occur.

@ebinnion ebinnion added [Status] In Progress and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Nov 16, 2017
@ebinnion ebinnion dismissed gravityrail’s stale review November 16, 2017 22:59

Dismissing to address feedback

@ebinnion ebinnion added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] In Progress labels Nov 17, 2017
@ebinnion
Copy link
Contributor Author

Feedback was addressed. This is ready for review again.

oskosk
oskosk approved these changes Nov 17, 2017
View reviewed changes
Copy link
Contributor

@oskosk oskosk left a comment

Choose a reason for hiding this comment

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

LGTM!

@oskosk oskosk added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Nov 17, 2017
@gravityrail gravityrail merged commit 246227a into master Nov 18, 2017
@gravityrail gravityrail removed the [Status] Ready to Merge Go ahead, you can push that green button! label Nov 18, 2017
@gravityrail gravityrail deleted the fix/photon-width-height branch November 18, 2017 03:27
@jeherve jeherve added [Status] Needs Changelog [Status] Needs Testing We need to add this change to the testing call for this month's release and removed [Status] Needs Changelog [Status] Needs Testing We need to add this change to the testing call for this month's release labels Nov 20, 2017
jeherve added a commit that referenced this pull request Nov 24, 2017
@jeherve
Changelog: add #8196
d177882
oskosk pushed a commit that referenced this pull request Nov 28, 2017
@jeherve @oskosk
Changelog 5.6: running changelog. (#8192)
7844644 
* Changelog 5.6: create base for changelog.

* Update changelog with 5.5.1 info.

* Changelog: add #7930 and #8238

* Changelog: add #8076

* Changelog: add #8100

* Changelog: add #8117

* Changelog: add #8141

* Changelog: add #8143

* Changelog: add #8147

* Changelog: add #8149

* Changelog: add #8153

* Changelog: add #8173

* Changelog: add #8184

* Changelog: add #8196

* Changelog: add #8199

* Changelog: add #8093

* Changelog: add #8171

* Changelog: add #8182

* Changelog: add #8202, #8222

* Changelog: add #8228

* Changelog: add #8240

* Changelog: add #8251

* remove AL card change
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gravityrail gravityrail gravityrail left review comments

@oskosk oskosk oskosk approved these changes

Assignees

@ebinnion ebinnion

Labels
[Feature] Photon aka "Image CDN". Feature developed in the Image CDN package and shipped in multiple plugins [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Milestone
5.6
Development

Successfully merging this pull request may close these issues.
4 participants
@ebinnion @jeherve @gravityrail @oskosk