Make WordPress Core

Opened 3 years ago

Closed 17 months ago

Last modified 8 weeks ago

#54308 closed defect (bug) (fixed)

Customizer: Can't crop header images

Reported by: alshakero's profile alshakero Owned by: alshakero's profile alshakero
Milestone: 6.2 Priority: normal
Severity: normal Version:
Component: Customize Keywords: has-patch has-testing-info
Focuses: ui, javascript Cc:

Description

Quoting https://github.com/Automattic/wp-calypso/issues/52182

Steps to reproduce the behavior

  • Activate a theme that uses a custom header image:
  • Go to My Site > Appearance > Customize > Header Image
  • Click on Add new Image
  • Select and image and then click on Select and crop

What I expected to happen

To be able to crop the image to the correct header size and then save it to the header

What actually happened

When you get to the next screen, there is no cropping selection. Dragging across the image to create a selected section results in Drop image. Clicking on Crop results in the error {file name} There has been an error cropping your image. if there is a Skip Cropping option and you select it, it does not update the header image

There is a video of my test site here: d.pr/v/0kpjbn

Here is the diagnosis and proposed fix

imageAreaSelect breaks for non-integer image width

WordPress relies on imageAreaSelect in Customizer.

imageAreaSelect has a function called adjust, this functions is called on every change to the selection box. And it's also called on-load to select the entire image.

adjust has a check to see whether selection is outside the image boundaries, in which case it caps the selection values to image boundaries and re-renders.

This capping is done by using Math.min and Math.max, which return NaN if one of their operands is undefined.

When an image is loaded, we render the selection box, then we call adjust. If the image has a width of 400.6, its width is rounded to 401 and the selection box's width is set to that, when we check if the selection went outside the image if(selection.x2 > imgWidth) we get true, because the rounded value is indeed bigger than the original value. When we come to re-render by calling doResize, Math.min returns NaN and the selection box disappears.

A possible solution is to use floor instead of round, this way the check if(selection.x2 > imgWidth) won't return true for non-integer image width.

Another solution might be to check if any of the coordinates are undefined in the beginning of doResize. I tried both solutions and both work, but I prefer the first one, because the second one might hide bugs we don't know about and make it harder to debug them.

Attachments (2)

54308.diff (24.8 KB) - added by azaozz 17 months ago.
54308.2.diff (475 bytes) - added by ironprogrammer 17 months ago.
Append WordPress version to imgAreaSelect version.

Download all attachments as: .zip

Change History (38)

This ticket was mentioned in PR #1774 on WordPress/wordpress-develop by alshakero.


3 years ago
#1

  • Keywords has-patch added; needs-patch removed

#2 @yansern
2 years ago

Tested this on zoomed browser for both portrait & landscape images, and unzoomed browser on both portrait & landscape images. Resize handle is showing as expected!

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


2 years ago

#4 @costdev
2 years ago

  • Keywords needs-testing added

#5 @audrasjb
2 years ago

  • Milestone changed from Awaiting Review to 6.0
  • Version trunk deleted

Hello, thanks for reporting this ticket. It appears this issue wasn't introduced in WP 5.9, so I'm removing the trunk version from the ticket.

Also, let's move this to 6.0 since it has a ready-to-test PR :)

This ticket was mentioned in PR #2302 on WordPress/wordpress-develop by arthur791004.


2 years ago
#6

Trac ticket: https://core.trac.wordpress.org/ticket/54308#ticket

Fixes https://github.com/Automattic/wp-calypso/issues/52182

If the doResize is called without initialized the x1, x2, y1 and y2 values, it causes x1 is NaN and then the selection area would be wrong.

Why do doResize call at the beginning? As imgAreaSelect uses Math.round for calculation and the height/width of image is non-integer, it will make the following condition becomes true
{{{js
If imgWidth is 1448.95, selection.x2 will be 1449 so that the doResize() is triggered before x1, x2, y1, y2 are initialized

if (selection.x2 > imgWidth
selection.y2 > imgHeight) {

doResize();

}
}}}

Thus, https://github.com/WordPress/wordpress-develop/pull/1774 replacing Math.round with Math.floor fixes the issue.

#7 @arthur791004
2 years ago

Hello,

