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

Do a full pass on responsive images #50

Open
mor10 opened this Issue Oct 16, 2018 · 20 comments

Comments

Projects
None yet
5 participants
@mor10
Contributor

mor10 commented Oct 16, 2018

With full-width images being a thing in this theme it is essential we do a full pass on responsive images to ensure the browser has sufficient info (ie the correct sizes attributes) to load the image size it needs for the current display. This requires testing. TBC.

@kjellr

This comment has been minimized.

Collaborator

kjellr commented Oct 17, 2018

In general, sizes attributes for full-width images should be handled in Gutenberg. As long as this is covered there, we should be set.

Wide width featured images are another story though- so I'll mark this as a task to follow up on.

@kjellr kjellr added the task label Oct 17, 2018

@mor10

This comment has been minimized.

Contributor

mor10 commented Oct 17, 2018

I'll do some testing to see if any issues arise. Earlier versions of Gutenberg were not handling this well, but I know work has been done so hopefully it's a non-issue.

@LittleBigThing

This comment has been minimized.

LittleBigThing commented Oct 25, 2018

Related to this issue. We are using a background image as a featured image for single posts and pages with background-size: cover. This means that the same image is loaded for each screen size and we are not taking advantage of responsive images as available in core for the <img> tag.

As an alternative, we could output the collection of images in <img> and use object-fit: cover to style it. Object-fit has a quiet good browser support, except for IE.
It was also used for Twenty Seventeen, so we have an example. 👍

I think that this is still doable and that it could make a difference for mobile/tablet usage. Although I'd need to test it first regarding the actual gains as that depends on the available image sizes and how much an images has to 'be fitted' to have it cover the whole screen.

Also related to #181.
This can also be related to supporting header media (like images and videos) if requested.

@joyously

This comment has been minimized.

joyously commented Oct 25, 2018

Be sure to consider #208.

@LittleBigThing

This comment has been minimized.

LittleBigThing commented Oct 25, 2018

Ok, thanks. The issue of having small featured images will probably remain using the above suggestion.

@kjellr

This comment has been minimized.

Collaborator

kjellr commented Oct 25, 2018

@LittleBigThing A refactor to use images that way would be fantastic. I'm not sure @allancole or I will have a chance to work on that before RC1, but I'd definitely encourage any PRs.

@mor10

This comment has been minimized.

Contributor

mor10 commented Oct 25, 2018

I'll take a closer look on Monday. We can leverage CSS Grid to make the featured image responsive and accessible with a background image fallback.

@mor10

This comment has been minimized.

Contributor

mor10 commented Oct 29, 2018

I've done some preliminary testing and I think work needs to be done here. At present, responsive images are not handled correctly resulting in a significant performance hit. How to solve this problem depends in part on the result of Gutenberg issue 6177. Tagging @azaozz for input.

