Update/image block #11377

azaozz · 2018-11-01T21:01:02Z

Fixes and updates to handling images in the image block.

There is only one small UI change: the "Image Size" drop-down in the inspector is reduced to only show "Default" and "Thumbnail" sizes, plus any cropped sizes added by themes and plugins. The reason is that selecting an image by file size is a thing from the past. All available sizes will be in the srcset attribute in the <img> tag, so the browser is not constrained to a specific file.

Another change is that the 25, 50, 75, 100% buttons work properly (visually) now. Before they were calculated from the image file dimensions. As that was usually a huge image/photo changing them had no effect at all. Now they are based on the actual block width, so setting several different images to 50% width will properly make them the same size.

Another somewhat visible change is that images inside the editor support high-density screens as there are srcset and sizes attributes.

The under-the-hood changes are a lot more:

The "large" image size is used by default. This fixes a regression where all images were always loading at full size (often 3-4MB), both in the editor and the front-end.
The image block is now dynamically rendered on the front-end.
There are always width and height attributes on the front.
Images can be scaled when the theme doesn't match the editor width. For example a 50% wide image in the editor will always display properly at 50% wide on the front-end in any theme (as long as the theme sets $content_width or the new $block_width).
The srcset and sizes attributes are properly calculated for all images (after the width and height are determined.
There is more data passed in the block attributes allowing much more precise rendering on the front-end. Once this is merged, we will be able to pass the $block_attributes array to the wp_calculate_image_srcset and wp_calculate_image_sizes filters allowing generation of much better values by the themes.

See #6177.

- Add `srcset` and `sizes` inside the editor. - Add `data-wp-attachment-id` and `ata-wp-percent-width` to the img tag. Will be used on the front-end to properly set `srcset` and `sizes`, and also img `width` and `height` where missing. - Improve setting/resetting the image dimensions.

Add `srcset` to the attachment data from the API and to the data in the media modal.

See #6177.

…d warnings after ignore!)

Most functionality works as expected. Still todo: image dimensions by entering width or height. These need to override the `scale` attribute.

- Don't save srcset and sizes in block props. - Better names? - Clean up few eslint-disable-*.

Tweak some names, clean up few things, simplify the php parts, fall back to the original block html if some image data is not there.

- Better handling of existing (old) image blocks and just uploaded images. - Add support for scaled external images.

Added few more block props that are passed to the PHP filters in WP. Now themes and plugins can easily decide how to handle all aspects of rendering the image block on the front-end.

ZebulanStanphill · 2018-11-01T21:08:10Z

.eslintrc.js

@@ -181,6 +181,8 @@ module.exports = {
 				};
 			} ),
 		} ],
+		// TODO: removeme, maybe? Without this linting fails horribly on Windows, many thousands of false positives :)
+		// 'linebreak-style': 'off',


Is this supposed to be part of this PR?

Not really. Still work-in-progress though, will do the TODO :)

ZebulanStanphill · 2018-11-01T21:11:01Z

packages/block-library/src/image/index.php

+				),
+				'align' => array(
+					'type' => 'string',
+					'enum' => array( 'center', 'left', 'right', 'wide', 'full', '' ),


From my testing in my own PRs, the empty '' is unnecessary.

Yeah, probably. However it makes that a bit more readable, i.e. all "possible values" are listed (so somebody looking at that place in couple of years won't think it's weird/buggy) :)

This was actually copied from https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/latest-comments/index.php#L177.

Technically, '' isn't actually a value that the attribute ever gets in practice. It technically gets null when no value is set, right? I mean, due to JavaScript type coercion, those 2 values end up being treated the same, but if you're going to put it there to make things clearer, I think putting null there would make more sense.

But by that logic, almost every instance of enum should include null or '' when the attribute is optional for the block. I don't think it makes the code more readable; I think it actually createst confusion, because it implies that you can disallow an empty string or null value by removing it from the enum, which is not the case. Essentially, it's dead code that has no purpose, but implies that it does by existing.

I actually made #9832 to remove the '' from the Latest Comments block a while back.

lib/load.php

azaozz · 2018-11-06T12:52:29Z

a few file permission changes that have accidentally been committed here

Yeah, that's pretty weird though, not sure where these come from as I'm on Windows, perhaps some git "internals"? Looking there, do we really want 755 on .js files? Thought (source) files should not have the exec bit set, i.e. these files actually need 644?

Hmmm, these permission changes seem to -sometimes- happen when I merge master..? Even weirder than before :)

