Custom Logo: Improper image size appears momentarily after selection

[westonruter]

In testing the Custom Logo in Twenty Sixteen, I found a strange bug where the the logo would appear momentarily in a large size and then get resized down properly. The properly-sized logo would appear once the selective refresh request returned. The momentarily-oversized image was the one updated via the postMessage preview logic. See screencast: ​https://youtu.be/JkQGhB_LzGo

I found that the media get-attachment Ajax request was not returning the twentysixteen-logo among the available sizes. I was surprised to find that in order to get a custom image size to be returned from the media get-attachment Ajax queries, the image size had to be explicitly added to the image_size_names_choose filter. This filter is probably specifically for the UI to ensure that there are names for the sizes to show, but this makes it a headache for themers to add support for Custom Logos.

Here is what a theme currently has to do to add support using a custom image size as a custom logo: ​https://github.com/WordPress/twentysixteen/pull/434

UPDATE: We can go ahead and remove the JS logic for applying the image change directly in favor of just relying on selective refresh. See comments at https://core.trac.wordpress.org/ticket/36096#comment:13

[westonruter]

The use of the image_size_names_choose filter in get-attachment Ajax requests was introduced in [22850] for #22598.

[westonruter]

It seems this problem wasn't faced by Jetpack because it included the image sizes in the site_logo option: ​https://github.com/Automattic/jetpack/blob/43c61c4377f427efa94f1e3e1ac754e419eca3e2/modules/theme-tools/site-logo/inc/class-site-logo.php#L101

Now we're just storing the attachment ID and using whatever sizes are available at runtime (which is a better approach than storing the sizes in the DB with the attachment ID).

[karmatosed]

I don't seem to have found this in testing. @obenland was there a change made?

It feels a lot to ask for themers to have to add that code to every theme in order for site logos to work correctly. I'm not really keen on how much we're expecting people to add to a theme to support this. It feels like it will with each thing get used less. On top of that, it's not something other features have asked.

[obenland]

I had just removed that actually in [36839]. We should probably bail in the callback if we're not DOING_AJAX I assume?

[westonruter]

In 36851:

Customize: Eliminate unnecessary WP_Customize_Site_Logo_Control in favor of re-using WP_Customize_Image_Control.

• Removes double margin-bottom from all the media controls.
• All media controls now send {settingId}-attachment-data messages to the preview when a media setting is updated so that the preview has access to the attachment data.
• Fixes receiving of attachment-data message for custom_logo which resulted in instant JS-applied preview not working. See #36096.

See #33755.
Fixes #35941.

[westonruter]

@karmatosed @obenland The reason why the bug wasn't visible was because there was a typo in the attachment-data message name being received from the pane, resulting in the JS-applied instant preview not working. See https://core.trac.wordpress.org/changeset/36851/trunk/src/wp-includes/js/customize-preview.js

Yeah, so you're saying that the filter should only be added if DOING_AJAX? That would work. Actually, how about adding the filter inside of wp_ajax_get_attachment() to make the connection explicit?

[westonruter]

I see what you mean. Yeah, reverting the commit and including an DOING_AJAX check around the filter addition for image_size_names_choose would do it I think.

[obenland]

Hm, even with that enabled I still experience the bug in Twenty Sixteen. It doesn't appear in Confit for example. Might this be a styling issue?

[westonruter]

@obenland are you trying images that have been uploaded since the theme was active? It could be that the image size wasn't generated. I was noticing that particular issue myself.

[obenland]

Yes, that is the problem. I tested an existing image and a freshly uploaded one with Confit activated and Twenty Sixteen in Live Preview. So that filter will solve it for the activated theme, but not for Live Preview.

[westonruter]

@obenland what is the solution in the case of theme switch preview? Is it that the JS logic needs to make sure it doesn't pick a size that is bigger than the target dimension? Or does it need to force whatever image chosen to have the right width/height?

[westonruter]

I chatted with @obenland over Slack and suggested that we might as well drop the logic for applying the custom logo changes entirely with JS and instead rely on selective refresh to handle the preview. The problem with using JS to apply the preview is that it means we basically have to re-implement the entire image_downsize() logic from PHP in JS to determine which available image to use for the logo and to give it the proper dimensions. Even when this is done, there will be aspects we'll miss because WP allows the image_downsize() logic to be filtered. By using selective refresh, we'll be assured to get the canonical PHP-rendering of the logo.

Since the only value of the JS instant preview is to show the image in place while waiting for the selective refresh request to respond, and since the response should come in less than a second, I don't think there is a lot of value to duplicate the logo display logic in JS and we can get rid of a lot of JS code.

[westonruter]

I talked about this with @obenland over Slack, and we agreed to go ahead and remove the JS logic for applying the preview in favor of relying on Selective Refresh. This will allow us to re-use the image_downsize() logic in WordPress without having to re-implement it in JS.

[westonruter]

(This will eliminate the bug where a theme could see a large image appear momentarily for a custom logo.)

[westonruter]

36096.0.diff​ keeps the preview logic DRY by letting selective refresh handle everything; this also eliminates the problem of an improperly-sized image being shown momentarily before selective refresh returns.

[westonruter]

@obenland @karmatosed @iamtakashi Please give 36096.0.diff​ a try. With this, we can close ​https://github.com/WordPress/twentysixteen/pull/434

[westonruter]

In 36990:

Customize: Rely on selective refresh exclusively for previewing custom logo changes.

Eliminates JS logic (from [36698]) which attempted to do pure JS update while waiting for the selective refresh response to return. The duplicate JS logic lacked a re-implementation of the image_downsize() functionality available in PHP, and so the JS preview logic would fail to properly preview images that didn't have the exact theme image size generated. To keep the code DRY and to eliminate the momentary display of an improperly-sized image, the duplicated JS logic is now removed.

See #27355.
See #33755.
Fixes #36096.