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

[azaozz]

This is the second "chunk" split from #11377. It contains:

1. Adds fileWidth, fileHeight, editWidth, userSet block attributes. These would allow us to add width and height, and the themes to generate better sizes attributes on the front-end.
   • fileWidth and fileHeight are the image file dimensions at the time when the image was inserted. For "local" images they can also be "extracted" from the image meta, however there are edge cases where that meta may have changed since the post was published. Also needed for non-resized external images where we don't know the width and height.
   • editWidth holds the block width inside the editor at the moment the post was created/edited. Can be used to scale images on the front-end when the block widths don't match (in themes with fluid width or after changing the theme).
   • userSet indicated when the user has set the width and height manually. In these cases the theme should (probably) honor the settings when scaling.
2. Fixes the 25, 50, 75, 100% buttons to scale the image relative to the block width. This makes it "visual". Works well in combination with editWidth when scaling images on the front-end. Also images "line-up" when setting them to the same scale.
3. Fixes resizing by dragging to prevent upscaling.
4. Fixes direct input resizing to display values only when they were entered by the user.
5. Makes the image block non-dynamic (compared to Update/image block #11377) and uses the new render_block filter to add attributes to the img tag on the front-end, including srcset and sizes.

[mor10]

I'm trying to test this but am having no luck. Probably doing something stupid or wrong:

function twentynineteen_calculate_image_sizes_attr( $image_attributes, $block_attributes ) {
	var_dump($image_attributes);
	var_dump($block_attributes);
}

add_filter( 'render_block_core_image_tag_attributes', 'twentynineteen_calculate_image_sizes_attr', 10, 2 );

This is just a rough test to see what is contained within the attributes so I can build my conditions. Running the above code in WP 4.9.8 and this fork does nothing. Various permutations have not resulted in anything happening when calling the render_block_core_image_tag_attributes filter.

Like I said, probably doing something wrong. Please advise.

[azaozz]

Uh, sorry, should have explained better. All the code in lib/compat.php (the render_block_core_image_tag_attributes filter is there) runs off the new render_block filter here. That filter is in the 5.0 branch, was added ~ a week ago. Please test in recent 5.0-beta :)

Also, it is not there yet but the (old) wp_calculate_image_sizes filter will get the $block_attributes too. Let me know if something else/more is needed, also for the attributes passed from the editor.

[mor10]

@azaozz What, in your opinion, should be the preferred method for themes to control the sizes attribute? The wp_calculate_image_sizes filter or the render_block_core_image_tag_attributes filter? Is one more robust / performant / future proof than the other?

[mor10]

@azaozz This works! Look at the PR to Twenty Nineteen above.

[azaozz]

@mor10 these filters have different scopes, depends what the theme wants to accomplish :)

• The new render_block_core_image_tag_attributes gives full control of the <img> tag on the front-end (after this PR is merged of course). The theme should use this filter if it needs to set more/other attributes, or to re-calculate some of the existing attributes except width, height, srcset, and sizes. These four have their own filters that (will) get the same data, the existing filters for srcset and sizes plus the new block_core_image_get_width_height. Another difference is that render_block_core_image_tag_attributes runs only in WP 5.0+ and only when rendering the image block. In the future it may run for other blocks that contain images (Cover, Media and Text), or we may add other filters there..?

• The wp_calculate_image_sizes is preferred when the theme looks at the sizes attribute. It will get the $block_attributes once this PR is merged, which will (hopefully) be enough to generate a much more precise sizes attribute. It also exists in older WP versions making it more compatible.

Ideally every theme that supports the Block editor would hook into block_core_image_get_width_height (as it controls image scaling and to some extend generation of srcset and sizes) and wp_calculate_image_sizes. These two would give "full control" over sizing of images on the front-end. Alternatively it can use render_block_core_image_tag_attributes and re-calculate some of the img tag attributes (it runs "after the fact" so it will be a tiny bit slower).

Thinking we will have to post something like this explanation on /make as soon as this PR is merged. Would you be up for that? :)

Edit: just renamed gutenberg_get_image_width_height filter to block_core_image_get_width_height for consistency. Fixed above.

[mor10]

