Skip to content
This repository has been archived by the owner on Jan 15, 2019. It is now read-only.

Fix sizes attribute for regular aligned body images. #701

Closed
wants to merge 2 commits into from

Conversation

mor10
Copy link
Contributor

@mor10 mor10 commented Dec 2, 2018

This PR fixes the incorrect sizes attribute for regular aligned body images. It does not solve for alignwide and alignfull images, but is an improvement over core. While we wait for the wp_calculate_image_sizes filter to be updated at some later date, this PR should be merged with core before 5.0 release to avoid even regularly aligned images receiving an incorrect sizes attribute.

This PR follows the same pattern as what is already in previous default themes including Twenty Seventeen: https://github.com/WordPress/WordPress/blob/master/wp-content/themes/twentyseventeen/functions.php#L491-L517

@kjellr I strongly recommend this be merged before 5.0 release.

@kjellr
Copy link
Collaborator

kjellr commented Dec 3, 2018

Thanks, @mor10. In general, this seems like a sound change, and the values you added match up with the expected widths for non-wide/full images. 👍

What's the best way to test this? I can see that the sizes attribute is properly updated when running this PR, but I'm not 100% sure how to verify that those rules are being used. Can you provide some steps? Thank you!

@mor10
Copy link
Contributor Author

mor10 commented Dec 5, 2018

@kjellr Easiest way to test:
0. Make a post with a single regular aligned image (use a wide source file: 1500px or wider)

  1. boot up Firefox
  2. open developer tools
  3. turn on "Responsive Mode" or whatever that thing is called
  4. scale the window down to a small mobile size
  5. set display resolution to 1x
  6. Open the Network inspector
  7. Reload the page
  8. Watch the network inspector for the image file downloaded
  9. Slowly widen the viewport and see new image files dropping in at the specified breakpoints.

Yes, this is a total pain to test, which is why it's often overlooked. To test you have to force the browser to make visible what it very much wants to keep hidden.

@mor10
Copy link
Contributor Author

mor10 commented Dec 5, 2018

For the record, the output code here is identical to the output code in this demo, which has graphic size displays for easier reference: https://gutenberg.mor10.com/responsive-images-demo-corrected-code/

Same procedure as above applies for testing.

@kjellr
Copy link
Collaborator

kjellr commented Dec 5, 2018

Thank you! Those instructions are very helpful. I tested this out, and I can see that the browser is now pulling in different image sizes than before.

Testing with a 1920x1080px non-aligned image, inserted via Gutenberg (and set to Image Size: Full).

Default:

  • <300px: A 300px image is loaded
  • >300px: A 768px image is loaded
  • >768px: A 1024px image is loaded
  • >1024px: A 1568px image is loaded
  • >1568px: The original 1980px image is loaded

With this PR:

  • <300px: A 300px image is loaded
  • >300px: A 768px image is loaded
  • >1195px: A 1024px image is loaded
  • >1579px: A 1568px image is loaded
  • >2395px: The original 1980px image is loaded

For non-aligned images, this looks great. But unfortunately alignfull images are picking up these same image sizes. This has a negative impact on those images:

Current:
Full-width Image on a 1000px screen pulls in a 1024px-wide image.
screen shot 2018-12-05 at 1 13 01 pm

This PR:
Full-width Image on a 1000px screen pulls in a pixellated 768px-wide image.
screen shot 2018-12-05 at 1 12 18 pm

In the case of Twenty Seventeen, a custom setting for that sizes attribute was important because you'd never end up with a 100% wide image inside of a post. But since Gutenberg allows users to insert 100% wide images here, it doesn't make sense to limit the width for all images like this. Unless we can adjust this PR to exclude full/wide images, I think it's better for us to wait for a more complete solution upstream in Gutenberg.

Edit: I'll also note that the demo you shared above doesn't seem to have this issue, so I'm not sure what's different there.

@mor10
Copy link
Contributor Author

mor10 commented Dec 5, 2018

@kjellr What you're seeing with wide and full images is the exact problem I've flagged in various tickets and PRs in Twenty Nineteen (#50), Gutenberg (issue 6177), and WordPress Core (trac ticket 45407). The solution sits in #629.

The demo I shared above has proper sizes attributes for all image widths as per #629.

@kjellr
Copy link
Collaborator

kjellr commented Dec 6, 2018

@kjellr What you're seeing with wide and full images is the exact problem I've flagged in various tickets and PRs in Twenty Nineteen (#50), Gutenberg (issue 6177), and WordPress Core (trac ticket 45407). The solution sits in #629.

The demo I shared above has proper sizes attributes for all image widths as per #629.

Yes, I see that. I meant to convey that until a solution to that problem is sorted out, this PR should probably be put on hold — since it has a negative impact on the appearance of alignfull/alignwide images.

@kjellr
Copy link
Collaborator

kjellr commented Jan 14, 2019

This PR has been ported over to Trac as a patch on this ticket: https://core.trac.wordpress.org/ticket/45985

@kjellr kjellr closed this Jan 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants