Make WordPress Core

Opened 5 years ago

Last modified 2 years ago

#45407 assigned defect (bug)

Add block attributes to `wp_calculate_image_sizes` to allow for proper handling of `sizes` attribute

Reported by: mor10's profile mor10 Owned by: joemcgill's profile joemcgill
Milestone: Future Release Priority: high
Severity: major Version:
Component: Media Keywords: dev-feedback needs-patch
Focuses: Cc:

Description (last modified by azaozz)

Related Gutenberg issues:

Related Trac tickets:

Problem

For images with no alignment or images where the image block is aligned alignwide and alignfull, the sizes attribute reads:

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

This means regardless of the displayed width of an image, the largest image file pulled by the browser is the generated 1024px image. If the displayed image width is wider than 1024px, the browser will stretch the image to compensate causing blur and artifacts.

Historically this was solved by theme developers by augmenting the sizes attribute using the wp_calculate_image_sizes filter.

The new alignwide and alignfull widths cannot currently be accommodated using this filter because the filter does not contain information about the alignment value of the containing block.

Proposed solution

Modify the wp_calculate_image_sizes filter to include a new fifth parameter $block_attr containing an array which includes [align] which holds the alignment value for the containing block.

This is a stop-gap measure as we wait for Gutenberg PR 11973 https://github.com/WordPress/gutenberg/pull/11973 to merge.

Why this is a blocker

From my perspective, this is a blocker to 5.0 for three main reasons:

  1. As stated earlier, the current implementation of images means images with no alignment or images where the image block is aligned alignwide and alignfull are supplied with an incorrect sizes attribute by core. Theme developers cannot fix this because they have no way of filtering for different block widths. The end result is the visitor experiences blurry and/or artifact-heavy images which will be interpreted as an error. No theme (Twenty Nineteen included) handles responsive images correctly at the moment meaning all "Gutenberg-ready" themes with wide and full image options will display blurry/artifact-full images.
  2. A solution to this problem must be one which stands the test of time. Once a solution is introduced, every theme (including core themes) must be updated. Shipping an interim solution which will later be changed puts an undue burden on theme developers and will impose legacy debt on WordPress Core to provide support for the interim solution in the long term. The proposal to add $block_attr with the [width] value to the wp_calculate_image_sizes filter is a mini version of what is proposed in Gutenberg PR 11973 and will be forward-compatible to more advanced images handling by themes.
  3. 5.0 is shipping to millions of sites world wide. WordPress has a responsibility to follow web standards and use them correctly. The current implementation of the sizes attribute is insufficient and causes incorrect behavior for the end-user.

Further context

Attachments (1)

45407.diff (7.9 KB) - added by azaozz 5 years ago.

Download all attachments as: .zip

Change History (51)

This ticket was mentioned in Slack in #core-themes by mor10. View the logs.


5 years ago

#2 @azaozz
5 years ago

  • Description modified (diff)

@azaozz
5 years ago

#3 @azaozz
5 years ago