I can help write a post explaining all this. Will test the different approaches first thing next week to get a clear picture of what each of the filters does and how best to guide theme developers to get this baked in before launch.

[aduth]

1. Adds fileWidth, fileHeight, editWidth, userSet block attributes. These would allow us to add width and height, and the themes to generate better sizes attributes on the front-end.

I have a hard time imagining there's not at least some redundancy in having all of width, fileWidth (fileHeight), userSet, and editWidth.

• Can userSet be derived as a function of the presence of width or height?
• When would editWidth not be equal to width in scenarios where e.g. it'd be used for scaling?
• Does it matter what the fileWidth and fileHeight are for external images? Can that be determined server-side (cached if necessary) if it's relevant there?
  • At worst, could it be reflected in width and height?

[azaozz]

Of course there is some redundancy, but it's a "good thing" imho :)

Can userSet be derived as a function of the presence of width or height?

Sure, except when were width and height set from resizing by dragging, and when were they set directly by the user. In some cases that may not matter in others (scaling on the front end) it's pretty critical.

When would editWidth not be equal to width in scenarios where e.g. it'd be used for scaling?

Pretty much all resized images? Both "100% width" and resized images can potentially be scaled.

Does it matter what the fileWidth and fileHeight are for external images?

Yes, since the image block doesn't add width/height for non-resized images. There must be some way for the theme (front-end) to get at least the actual image dimensions. If these are known, the theme (or core) can decide to scale these images too. Also, images outputted without width and height attributes behave "badly", make the page "jump" while loading, push the comments form below the fold (while the user is trying to type), etc.

Can that be determined server-side (cached if necessary) if it's relevant there?

Possibly, but to store it we will have to change the image meta schema.

At worst, could it be reflected in width and height?

Sure. That would mean the image block will always add width and height attributes to all images? That behavior seems to be the default in pretty much all editors I've seen, not sure why this was removed from the image block...

However, even when having width and height we would still prefer to know the fileWidth and fileHeight at least for external images. And, if we add them for external, why not for internal? :)

I don't see any problem in saving some extra/redundant image meta in the image block props. This will make in more futureproof. Not having enough meta is much worse (see the image attachments meta where we could have stored few more very very useful values).

[mtias]

Is this needed?

[dmsnell]

if this is what I'm thinking it is, it sounds like it's something like intentionallySetWidth and intentionallySetHeight or displayWidth and displayHeight

is there confusion since this boolean value doesn't entirely describe why it exists or what the user set?

[azaozz]

You mean userSet = userSet || undefined; or userSet? The first is probably not, just some "normalization", but as that's an internal function it can be skipped.

The userSet indicates whether the image width and height were set directly by the user (typed in the width or height fields in the inspector), or they were added as a result of resizing the image by dragging. This would make a difference when the theme or a plugin is deciding to scale or not to scale an image on the front-end.

In the previous iteration these were actually two values: userWidth and userHeight. Switched it to one boolean after some critique that we store too much image meta in the block props. Can revert that easily :)

[aduth]

Worth noting there's a subtlety here in that if the argument was called with userSet = false, it would normalize the value to undefined, which in practice has the same result since the default of the attribute is false. And while that default is unlikely to change, if it were to, the normalizing would have the adverse effect of preventing a value of explicit false to be provided as an argument.

[dmsnell]

is there a reason for this not to be hooking into the block filter so that we can get the actual attributes?

function _gutenberg_cache_images_meta( $html, $block ) {
	if ( 'core/image' === $block[ 'blockName' ] ) {
		_prime_post_caches( $block[ 'attrs' ][ 'id' ], /* why aren't these */ false, /* commented? */ true );
	}

	return $html;
}

add_filter( 'rendered_block', '_gutenberg_cache_images_meta', 3 );

[azaozz]

The purpose here is to get the IDs of all images (where we will do srcset and sizes) from the post before we parse the blocks. That's needed to get all image attachment meta in one go from the DB as that's faster.

So we need an array with all attachment IDs for the images used in the post. This is equivalent to https://core.trac.wordpress.org/browser/tags/4.9.8/src/wp-includes/media.php#L1336.