azaozz · 2018-11-09T12:41:10Z

This (experimental) PR introduces quite a few changes. Been thinking how we can split it to make it a bit more manageable :)

The larger changes are:

Use "large" size when adding local images. This is a must-have fix, otherwise we force everybody to download huge images.
Add srcset and sizes attributes inside the image block in the editor. This is (very) nice to have as it supports "retina" images. Currently it works well but can be implemented later if there are any problems.
Ensure all images have width and height attributes on the front-end. This is essential if we want the images to be "responsive" (i.e. srcset and sizes won't work if we don't have width and height).
Implements a way for the theme to set the three block widths (default, wide, full) in CSS, PHP and JS and in the editor and the front-end. This is another essential feature. It would also let images be scaled on the front-end according to the theme's "block width" and maintain image dimensions when themes are switched. This is more critical now that we have three widths, not just content_width. See content_width and new block widths #5650.
To support the above four, make the image block "dynamic" on the front-end. That also needs some more data about the images to be passed in block attributes. This is a "no-brainer", and will allow generation of much more precise sizes attributes on the front-end by the themes.
With the above 5 in place, selecting individual image files in the inspector doesn't make sense any more. The available sizes were reduced to "Default", "Thumbnail" and possibly any custom crops added by plugins and themes. This is another "nice to have" as it moves away from '98s image handling. If that's not added, we would need a workaround for when the user selects a small image. Likely we will need to always set the height/width inside the editor.
The "resize to 25%, 50%, 75%, 100%" was fixed to work "visually". Currently it fails most of the time as it sets the dimensions according to the file dimensions, i.e. it makes no difference at all as most images are very large. Now it works by resizing the image to the percentage according to the block width, i.e. when two images are set to 50%, they have exactly the same width which is 1/2 of the block width, so images can be made to line-up. This is essential bug fix, probably should make a separate issue for it.
There are small fixes to the "resize by pixels" and "reset" button. Not essential but nice to have, can be considered "UI polishing".

As far as I see 1, 3 and 4 are "blockers". To implement 3 and 4 we will need 5. Once we have 1, 3, 4, and 5 in place the rest would be pretty easy to add.

azaozz · 2018-11-09T13:24:47Z

Discussing how to best handle this (thanks @catehstn), perhaps we can do 1 and 5 in parallel in separate PRs, then add 3 and 4 as they are essential for continuing to support responsive images on the front-end. Then 2, 6 and 8 can be added easily with everything else in place, and I'll make separate issue for 7.

@mtias does that sound ok? :)

mor10 · 2018-11-09T16:56:54Z

I recommend fast-tracking the components that make theme integration possible so we can get it baked into Twenty Nineteen and tested at scale. Once we know it works, the functionality needs to be communicated to all theme developers to ensure themes are handling responsive images properly. This is especially the case for any theme claiming to be "Gutenberg ready".

I have time set aside to submit PRs for Twenty Nineteen to get this sorted and to write an exhaustive article / documentation on how to get this right on the front end to avoid massive performance hits across the web because of a WordPress update.

azaozz · 2018-11-09T17:43:54Z

@mor10 sure, think we can handle that on #5650. The "example" code is: https://github.com/WordPress/gutenberg/pull/11377/files#diff-ad1f24a74f64a1c137bfb28da9349bd1R30 but instead of a global better to use a function or two, and "normalize".

So themes could do

// Check context...
set_block_width(
    array (
        'default' => 600,
        'wide' => 780,
        'full' => 1024,
    )
);

and we will store that "somewhere". Then have a get_block_width() to retrieve it and filter it.

Of course this will also need some JS and CSS to set these widths globally in the editor.

mor10 · 2018-11-09T18:10:17Z

@azaozz I'm not clear on how this would work for the sizes attribute in practice. Twenty Nineteen is a great example of this: All image widths, including the "regular" width, are fluid and can only be defined using relative values and calc. Or am I misunderstanding you?

joemcgill · 2018-11-09T22:56:53Z