In 45407.diff: add an optional param $block_attributes to the srcset and sizes functions and pass it to the corresponding filters (will be used after https://github.com/WordPress/gutenberg/pull/11973 is merged).

#4 follow-up: @desrosj
5 years ago

  • Milestone changed from Awaiting Review to 5.0

Moving this to the 5.0 milestone for visibility because the PR linked by @azaozz is slated for version 4.6 of the plugin (before 5.0).

This ticket was mentioned in Slack in #core by mor10. View the logs.


5 years ago

This ticket was mentioned in Slack in #core by mor10. View the logs.


5 years ago

#7 @mor10
5 years ago

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.

This ticket was mentioned in Slack in #core-media by mor10. View the logs.


5 years ago

#9 @karmatosed
5 years ago

  • Milestone changed from 5.0 to 5.0.1

#10 in reply to: ↑ 4 @azaozz
5 years ago

Replying to desrosj:

Right, https://github.com/WordPress/gutenberg/pull/11973 was moved to Gutenberg 4.7. This needs to go in after the PR (or part of it) is merged :)

Also see https://github.com/WordPress/gutenberg/issues/6177#issuecomment-442953527 for the long term solution.

Last edited 5 years ago by azaozz (previous) (diff)

#11 @pento
5 years ago

  • Milestone changed from 5.0.1 to 5.0.2

#12 @pento
5 years ago

  • Milestone changed from 5.0.2 to 5.0.3

#13 @mor10
5 years ago

Can someone explain why this keeps getting bumped? Responsive images are still broken for all sites with support for wide images and theme developers can't provide proper instructions to the browser.

If the blocker is the size and complexity of 11973, let's isolate just the passing of the block alignment to the filter and get this particularly ugly hole patched as we work on a better solution for images and media long term.

#14 @audrasjb
5 years ago

  • Keywords dev-feedback added

Hi,

This ticket is triaged in milestone 5.0.3 with severity: blocker. If the patch is ok, it will needs commit and backport to the related branch. Adding dev-feedback keywords to see if it can land in 5.0.3 at the very beginning of January.

Last edited 5 years ago by audrasjb (previous) (diff)

This ticket was mentioned in Slack in #core by desrosj. View the logs.


5 years ago

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


5 years ago

This ticket was mentioned in Slack in #core-editor by mike. View the logs.


5 years ago

#18 @kirasong
5 years ago

I reached out in #core-editor (linked above), then in https://github.com/WordPress/gutenberg/pull/11973 as a followup, as I noticed that the PR is not merged and does not have a milestone.

As far as I can tell, some way for the editor to provide the attributes (whether the above PR, or a different approach) will still need to land in Gutenberg before the core patch on this ticket will be helpful.

#19 follow-up: @youknowriad
5 years ago

I suggest we stop thinking about stop-gap fixes and think about a more future-proof solution that doesn't involve tweaks from the theme.

Some ideas:

  • Remove the image size picker
  • Add a new default image size in WordPress (big one)
  • A way for themes and block containers to define which size to use by default for each alignment option. For example, when a user makes an image "wide", the default "src" used switches to the big image size which means the filter will adjust the sizes attribute accordingly.

#20 @ocean90
5 years ago

  • Milestone changed from 5.0.3 to 5.1
  • Severity changed from blocker to major

Punting since there doesn't seem to be a final decision made on the proper solution.

#21 in reply to: ↑ 19 @mor10
5 years ago

Replying to youknowriad:

I suggest we stop thinking about stop-gap fixes and think about a more future-proof solution that doesn't involve tweaks from the theme.

As I've explained before, the current code shipped is incorrect and causes browsers to use the wrong image sources for different displays. This is non-trivial and deserves to be resolved immediately. The current methodology for handling responsive images is controlling the sizes attribute via themes. This has been the standard method since 2015 and it is one employed by all default themes including Twenty Nineteen.

While I agree this is a sub-optimal solution, it is the status quo. What I'm asking for is to get core updated so the current recommended methodology works correctly. As stated, it does not at the moment for all the reasons listed earlier in the ticket.

Some ideas:

  • Remove the image size picker

Not related to this ticket.

  • Add a new default image size in WordPress (big one)

Yes,

  • A way for themes and block containers to define which size to use by default for each alignment option. For example, when a user makes an image "wide", the default "src" used switches to the big image size which means the filter will adjust the sizes attribute accordingly.

Not a viable solution to the current problem because themes must have a way of defining, per layout, what those widths are.

Responsive images markup (srcset and sizes) does not control the layout of the page, it informs the browser what source file to load based on the eventual displayed width in advance of paint so the browser doesn't download an incorrectly sized image for the layout once CSS is rendered. Thus the information about layout width must come from the theme since the theme is the only code with knowledge of the eventual width of any one image.

#22 @mor10
5 years ago

@youknowriad I wrote up a breakdown of why knowing the alignment property of the container is essential for themes, complete with visuals and code examples, a while back in a Gutenberg ticket. What you're talking about in the last point on your list is something similar to what I proposed back then:

https://github.com/WordPress/gutenberg/issues/6177#issuecomment-400095305

#23 @youknowriad
5 years ago

Good thoughts @mor10 I missed this originally.

I'd add that ideally, it's not the responsibility of the theme or plugin to control the output but the theme should be able to provide the necessary information (like you suggested) to the editor in order to produce the correct output.

I think there are more use-cases than the ones you highlighted, If you consider a sidebar block, and a main content block side by side. You'd end up with questions like:

  • Do we allow wide alignments in the sidebar area?
  • How can we define with alignments are allowed in which areas?
  • What is the default image size used for each alignment in each area?

So coming up with the API that answers all these use-cases is not an easy task. A suggestion could be that each InnerBlocks area has a config prop that could look like this:

{
   alignments: {
      // available alignment => default image size
      left: "large",
      right: "large",
      wide: "very-large"
   }
}

Now the question remains, how does a theme provides this config to the global canvas for a post type (which may not make sense anymore in phase2, or make less sense), and how does a theme extends this prop for a given block.

If there's someone willing to explore these, please go for it.

Last edited 5 years ago by youknowriad (previous) (diff)

#24 @mor10
5 years ago

@youknowriad The new layouts introduced with Gutenberg, and the potential for other layouts once phase 2 gets off the ground, are defining the very edge of the RICG responsive images spec, and in many cases pushing the spec beyond its limits. This is an ongoing conversation I'm having with people outside the WordPress community as well as here. It is also not relevant to this ticket.

This ticket addresses a specific bug introduced with Gutenberg which renders the html output of 100% of sites displaying wide and full aligned images incorrect. The proposed solution builds on existing methodology in themes which is in use across the ecosystem including in default themes, and can easily be made backwards compatible to older versions of WordPress.

While it is true the future of layouts in WordPress will require a complete rethink of media, and that there is a good chance WordPress will be the proving ground for rethinking media for the web platform, these discussions are all in their infancy and any resolution is in the future. The bug flagged here is present, currently active on every WordPress site running 5.0.x, and is directly impacting users in the real world.

This bug can and should be fixed immediately so we can move on and discuss the larger question of how media should be handled in WordPress and on the web in the future.

#25 @joemcgill
5 years ago

  • Priority changed from normal to high

#26 @joemcgill
5 years ago

  • Owner set to joemcgill
  • Status changed from new to assigned

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


5 years ago

This ticket was mentioned in Slack in #core-media by desrosj. View the logs.


5 years ago

#29 @joemcgill
5 years ago

  • Milestone changed from 5.1 to 5.2

The approach in 45407.diff requires image blocks to be rendered on the server rather than directly using the markup stored in post content. Since that part has not been implemented in Gutenberg yet—and might not—I'm going to punt this for now.

Another potential approach would be to modify the regex in wp_make_content_images_responsive() so it includes containing figure elements and pass alignment data as a new parameter to wp_image_add_srcset_and_sizes(), which can then be added to the filter for sizes.

#30 @azaozz
5 years ago

Another potential approach would be to modify the regex...

Yeah, looked at that too but it would be a (hacky) fix on top of another hacky fix (parsing HTML with regex) on top of another... :)