That should also include images from "Cover" and "Media and Text" blocks, and probably the Gallery block (if images there are "hardcoded"). Any ideas how to do that without a regex are very welcome :)

[aduth]

Previously: #11377 (comment)

I'm not sure I understand why this would need to occur prior to the parse. If it at least occurs prior to any block evaluating its render_callback, wouldn't that be equally sufficient to warm the cache?

[aduth]

If it at least occurs prior to any block evaluating its render_callback, wouldn't that be equally sufficient to warm the cache?

Since this doesn't exist as an option today anyways (requires filter on this value), we probably can't do anything of this sort right now.

Should follow-up with a Trac ticket as appropriate.

[azaozz]

Yeah, this doesn't do anything if we do it "per block". It makes sense only when it happens for all of post_content at once before we start to look at individual images and generating srcset and sizes.

[dmsnell]

if we hook into the block-level filter (the design of which I favor in #10108) then we can track the parse through the blocks and then prime the cache on the do_blocks filter which occurs after parsing and rendering is complete.

that is, we could imagine something like this…

function _gutenberg_cache_images_meta( $html, $block ) {
	if ( $block['blockName'] === 'core/image' ) {
		$this->add_image( $block['attrs']['id'] );
	}

	return $html;
}
add_filter( 'rendered_block', '_gutenberg_cache_images_meta', 3 );

function _gutenberg_prime_cache() {
	if ( empty( $this->images ) ) {
		return;
	}

	// whatever command does all images in one go
	_prime_post_caches( $this->images, false, true, true, true, false, false );
}
add_filter( 'do_blocks', '_gutenberg_prime_cache' );

and this because we know that the final do_blocks filter runs after block-level processing.

[dmsnell]

noting that this is still based on a RegExp instead of using the per-block and per-document filters

[azaozz]

Another option would be to "unify" this with the "old" cache priming. We have the (required) wp-image-#### className on all images, including in Cover and Media and Text blocks.

Will need to abstract wp_make_content_images_responsive() a bit, move the caching out and run it earlier on the_content filter.

On the other hand _prime_post_caches() is "clever enough" to not fetch posts and meta that are already cached so running two pre-caching methods won't "hurt" anything.

[mtias]

I'm leaning towards doing this in conjunction with #10108 because I want to be sure the design is ergonomic enough to power this use case in a way that is scalable and transitive to other cases on other blocks.

[dmsnell]

this name is definitely confusing here…does the width have a height?

[azaozz]

Well, you cannot really read the name of a function without reading it's parameter names, then it becomes pretty clear imho :) But lets change it to something more descriptive.

[dmsnell]

there's no need here for two calls to setAttributes() - we can collapse them into one in a couple of different ways. my favorite is with Object.assign()

// what does this name mean?
resetWidthHeight( fileWidth, fileHeight ) {
	this.props.setAttributes( Object.assign(
		{
			width: undefined,
			height: undefined,
			userSet: undefined,
		},
		fileWidth && fileHeight && {
			fileWidth,
			fileHeight,
		}
	) );
}

[azaozz]

Sure. The purpose here is to make it as easy to read and understand as possible. After going through babel and minification, the code that actually runs in production will be completely different.

(I actually "have a beef" with the way a lot of the code is written. All the redundant indentation, nested lambda functions (no names = poor context), "clever" conditionals, almost no inline comments... There seem to be quite a few cases where the code becomes more readable after running it through babel and minification, then "unminifying" it. But lets look at that after 5.0 is out.) :)

[aduth]

there's no need here for two calls to setAttributes() - we can collapse them into one in a couple of different ways. my favorite is with Object.assign()

(Noting that the supplied example doesn't use Object.assign and wouldn't work as written)

[dmsnell]

(Noting that the supplied example doesn't use Object.assign and wouldn't work as written)

Good call! 😆 (fixed)

[aduth]

Sure. The purpose here is to make it as easy to read and understand as possible. After going through babel and minification, the code that actually runs in production will be completely different.

The main thing for me here would be to avoid multiple calls to setAttributes, as it will incur a render for each (unnecessary impact on performance). This will remain true after transpilation and minification.

I pushed bf67408 which I think is a reasonable compromise to preserve a similar readability.