I also dive into this issue and found that the root cause is the doResize function is triggers before x1, x2, y1, y2 are initialized. Then, it causes the calculation for x1 = min(x1, left + imgWidth); results NaN. (See https://github.com/WordPress/wordpress-develop/blob/968b3fc5a4aa9871ce8654b48502f8710fc804a1/src/js/_enqueues/vendor/imgareaselect/jquery.imgareaselect.js#L597). Therefore, there is no cropping selection at the beginning.

See https://github.com/WordPress/wordpress-develop/pull/2302

Last edited 2 years ago by arthur791004 (previous) (diff)

autumnfjeld commented on PR #2302:


2 years ago
#8

@arthur791004 I'm curious if you could add a test for this, seems like if there had been a test case, this bug would have been caught with ates.

autumnfjeld commented on PR #1774:


2 years ago
#9

@arthur791004 @alshakero
Just confirming that _both_ this PR #1774 and https://github.com/WordPress/wordpress-develop/pull/2302 should be merged to fix https://core.trac.wordpress.org/ticket/54308#ticket.

alshakero commented on PR #1774:


2 years ago
#10

@autumnfjeld AFAICT, my PR is sufficient to resolve the issue because it circumvents the issue solved in #2302.

arthur791004 commented on PR #1774:


2 years ago
#11

@alshakero Thanks for adding my approach to your trac ticket! I think your PR is good and it can fix the case you mentioned. But I'm not sure there is any other case or not. For example, it seems that the developer can set the initial selection via options here. If the value is greater than image width/height, it might resolve NaN again. So I still think initializing those variables is perhaps safer.

alshakero commented on PR #1774:


2 years ago
#12

@arthur791004 that's a great point!

autumnfjeld commented on PR #1774:


2 years ago
#13

@alshakero Just curious if there are any blockers to finishing this work. Did you need any further assistance?

cc: @arthur791004

alshakero commented on PR #1774:


2 years ago
#14

Hi @autumnfjeld! I'm just waiting for review in Trac. Is there anything I can do to accelerate getting this merged?

#15 @costdev
2 years ago

  • Milestone changed from 6.0 to 6.1

With 6.0 RC1 starting soon, I'm moving this to the 6.1 milestone.

#16 @nmutua
20 months ago

Hi @costdev & @alshakero . I'm hoping to get clarity on how we can get this PR https://github.com/WordPress/wordpress-develop/pull/1774 reviewed & merged, I am not very familiar with all the process here and how to move this along. Any input would be appreciated. :)

Thank you!

cc: @arthur791004

#17 @audrasjb
19 months ago

  • Keywords changes-requested dev-feedback 2nd-opinion added; needs-testing removed

Hello,
It looks like this PR fixes the issue directly on imgareaselect which is a third party library.

That's why unit tests are failing there.

The patch should either be reported to be fixed upstream, or, if possible, we could patch the issue on WordPress own files, with a dedicated JS file that override imgareaselect's settings.

#18 @desrosj
19 months ago

  • Keywords dev-feedback 2nd-opinion removed

I haven't yet tested this report or the proposed fix. But wanted to point out a few things I noticed while scrubbing tickets in 6.1.

The imgareaselect library is considered adopted by WordPress Core as of [50270]. It has been largely unmaintained since 2013 (see ticket:51812#comment:50).

It looks like all of the tests are passing, but the pull request is targeting the master branch of wordpress-develop. This was changed a few years ago, and the pull request should be updated or recreated to be on top of trunk/merged into.

#19 @arthur791004
19 months ago

Yes, I also think it should be fixed by WordPress Core as https://plugins.jquery.com/imgareaselect has been unmaintained since 2013.

It looks like all of the tests are passing, but the pull request is targeting the master branch of wordpress-develop. This was changed a few years ago, and the pull request should be updated or recreated to be on top of trunk/merged into.

Okay, we'll change the target branch to the trunk! thanks!

#20 @audrasjb
19 months ago

Oh! thanks for the feedback @desrosj!

Shouldn't we have a way to indicate adopted libraries?

Either on the library main file itself or on the vendor readme.md file.

At least we should indicate this somewhere in the developer documentation.

But this is something for another ticket (which I'll add to my todo for 6.2).

#21 @audrasjb
19 months ago

  • Keywords changes-requested removed

#22 @audrasjb
19 months ago

  • Milestone changed from 6.1 to 6.2

With WP 6.1 RC 1 scheduled today (Oct 11, 2022), there is not much time left to address this ticket. Let's move it to the next milestone.

Ps: if you were about to send a patch and if you feel it is realistic to commit it in the next one or two hours, please feel free to move this ticket back to milestone 6.1.

obenland commented on PR #2302:


17 months ago
#23

@arthur791004 Should this be closed in favor of https://github.com/WordPress/wordpress-develop/pull/1774?

#24 @obenland
17 months ago

@ocean90 @SergeyBiryukov Is https://github.com/WordPress/wordpress-develop/pull/1774 something one of you could review/land?

#25 @ironprogrammer
17 months ago

Reproduction Report

For the test I used this test image at 1724x2300 pixels (some example dimensions reported in wp-calypso issue 52182).

Environment

  • Hardware: MacBook Pro Apple M1 Pro, Apple Studio Display (5K)
  • OS: macOS 12.6.1
  • Browser: Safari 16.1 and Mozilla Firefox 107.0, browser windows sized to 1280x1336 at 100% zoom
  • Server: nginx/1.23.2
  • PHP: 7.4.33
  • WordPress: 6.2-alpha-54642-src
  • Theme: dara v1.2.1

Actual Results

  • ✅ When using an image whose resized dimensions have a decimal that can result in a rounding error (described in #ticket), the crop tool handles overlay does not appear.

Additional Notes

  • Unable to reproduce in Google Chrome 107.0.5304.110 with window sized to 1280x1336 and 100% zoom.

@costdev commented on PR #1774:


17 months ago
#26

I think the PR needs to either rebase on trunk, or merge trunk to resolve the test failure on PHP 8.2 single site.

#27 follow-up: @ironprogrammer
17 months ago

  • Keywords has-testing-info added

Test Report

For the test I used this test image at 1724x2300 pixels.

Patch tested: https://github.com/WordPress/wordpress-develop/pull/1774.diff 👍🏻

Environment

  • Hardware: MacBook Pro Apple M1 Pro, Apple Studio Display (5K)
  • OS: macOS 12.6.1
  • Browser: Safari 16.1 and Mozilla Firefox 107.0, browser windows sized to 1280x1336 at 100% zoom
  • Server: nginx/1.23.2
  • PHP: 7.4.33
  • WordPress: 6.2-alpha-54642-src
  • Theme: dara v1.2.1

Actual Results

Additional Notes

Patch also fixes a similar cropping problem for the Customizer Site Identity > Logo and > Site Icon. See issue described in #55377.

alshakero commented on PR #1774:


17 months ago
#28

Hi @costdev! I rebased it 👍🏼

#29 in reply to: ↑ 27 @azaozz
17 months ago

Replying to ironprogrammer:

Thanks, great test report! Can also easily reproduce it with the linked image.

Looking at the patches, thinking both are worth adding. 1774 to fix the issue and 2302 as an enhancement that can also prevent similar issues.

As the imgAreaSelect library has been previously modified, and now seems unsupported, don't think there are problems to add these patches.

Patch also fixes a similar cropping problem...

Nice catch. Will close that ticket with the same commit.

@azaozz
17 months ago

#30 @azaozz
17 months ago

In 54308.diff: unified diff of both PRs.

#31 @azaozz
17 months ago

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

In 54903:

Media: Fix the initialization of imgAreaSelect when cropping a header image or a site icon or logo.

Props alshakero, arthur791004, nmutua, desrosj, audrasjb, ironprogrammer, obenland, costdev, ajmaurya.
Fixes #54308, #55377.

#32 follow-up: @SergeyBiryukov
17 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Thanks everyone!

Nitpick: version 0.9.10-wp2 looked a bit confusing to me when reading the changeset, as it's not quite clear what 2 refers to. The second iteration of WordPress' modifications to the file?

Should it perhaps be a date (0.9.10-wp-20221130) or the WP version in which it was last changed (0.9.10-wp-6.2)?

#33 in reply to: ↑ 32 @azaozz
17 months ago

Replying to SergeyBiryukov:

Should it perhaps be a date (0.9.10-wp-20221130) or the WP version in which it was last changed (0.9.10-wp-6.2)?

You're right, wanted to add some sort of change to the version string signifying another WP patch to the library. Anything should work imho, perhaps wp-6.2 would be most informative, but date or ticket number would work just as well :)

Last edited 17 months ago by azaozz (previous) (diff)

@ironprogrammer
17 months ago

Append WordPress version to imgAreaSelect version.

#34 @azaozz
17 months ago

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

In 54928:

Media: Fix the version string of imgAreaSelect to indicate when the second set of modifications were made.

Props SergeyBiryukov, ironprogrammer.
Fixes #54308.

@ironprogrammer commented on PR #1774:


16 months ago
#35

Resolved in changeset 54903. This PR can be closed.

@ironprogrammer commented on PR #2302:


16 months ago
#36

Resolved in changeset 54903. This PR can be closed.

Note: See TracTickets for help on using tickets.