Yet another possible approach would be to not regenerate all the image block HTML, just run it through a filter with all the extra image data as params. But then a theme or a plugin will still have to regenerate it or parse it and replace attributes with regex.

There is even an option to add the available image data at time of editing as attributes to the <img> tag and leave it all to themes/plugins to handle. Somehow not thinking that'd be good.

Thinking best would be to take a step back and reevaluate the scope of this ticket. We have the time now, it is for WP 5.2 :)

#31 @bjf2000
5 years ago

How wide an issue is this? Or how can one at least spot-check key images beforehand for a given site not yet on 5.x?

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


5 years ago

#33 @antpb
5 years ago

  • Milestone changed from 5.2 to 5.3

In the recent Media meeting, we discussed this ticket and determined since the approach is still being discussed on how to move forward, we will be moving this out of the 5.2 milestone and into 5.3. Happy to move this back if anyone has a proposed patch.

Last edited 5 years ago by antpb (previous) (diff)

#34 @mor10
5 years ago

The underlying issues around the responsive images spec and how browsers handle high resolution screens is now being discussed by the WHATWG in this ticket: https://github.com/whatwg/html/issues/4421

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


4 years ago

This ticket was mentioned in Slack in #core by pento. View the logs.


4 years ago

#37 @kirasong
4 years ago

We chatted about this ticket in today's triage.

It's my understanding that this change still depends on https://github.com/WordPress/gutenberg/issues/6177 or similar change in Gutenberg.

Is that the case? Either way, are you planning on doing any additional work here for 5.3?

This ticket was mentioned in Slack in #core-media by mike. View the logs.


4 years ago

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


4 years ago

This ticket was mentioned in Slack in #core by peterwilsoncc. View the logs.


4 years ago

This ticket was mentioned in Slack in #core-media by mike. View the logs.


4 years ago

#42 @kirasong
4 years ago

  • Keywords needs-patch added
  • Milestone changed from 5.3 to Future Release

Since there hasn't been any movement on this recently, and 5.3 RC is happening shortly, going to move this to Future Release.

Feel free to move to 5.4, or back in if something is ready to commit.

#43 @peterwilsoncc
4 years ago

Related (potential duplicate, can't decide) #49966

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


3 years ago

#45 @antpb
3 years ago

  • Milestone changed from Future Release to 5.8

This seems to be a high impact ticket that many folks are asking about. Image sizes in general are maybe in need of additional context when used in blocks such as Galleries. @skorasaurus was kind enough to summarize many related issues in this gist: https://gist.github.com/skorasaurus/a01249d4302226bf12c80dd979322303

From discussions in the media meeting, we think that this ticket might be the best starting place to making it easy for components such as a gallery to provide context for the sizes it needs. Marking 5.8 so we can investigate and keep discussions rolling through the cycle. This is not so much an indicator that it will be solved in this cycle.

#46 @JeffPaul
3 years ago

@antpb what's the latest on this analysis and do you think it'll have any portions that land in 5.8?

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


3 years ago

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


3 years ago

#49 @JeffPaul
3 years ago

  • Milestone changed from 5.8 to Future Release

With 5.8 Beta 1 next week and no patch/PR on this ticket, I'm going to punt this to Future Release. Once a patch/PR is available this ticket can be added back to a numbered milestone.

This ticket was mentioned in Slack in #core-media by adamsilverstein. View the logs.


2 years ago

Note: See TracTickets for help on using tickets.