Customizer: Can't crop header images

[alshakero]

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.

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

The ticket has all the information needed.

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

[yansern]

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

[audrasjb]

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

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.

[arthur791004]

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

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

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

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

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

@arthur791004 that's a great point!

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

cc: @arthur791004

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

[costdev]

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

[nmutua]

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

[audrasjb]

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.

[desrosj]

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.

[arthur791004]

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!

[audrasjb]

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

[audrasjb]

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.

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

[obenland]

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

[ironprogrammer]

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.

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

[ironprogrammer]

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

• ✅ The cropping controls issue with ​sample 1724x2300 image was resolved with patch.

Additional Notes

• Tested regression with this ​1724x2299 image.
• Note that a ​2300x1724 landscape version of the originally failing image was not affected by this issue, and remained "croppable".

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

Hi @costdev! I rebased it 👍🏼

[azaozz]

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]

In 54308.diff: unified diff of both PRs.

[azaozz]

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.

[SergeyBiryukov]

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

[azaozz]

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

[ironprogrammer]

Append WordPress version to imgAreaSelect version.

[azaozz]

In 54928:

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

Props SergeyBiryukov, ironprogrammer.
Fixes #54308.

Resolved in changeset 54903. This PR can be closed.

Resolved in changeset 54903. This PR can be closed.