[dmsnell]

looks like we're introducing rounding errors when none are needed.

const width = targetWidth = ( fullWidth * ( targetWidth / fullWidth ) );

if we look in the math world the fullWidths will cancel each other out, but in JS we'll probably convert first to the ratio and then multiply, leaving error. the good news is that the accurate way is faster due to needing no computation.

constrainImageDimensions( width, height, targetWidth ) {
	return {
		height: Math.max( 1, Math.round( height * targetWidth / width ) ),
		width,
	};
}

Also, since this doesn't depend on any instance variables, might as well put it outside of the class and make it a simple helper function?

function scaledBy( value, ratio ) {
  return Math.max( 1, Math.round( value * ratio ) );
}

…

const width = fullWidth;
const height = scaledBy( height, width / targetWidth );

[azaozz]

constrainImageDimensions is pretty much a copy of core's wp_constrain_dimensions(): https://core.trac.wordpress.org/browser/tags/4.9.8/src/wp-includes/media.php#L400. We can change/improve it as long as it works in exactly the same way (and is "easy to read") :)

since this doesn't depend on any instance variables, might as well put it outside of the class and make it a simple helper function

Sure, it probably can be re-used somewhere else.

[dmsnell]

We can change/improve it as long as it works in exactly the same way

well it caught my attention because a bug in the algorithm is already mentioned in the comment 😉
then I realized it's doing much more than it needs to, and unlike the core function we're only scaling one dimension.

seems silly to me to drag out old code that fulfills other purposes in a way that's inefficient and buggy 🙃 (my tone is light-hearted here).

"easy to read"

probably won't ever reach consensus here, but I am sharing my own difficult in figuring out what the code is supposed to do. you can take what you want from it. my bias is that I'm pretty ignorant and unable to keep lots of stuff in my head at once; smarter people will probably be able to do more in-head parsing/compiling/state-tracking.

[dmsnell]

using map without returning a new list and when modifying things inline is pretty unusual. we have a couple of choices depending on what our intention is. it looks like we're looking for a specific value and using that if it exists.

updateImageURL( url, sizeOptions ) {
	const { width, height } = sizeOptions;

	if ( width && height ) {
		return this.setAttributes( {
			url,
			fileWidth: width,
			fileHeight: height,
		} );
	}

	const option = find( sizeOptions, ( { value } ) => value === url );

	if ( option ) {
		return this.setAttributes( {
			url,
			fileWidth: option.imageData.width,
			fileHeight: option.imageData.height;
		} );
	}
}

[azaozz]

Right, it doesn't need to use map there. In the previous iteration sizeOptions was different so we always needed to iterate to find the proper values, now it's simpler.

[markfinst]

What about adding srcset by default?

• Every image within Image or Gallery block has srcset and sizes.
• srcset contains list of all available image sizes which aspect ratio matches original. If user crops image - we return all images that match aspect ratio of cropped.
• sizes attribute is generated automatically depending on where image is added (for example, if image is added to full-width container sizes would be 100vw).

To implement this, WordPress core badly needs an API method for srcset and sizes, specifically:

get_image_srcset_attribute_array( $image_id, $max_image_width );

Returns array of images that should be be used in srcset, in format:

array(
  '123' => array(
    'url' => 'https://...',
    'width' => 600,
    'height' => 400,
    'size_key` => 'thumbnail'
  ),
 ...
)

So theme developers can easily filter it.

get_image_sizes_attribute( $size_key, $size_ratio = 1 )

• $size_key is a key that describes location of the image (full-width, align-left, content ...)
• $size_ratio is relative width multiplier, for example if image is added in 1/3 column, size_ratio should be 0.33.

Should return sizes attribute value.

[azaozz]

To avoid the arbitrary clientWidth - 30, perhaps can append another div here with class="editor-block-list__block-edit" to mimic the default block layout, then get clientWidth from it. I'll try it.

I know, it's fragile, but seems workable for now.

Done in 078a75b.

There still appear to be an inconsistency/delay when TwentyNineteen is active, and a post with images is opened for editing. Seems caused by the CSS as the images are also not sized properly, "hang out on the right" a bit. Looks like some CSS is loaded/added later, after rendering has finished. That causes a re-flow in the browser. When that is fixed, the clientWidth will be correct from the start.

