WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#36096 closed defect (bug) (fixed)

Custom Logo: Improper image size appears momentarily after selection

Reported by: westonruter Owned by: westonruter
Milestone: 4.5 Priority: normal
Severity: normal Version: 4.6
Component: Customize Keywords: has-patch commit
Focuses: Cc:

Description (last modified by 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

Attachments (1)

36096.0.diff (2.4 KB) - added by westonruter 5 years ago.

Download all attachments as: .zip

Change History (22)

#1 @westonruter
5 years ago

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

#2 @westonruter
5 years ago

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).

#3 @karmatosed
5 years ago

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.

#4 @obenland
5 years ago

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

#5 @westonruter
5 years ago

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.

#6 @westonruter
5 years ago

@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?

#7 @westonruter
5 years ago

  • Keywords needs-patch added
  • Owner set to obenland
  • Status changed from new to assigned

#8 @westonruter
5 years ago

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.

#9 @obenland
5 years ago

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?

#10 @westonruter
5 years ago

@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.

#11 @obenland
5 years ago

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.

#12 @westonruter
5 years ago

@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?

#13 @westonruter
5 years ago

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.

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


5 years ago

#15 @westonruter
5 years ago

  • Owner changed from obenland to westonruter
  • Status changed from assigned to accepted

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.

#16 @westonruter
5 years ago

  • Description modified (diff)
  • Summary changed from Custom Logo: Use of custom image size requires unexpected filtering of image_size_names_choose to Custom Logo: Improper image size appears momentarily after selection

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

@westonruter
5 years ago

#17 @westonruter
5 years ago

  • Keywords has-patch added; needs-patch removed

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.

This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.


5 years ago

#19 @westonruter
5 years ago

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

#20 @westonruter
5 years ago

  • Keywords commit added

#21 @westonruter
5 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

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.

Note: See TracTickets for help on using tickets.