@azaozz thanks for breaking this down, it's very helpful, and I agree with you that 1, 3, and 4 are blockers. I'm not sure that 5 is necessary, because the responsive filters on the_content will still work without making the image block dynamic. That said, it certainly seems like a really nice approach that could have several other benefits (see #8472).

I have some time to dedicate towards moving these pieces forward. Are there any that you would prefer I focus on?

chrisvanpatten · 2018-11-09T23:46:24Z

Re item 5, #11523 might be an entry point to being able to filter the image block's output without making it explicitly a dynamic block.

azaozz · 2018-11-10T16:29:58Z

@chrisvanpatten yep, good catch, thanks for linking that. If block attributes are also passed to the filter proposed in #11523, there will be no need to make the image block dynamic.

@joemcgill # 5 is needed if we want # 3. Also, we need to pass some more data from the editor to the front-end and the best way to do that is in block attributes (block props). Currently this PR makes the image block dynamic so that mechanism works, but if the filter from #11523 is implemented (and gives access to block attributes), we won't need that.

I have some time to dedicate towards moving these pieces forward.

That's great! IMHO the most important thing that we need to get right is how to best calculate the sizes attribute taking into account the theme's "block width" on the front-end (# 4). This also touches on @mor10's comment above: "what's the best way for the theme to specify block width".

Currently we always look at the image width in pixels (as the image file dimensions are in pixels). This lets us calculate a "compatible" sizes attribute based on the width attribute from the image tag. However that can be a lot better/more precise. For now (in this PR) that precise sizes attribute is still left for the themes to implement, we only make sure there are (expected) width and height values.

Additionally (in this PR) the image tag width and height attributes can be set dynamically based on image dimensions and the block width inside the editor while the post was edited. This ensures we always have a width as this is not set in the editor when images have not been resized there. This also allows scaling of resized images when the block width inside the editor differs from the block width on the front-end.

What `<img>` tags we want to have

(definitely on the front-end, preferably in the editor)

We should still follow the (old) best practice of having width and height attributes. This prevents content from jumping after images are downloaded (very noticeable on slower connections).
Ideally the sizes attribute will have more precise sizing and break points. This will have to be supplied by the theme, possibly in a block_width array and used in the editor and on the front-end. It should define the three possible block sizes: default, wide and full.
Need to support resizing of the image in the editor by dragging. That can be handled as "relative width" and scaled on the front-end if needed.
Need to support direct setting width/height (in pixels) in the editor. This is an exception where we must ensure we honor these attributes as set by the author.
Need to support images where the "full size" is smaller than the block width.

GlennMartin1 · 2018-11-13T20:56:42Z

Could this PR be milestoned 4.4?

joemcgill · 2018-11-15T20:29:41Z

@azaozz re:

That's great! IMHO the most important thing that we need to get right is how to best calculate the sizes attribute taking into account the theme's "block width" on the front-end (# 4). This also touches on @mor10's comment above: "what's the best way for the theme to specify block width".

The intention for the default sizes attribute has always been to set a default based on the image width (or content width/whichever is smaller) and that it would be overridden by themes.

I think the best way for themes to handle this would be for us to calculate sizes for an image block dynamically in a render callback where we can pass the block attributes (specifically, alignments) as you've described.

aduth · 2018-11-15T21:06:10Z

packages/block-library/src/image/index.js

 	linkDestination: {
 		type: 'string',
 		default: 'none',
 	},
+	srcSet: {


"for the editor's sake" is not something I think we should want as a reason for an attribute to exist. As I see it, the attributes should contain the minimal normalized data to be able to reconstruct the image in any context. fileWidth, fileHeight, srcSet, sizes are redundant if the id is already known.

aduth · 2018-11-15T21:09:13Z

packages/block-library/src/image/index.php

+function _gutenberg_cache_images_meta( $content ) {
+	// Need to find all image blocks and get the attachment IDs from them BEFORE the parser is run
+	// so we can get the image attachments meta all at once from the DB.
+	if ( preg_match_all( '/^<!-- wp:image {.*"id":(\d+),.*} -->$/m', $content, $matches ) ) {


Is this something where being able to hook to the parse (#10108) would prevent us from needing to try to match via a regular expression?

To cache the attachments meta we will need to know all attachments IDs before the meta is needed. The point is to do one call to the DB for all the meta data, not multiple calls. We would need to process all "image" blocks, all "media and text" (when an image is used), "gallery", and "cover".

I know it's somewhat hacky to use a regex, but not sure we can get all these IDs before callbacks have started/blocks are processed.

Maybe then in addition to block_pre_render it would be useful to have a filter which occurs immediately following the do_blocks parse, before the loop to render each individual block.

cc @dmsnell

fileWidth, fileHeight, srcSet, sizes are redundant if the id is already known.

Well, you'd think so but images (and image meta) is "the land of thousand edge cases" :) Example: the website was "exported" and then imported into another site. Now all attachment IDs stored in post_content are "wrong". (Well in this case WP will also fail to generate the srcset).

It would be beneficial to have some more info/meta about the image from the moment it was added to the post. Also, that somewhat helps with external images.

The srcSet and sizes would be pretty nice to have inside the editor but not essential for now. I'm almost ready with the next chunk split off this PR, there they are not used. However it has fileWidth, fileHeight, userWidth, userHeight for when the user set these manually (can reuse width and height and just add one prop indicating this), and editWidth that lets the theme scale images according to the width of the block in the editor when the post was created. Of course, none of these are set in stone, lets move the discussion to that pr (I'll ping you there).

in addition to block_pre_render it would be useful to have a filter which occurs immediately following the do_blocks parse, before the loop to render each individual block.

this is how I imagined we would do things like what's being discussed here. if we want to do post-wide operations or side-effecting transformations then we can filter each block and hold state in the filter function with a static variable or class property, then a closing operation at the end. I hadn't thought about placing that last filter though in this context, so thanks for pointing it out - I'll add it to #10108

aduth · 2018-11-15T21:27:58Z

Trying to digest this and #6177, there's a lot said about what should be shown on the front-end, what should be shown in the editor. I don't see so much about what's actually saved to disk (the content in the database). It's significant because the front-end and editor both have means to reconstruct what should be shown (by render_callback and edit, respectively). Thus, the saved content should be the minimal essence of the block necessary to enabling that. What we have here with the render callback seems like a good direction, though I'd not expect to see as many (any?) attribute changes to the block.

And if the primary concern is what's shown on the front-end, could we focus on that and disregard what's shown in the editor for now?

azaozz · 2018-11-15T22:11:54Z

the saved content should be the minimal essence of the block necessary to enabling that. What we have here with the render callback seems like a good direction, though I'd not expect to see as many (any?) attribute changes to the block.

Right. As image meta can be somewhat inconsistent and even change after the post is published, the idea is to store some data about the image while the post is created. Ideally that will be enough data so the theme could "calculate" a more precise sizes attribute. The srcset will be generated from the available image meta when the post is displayed.

gziolo · 2019-02-04T13:02:56Z

This didn't get to WordPress 5.0 and 5.1. What are the next steps planned, should it be refreshed? @youknowriad and @mapk any thoughts?

paaljoachim · 2019-09-09T05:25:15Z

Hey @azaozz and others.

Can we get a status update to how this PR is doing? Let us know how we can help to move it onward. Thanks.

bengreeley · 2019-11-26T22:23:07Z

Any updates on this one? Noticing some strange issues with the image block and the crops/image sizes that are being used

youknowriad · 2020-03-18T11:53:09Z

Trying to triage PRs today. It seems that this PR is stale. I'm going to close it, for now. Thanks all for your efforts. We should consider opening smaller targetted PRs to fix the existing image issues.

azaozz and others added 26 commits June 11, 2018 19:28

Update image block, add srcset to attachment data

4b3d889

Add `srcset` to the attachment data from the API and to the data in the media modal.

Image block: fix docs

be16a73

See #6177.

Merge master

041d1a5

Fixes after merging from master. (NOTE: JSHint still throws errors an…

64648c1

…d warnings after ignore!)

Merge branch 'master' into origin/update/image-block

28d9315

Merge branch 'master' into origin/update/image-block

0a73bc9

Fixes after update

1d0d17e

Use scale attribute instead of width and heightwq

dbba831

Merge branch 'master' into update/image-block

b6dc78f

Merge branch 'master' into update/image-block

108ea54

Interim - img block

22207ba

Most functionality works as expected. Still todo: image dimensions by entering width or height. These need to override the `scale` attribute.

Merge branch 'master' into update/image-block

4cb8189

In progress

ae95eab

- Don't save srcset and sizes in block props. - Better names? - Clean up few eslint-disable-*.

Merge branch 'master' into update/image-block

d9ef233

Almost complete!

0aea270

In progress

a476203

Tweak some names, clean up few things, simplify the php parts, fall back to the original block html if some image data is not there.

Merge branch 'master' into update/image-block

8d37181

Merge branch 'master' into update/image-block

9d0899e

In progress

22007e6

- Better handling of existing (old) image blocks and just uploaded images. - Add support for scaled external images.

Merge branch 'master' into update/image-block

5b5be6f

Complete?

5525986

Added few more block props that are passed to the PHP filters in WP. Now themes and plugins can easily decide how to handle all aspects of rendering the image block on the front-end.

Merge branch 'master' into update/image-block

83ca683

Merge branch 'master' into update/image-block

15520ea

A bit of cleanup

17c5563

Merge branch 'master' into update/image-block

6a98975

azaozz added the [Block] Image Affects the Image Block label Nov 1, 2018

ZebulanStanphill reviewed Nov 1, 2018

View reviewed changes

lib/load.php Outdated Show resolved Hide resolved

designsimply mentioned this pull request Nov 6, 2018

Image Block doesn't display image 'full size' even though this is the default #11529

Open

azaozz mentioned this pull request Nov 9, 2018

content_width and new block widths #5650

Closed

azaozz mentioned this pull request Nov 9, 2018

Image Dimension controls on the Image Block don't work #11530

Closed

azaozz mentioned this pull request Nov 9, 2018

Always prefer "large" image size #11682

Merged

chrisvanpatten mentioned this pull request Nov 9, 2018

Image resizing issues #11702

Closed

azaozz mentioned this pull request Nov 10, 2018

Apply a filter to each rendered static and dynamic block #11523

Closed

nickcernis mentioned this pull request Nov 13, 2018

Gutenberg: set block_width global studiopress/genesis-sample#132

Open

azaozz mentioned this pull request Nov 14, 2018

Fix issues with image initial size #11846

Merged

aduth reviewed Nov 15, 2018

View reviewed changes

azaozz mentioned this pull request Nov 16, 2018

Add more data about the image as block props in the image block #11973

Closed

gziolo added the Needs Decision Needs a decision to be actionable or relevant label Feb 4, 2019

youknowriad closed this Mar 18, 2020

youknowriad deleted the update/image-block branch May 27, 2020 17:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update/image block #11377

Update/image block #11377

azaozz commented Nov 1, 2018

ZebulanStanphill Nov 1, 2018

azaozz Nov 1, 2018

ZebulanStanphill Nov 1, 2018 •

edited

Loading

azaozz Nov 2, 2018

ZebulanStanphill Nov 2, 2018

azaozz commented Nov 6, 2018 •

edited

Loading

azaozz commented Nov 9, 2018 •

edited

Loading

azaozz commented Nov 9, 2018 •

edited

Loading

mor10 commented Nov 9, 2018

azaozz commented Nov 9, 2018

mor10 commented Nov 9, 2018

joemcgill commented Nov 9, 2018

chrisvanpatten commented Nov 9, 2018

azaozz commented Nov 10, 2018 •

edited

Loading

GlennMartin1 commented Nov 13, 2018

joemcgill commented Nov 15, 2018

aduth Nov 15, 2018

aduth Nov 15, 2018

azaozz Nov 15, 2018

aduth Nov 15, 2018

azaozz Nov 15, 2018

dmsnell Nov 16, 2018

aduth commented Nov 15, 2018

azaozz commented Nov 15, 2018

gziolo commented Feb 4, 2019

paaljoachim commented Sep 9, 2019

bengreeley commented Nov 26, 2019

youknowriad commented Mar 18, 2020

Update/image block #11377

Update/image block #11377

Conversation

azaozz commented Nov 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZebulanStanphill Nov 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azaozz commented Nov 6, 2018 • edited Loading

azaozz commented Nov 9, 2018 • edited Loading

azaozz commented Nov 9, 2018 • edited Loading

mor10 commented Nov 9, 2018

azaozz commented Nov 9, 2018

mor10 commented Nov 9, 2018

joemcgill commented Nov 9, 2018

chrisvanpatten commented Nov 9, 2018

azaozz commented Nov 10, 2018 • edited Loading

What <img> tags we want to have

GlennMartin1 commented Nov 13, 2018

joemcgill commented Nov 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented Nov 15, 2018

azaozz commented Nov 15, 2018

gziolo commented Feb 4, 2019

paaljoachim commented Sep 9, 2019

bengreeley commented Nov 26, 2019

youknowriad commented Mar 18, 2020

ZebulanStanphill Nov 1, 2018 •

edited

Loading

azaozz commented Nov 6, 2018 •

edited

Loading

azaozz commented Nov 9, 2018 •

edited

Loading

azaozz commented Nov 9, 2018 •

edited

Loading

azaozz commented Nov 10, 2018 •

edited

Loading

What `<img>` tags we want to have