[aduth]

I think we should be shortcutting here if the block is not the image block? It'll eventually be shortcutted at the below test of empty( $block_attributes['url'] ), but it seems both indirect and wasteful to let us get that far, when this is called for every block.

[azaozz]

Right, good catch. Added in 648811f.

[mtias]

I'm not super confident in going this route (i.e. rebuilding the markup) because it carries a lot of overhead in future updates to the image block client side.

Also, I worry we package a lot of this functionality in the image block alone, and any other block that uses images, misses on it.

[azaozz]

Using block props to rebuild the <img> tags seems the best option. It would be trivial to add support for more/other attributes in the future.

Of course we can always go "the other route" like in the current processing when adding srcset and sizes: use regex to parse some of the attributes, then add or replace them with recalculated values.

[mtias]

One general concern I have here is scalability if we want to reproduce a similar behaviour to WordPress/twentynineteen#629 for other blocks that are not wp:image, including third-party blocks that might be using media.

[azaozz]

Heh, yoda conditionals fail for negative checks?? Who added that to the coding standards?? It was not a part of the original :)

Needs revision.

[azaozz]

One general concern I have here is scalability if we want to reproduce a similar behaviour to WordPress/twentynineteen#629 for other blocks that are not wp:image, including third-party blocks that might be using media.

Right, the idea was to add that to the Image Block first, test it, tweak it, and then add support for all images in all blocks. It won't be necessary for all images to have all block props, would be enough to at least have width and height, or fileWidth and fileHeight. The "main" underlying idea is for all image tags to be filterable before output, preferably without using regex to parse the attributes.

Enabling support for other blocks would be as easy as changing (the negative yoda noncompliant) $block['blockName'] !== 'core/image' to include more blocks :)

[aduth]

Heh, yoda conditionals fail for negative checks?? Who added that to the coding standards?? It was not a part of the original :)

Needs revision.

I'm not sure I follow. 'core/image' !== $block['blockName'] would be the corrected form, passing lint and complying with the standard?

[getsource]

@youknowriad I'm working on triage for WP 5.0.3, on https://core.trac.wordpress.org/ticket/45407, and noticed that this PR was moved out of all GB milestones.

Is still the approach planned to get the information to core? If it is, should it be milestoned somewhere so that it gets tracked? If not, I'd appreciate being linked to any other relevant issues or PRs so that I can add them to the core ticket.

Thanks so much!

[youknowriad]

Correct if I'm wrong but I think we kind of agreed that this PR is not the right approach to solve these issues. At least we should break it into more discrete items and discuss the path forward.

About the milestoning. PRs are milestoned according the corresponding plugin release. This didn't seem ready for me for 4.9 or 5.0. But the priorities... are handled more precisely in issues.

At the moment we have two milestones:

• 5.0.x for the most pressing things
• 5.x for the ones after

We also have "Phase 2" for things that can come more down the road. I'd like us to work and triage and clarify these milestones more. In my personal priorities for phase2, I consider this meta issue as a good one to follow #13113

[mor10]

From my perspective, this PR is indeed too wide in scope and much of what it tries to solve deserves to be broken into smaller pieces or reallocated to a wider conversation about media in general.

Specifically regarding https://core.trac.wordpress.org/ticket/45407, this is still a pressing issue that needs a resolution immediately. Afaik it is possible to resolve that specific issue as proposed in the Trac ticket without altering large parts of core or GB. The main issue in the past seemed to be it requires both changes to Core and GB, but since GB is now core that shouldn't be a blocker.

I strongly recommend flagging the RICG ticket as high priority as core is still shipping incorrect code and preventing theme and plugin developers from resolving it on their end.

[gziolo]

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? It also sort of duplicates #11377 as it was extracted from it.

[aduth]

Per the most recent comments, it seems clear this pull request cannot be salvaged in its current state and will need to be revisited in separate smaller tasks.

Related tracking issues / tickets:

• Enhancement: images handling #6177
• https://core.trac.wordpress.org/ticket/45407