WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#33417 closed defect (bug) (fixed)

JS error when uploading small image as Site Icon and electing to Skip Cropping

Reported by: truonght Owned by: ocean90
Milestone: 4.3.1 Priority: normal
Severity: normal Version: 4.3
Component: Customize Keywords: has-patch
Focuses: javascript Cc:
PR Number:

Description (last modified by westonruter)

I select a _small_ image in Customize > Site identity > Site icon. After selecting the image, I choose “Skip Cropping” and then the modal changes to blank. I see an error in browse console:

Uncaught TypeError: Cannot read property 'url' of undefined

Attachments (2)

33417.patch (973 bytes) - added by tyxla 4 years ago.
Customizer Site Icon: SiteIconControl.setImageFromAttachment() - properly handling smaller image sizes when cropping step is skipped.
33417.diff (580 bytes) - added by obenland 4 years ago.

Download all attachments as: .zip

Change History (18)

#1 @westonruter
4 years ago

  • Keywords reporter-feedback added

@truonght Thanks for the report. I tried reproducing your steps, but I do not even see there being a “Skip Cropping” option after having selected an image.

#2 @truonght
4 years ago

Oh sorry. Now it work. I think problem is i didn't update 2015 theme :)

#3 @truonght
4 years ago

Oh. It still happens when I select a small image (about 32px)

#4 @westonruter
4 years ago

  • Description modified (diff)
  • Focuses javascript added
  • Keywords needs-patch added; reporter-feedback removed
  • Milestone changed from Awaiting Review to 4.3.1
  • Summary changed from Js error when skip sropping to JS error when uploading small image as Site Icon and electing to Skip Cropping

Thank you. I was able to reproduce the issue once I tried a _small_ image.

The error is happening in the SiteIconControl.setImageFromAttachment() method on this line:

$( 'link[sizes="32x32"]' ).attr( 'href', icon.url );

#5 @obenland
4 years ago

  • Owner set to obenland
  • Status changed from new to accepted

#6 @tyxla
4 years ago

With the current implementation, if there is an image with the size site_icon-32, it is used. If there is no image with that size, the thumbnail size is used. In case the image is smaller than thumbnail, it has only full size, which we should use in this case. So I suggest that we check for all of these sizes and finally the full size for this edge case. Patch coming up soon.

@tyxla
4 years ago

Customizer Site Icon: SiteIconControl.setImageFromAttachment() - properly handling smaller image sizes when cropping step is skipped.

#7 @tyxla
4 years ago

  • Keywords has-patch added; needs-patch removed

@obenland
4 years ago

#8 @obenland
4 years ago

I'd like to propose a different approach, requiring users to always crop the icon.

We need the icon to be square anyway, otherwise they're displayed distorted which is not a good user experience either.

#9 @westonruter
4 years ago

  • Keywords needs-testing added

@tyxla @truonght Thoughts on 33417.diff?

#10 follow-up: @tyxla
4 years ago

  • Keywords needs-testing removed

My two cents: I believe we should allow the user to skip the cropping step. There is no reason to require the cropping IMHO - why not allow the image to remain untouched? There are many cases when the user has prepared a special image for the site icon, and they would want it to remain uncropped.

Also, concerning 33417.diff, it does not fix the issue for me. I haven't made a research, but I guess that mustBeCropped is not fully implemented yet.

I've just tested my patch again, it fixes this issue and the "Skip Cropping" button works exactly as it should.

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


4 years ago

#12 @ocean90
4 years ago

  • Owner changed from obenland to ocean90

#13 @ocean90
4 years ago

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

In 34056:

Site Icon: For preview fall back to full size URL when thumbnail size doesn't exist.

Prevents a JavaScript error for rare cases when cropping is skipped and the image is smaller than thumbnail.

Props tyxla.
Fixes #33417.

#14 @ocean90
4 years ago

In 34057:

Site Icon: For preview fall back to full size URL when thumbnail size doesn't exist.

Prevents a JavaScript error for rare cases when cropping is skipped and the image is smaller than thumbnail.

Merge of [34056] to the 4.3 branch.

Props tyxla.
See #33417.

#15 in reply to: ↑ 10 @obenland
4 years ago

Replying to tyxla:

There is no reason to require the cropping IMHO - why not allow the image to remain untouched? There are many cases when the user has prepared a special image for the site icon, and they would want it to remain uncropped.

There are two reasons to force cropping, being able to generate the correct image sizes for app icons and making sure the image is actually square.

If 33417.diff didn't fix your issue we should see what the problem is and fix it rather than just patching over an error.

#16 @ocean90
4 years ago

#33865 was marked as a duplicate.

Note: See TracTickets for help on using tickets.