To replicate my procedure:

  1. Add a single 1600px wide image to a post using the "normal" displayed width (alignment doesn't matter).
  2. Open post in an incognito window making sure the viewport width is as narrow as possible (400px or so)
  3. Run the Network monitor in dev tools and monitor images.
  4. Slowly increase the width of the browser window to see new image files slot in.

At present configuration, the sizes attribute of the responsive images markup does not account for the actual displayed width of images:

  • At small viewport width, image files are loaded in with a width of 768px.
  • At the 768px breakpoint, images with a width of 1024px are loaded.
  • At the 1024px breakpoint, the original image file is loaded
  • I've only tested this with images up to 1600px wide. It's possible with wider images another step is added to this ladder.

Meanwhile the displayed width of the images are as follows:

  • Viewport width 0 to 767: max width 723px
  • Viewport width 768 to 1167: max width 778px
  • Viewports 1168 to infinity: max width: calc(6 * (100vw / 12))

The maximum width of my gigantic screen is 2560px. At that width, the displayed image width is 1280px.

At present, the sizes attribute for the inserted image reads:

sizes="(max-width: 1600px) 100vw, 1600px"

A more optimized sizes attribute would read:

sizes="(max-width: 767px) 723px, (max-width: 1167px) calc(8 * (100vw / 12)), (min-width: 1168px) calc(6 * (100vw / 12)), 100vw"

In this scenario, the 1024px wide image is only loaded when the browser width is 1156px, and the 1600px wide image is never loaded unless the screen is abnormally wide or at 2x resolution.

The performance gains here are significant.

The challenge here is there are a myriad of display possibilities for images within the Gutenberg paradigm, all of which must be accounted for on a case-by-case basis to ensure the browser has the right information to load the right sized image file at the right time. This has been extensively explored in Gutenberg issue 6177 and as of yet it doesn't seem to be fully resolved. See my comment for a more detailed analysis of how this could theoretically be solved.

@mor10

This comment has been minimized.

Contributor

mor10 commented Oct 29, 2018

@kjellr Based on the findings above I consider this a bug.

@kjellr

This comment has been minimized.

Collaborator

kjellr commented Oct 30, 2018

Thanks, @mor10. Happy to change this to a bug, but it sounds like we need to wait until Gutenberg allows us to specify the $max_display_area value(s), in order to properly address this, right?

@kjellr kjellr added the bug label Oct 30, 2018

@azaozz

This comment has been minimized.

azaozz commented Oct 30, 2018

@kjellr right, the idea in https://github.com/WordPress/gutenberg/tree/update/image-block is to introduce a global PHP var similarly to the current $content_width. It will have three settings: default, wide, and full (acting more like "max-width"), and hopefully both the editor and the front-end will use them when setting the widths. More info: https://github.com/WordPress/gutenberg/blob/update/image-block/packages/block-library/src/image/index.php#L26.

@LittleBigThing

This comment has been minimized.

LittleBigThing commented Nov 2, 2018

Added a PR #450 to take advantage of responsive images for the featured image on single posts. See latest update.

@mor10

This comment has been minimized.

Contributor

mor10 commented Nov 2, 2018

@LittleBigThing Nice work. I'll add in functionality in this PR to handle the featured image on index and archive pages and make sure the two don't conflict.

@LittleBigThing

This comment has been minimized.

LittleBigThing commented Nov 2, 2018

Thanks @mor10.

Do you think that featured images on index and archive pages need anything added? I haven’t checked but I suppose they are using the srcset attribute by default as the WP core functionality — added via the_post_thumbnail() — as they are not dependent from Gutenberg (I understood there were some problems there).

@LittleBigThing

This comment has been minimized.

LittleBigThing commented Nov 2, 2018

Sorry, @mor10, just read through your comment above again and the Gutenberg issue you mentioned, I think I understand now that it is a more general issue with how WP handles the sizes attribute.

@mor10

This comment has been minimized.

Contributor

mor10 commented Nov 6, 2018

Added #493 to inject correct sizes attribute on featured images on index and archive pages since these conform to the "normal" width display. Still waiting on a resolution to Gutenberg issue 6177 for the rest of the image sizes.

@mor10

This comment has been minimized.

Contributor

mor10 commented Nov 8, 2018

Until Gutenberg issue 6177 is resolved and the necessary changes are made to WordPress core, I would not recommend shipping Twenty Nineteen because of the significant performance hit imposed on visitors.

To illustrate the importance of resolving the responsive images issue before releasing this theme (or Gutenberg for that matter), take a look at this example:
screen shot 2018-11-07 at 4 27 08 pm

The original image uploaded here is 4048px wide, but the displayed width in the example is 834px. Because of how Gutenberg handles responsive images currently, the sizes attribute for the image reads as follows:

sizes="(max-width: 2560px) 100vw, 2560px"

As can be seen from the Network console, as the viewport is widened, the browser downloads new image files in accordance with the provided srcset attributes. Because of the incorrect sizes attribute, it keeps pulling down new sizes based on the viewport width up to and including the original image file which is the 5.2mb file at the bottom. This causes a significant performance hit on the visitor because unnecessarily large image files are downloaded. This is the very reason we have responsive images in WordPress.

In "classic" WordPress, this was solved on a theme-by-theme basis by hooking into the wp_calculate_image_sizes() and defining the sizes attribute to match the actual display of images as I did for Featured Images on index and archive pages in #493. However, as explained in Gutenberg issue 6131 and further in Gutenberg issue 6177 this is currently not possible.

There is currently no reasonable fallback method here: Without a resolution to 6177, we have to fall back on the current solution which forces the downloading of severely oversized image files causing a major performance hit. Setting wp_calculate_image_sizes() using the current available tools results in wide and full images being provided with inaccurate width source images which makes them useless.

Thus my opening statement: Until Gutenberg issue 6177 is resolved and the necessary changes are made to WordPress core, I would not recommend shipping Twenty Nineteen because of the significant performance hit imposed on visitors.

@mor10

This comment has been minimized.

Contributor

mor10 commented Nov 17, 2018

Added #629 with functional solution depending on merge of WordPress/gutenberg#11973.

Left/right aligned images and galleries remain untested.

@mor10

This comment has been minimized.

Contributor

mor10 commented Nov 29, 2018

A request has been made for a test to show current and corrected behavior, so I've built them both:

The two posts linked below show the current output from core and a corrected version respectively. Note the detailed instructions on how to test this. Responsive images are a core browser feature and browsers work very hard to make the functionality invisible. Forcing visibility requires being rather heavy handed in your testing.

Take special note of the following: Responsive images are impacted by display density. When you do testing, make sure you test for both 1x and 2x displays. This can be done using the mobile preview in your browser dev tools.

@mor10

This comment has been minimized.

Contributor

mor10 commented Dec 2, 2018

It looks unlikely that the larger problem of passing block attributes down to the wp_calculate_image_sizes filter will be resolved in time for 5.0. As an emergency patch, I've added #701 which fixes the sizes attribute for regular aligned images. This is the bare minimum required as the output from WordPress core incorrectly states the maximum displayed width of a regular image is 1024px while in the Twenty Nineteen theme the width can be any other value as the width is fluid.

The PR follows the pattern used in several other default themes including Twenty Seventeen: https://github.com/WordPress/WordPress/blob/master/wp-content/themes/twentyseventeen/functions.php#L491-L517

I strongly recommend merging this PR before 5.0 release to at least stay on par with previous default themes.

cc @joemcgill

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment