Make WordPress Core

Opened 2 years ago

Closed 12 months ago

Last modified 4 months ago

#55070 closed defect (bug) (fixed)

Image Editing Cropping is not working properly

Reported by: andy786's profile andy786 Owned by: adamsilverstein's profile adamsilverstein
Milestone: 6.2 Priority: normal
Severity: normal Version: 5.9
Component: Media Keywords: has-patch has-screenshots has-testing-info commit
Focuses: Cc:

Description

If any image uploaded in Media is being edited and cropped and then saved. It does get saved. But the final image which gets displayed after the process is the original image itself. But the correct part should be that the final image should always appear as the edited image.

These issues appear when we crop any image two or three times.

Attachments (2)

Untitled.mp4 (8.0 MB) - added by andy786 2 years ago.
Screen recorded video of the issue of image cropping
55070.diff (934 bytes) - added by adamsilverstein 13 months ago.

Change History (38)

@andy786
2 years ago

Screen recorded video of the issue of image cropping

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


2 years ago

#2 @antpb
2 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 6.0

confirmed seeing this behavior on my end. Happens when you crop an image and get to the attach details page. It uses the old crop. Even closing the modal and opening the image again had no luck.

#3 @sanketchodavadiya
2 years ago

@antpb I have debugged this issue and below is my finding.

This issue happens if we crop below 1024px x 1024px(Large image size).
So, below large images are being regenerated with the crop but bigger than that images are untouched.

In core file media-template.php https://github.com/WordPress/WordPress/blob/ca76abd7e838b5760bc010d4ad3a38e693696667/wp-includes/media-template.php#L360-L361
Large size image is being shown when image is open in Attachment details popup that never cropped.

Also, while cropping when make_subsize(...) > _resize(...) or resize(...) function run it throws WP_Error error_getting_dimensions.

#4 @antonvlasenko
2 years ago

I'm able to reproduce this bug.
I'm working on a patch and I will submit it as soon as it's ready.

This ticket was mentioned in PR #2489 on WordPress/wordpress-develop by anton-vlasenko.


2 years ago
#5

  • Keywords has-patch added; needs-patch removed

#6 @antonvlasenko
2 years ago

Steps to test this PR:

  1. Upload a relatively large image to the Media Gallery (at least 2000x 1500 pixels). It's just easier to test this issue with a bigger image.
  2. Click on the image.
  3. Click on the "Edit Image" button below the image.
  4. Crop the image (using the "Crop" button above the image).
  5. A cropped image should be about 400 x 400 pixels in size.
  6. Save the cropped image (click on the "Save" button).
  7. You should be able to see the cropped image. Before the fix, WordPress would display the original image and not the cropped one.
  8. [Additional step] You can crop the image again and observe that it's displayed correctly.
Last edited 2 years ago by antonvlasenko (previous) (diff)

#8 @ironprogrammer
2 years ago

  • Keywords has-screenshots added

Test Report

PR 2489 LGTM! 👍🏻

Env

  • WordPress 6.0-alpha-52448-src
  • Safari 15.4
  • Chrome 100.0.4896.60
  • macOS 12.3.1 (Monterey)
  • Gutenberg DISABLED 🔴

Steps to Test

  1. Under Settings > Media Settings, check the currently configured "Large size" image dimensions (default is 1024x1024).
  2. Upload a very large image to Media > Media Library (anything over the configured "Large size" above should work).
  3. After upload, with the selected image, click the Edit Image button under the preview.
  4. Click the Crop button and either adjust the crop using the handles overlay on the image, or by entering new dimensions in the IMAGE CROP > Selection sidebar. Set the crop to smaller than the site's configured "Large size", and click Crop again to apply the change. Observe that the image displayed refreshes to the cropped version. ✅
  5. Click the Save button. Observe that the image refreshes and shows the original un-cropped version. ❌
  6. 🛠 Apply the patch, then refresh the Media Library page.
  7. Restore the original un-cropped image by clicking Edit Image, then expanding the RESTORE ORIGINAL IMAGE sidebar and clicking Restore image. (Alternatively, upload a new very large image to work with.)
  8. ℹ️ Close the image viewer and re-select the image to re-open it (this is necessary because the crop tool does not reset to the original dimensions after being restored.)
  9. Click the Crop button and either adjust the crop using the handles overlay on the image, or by entering new dimensions in the IMAGE CROP > Selection sidebar. Set the crop to smaller than the site's configured "Large size", and click Crop again to apply the change. Observe that the image displayed refreshes to the cropped version. ✅
  10. Click the Save button. Observe that the image refreshes and shows the cropped version. ✅

Expected Results (✅)

  • After cropping a large image to be smaller than the site's configured "Large size", the image preview displays the cropped image. ✅

Additional Notes

The above tests were performed with the "Large size" configured to both 1024x1024 and 1200x1200.

Screenshots

https://cldup.com/mm_K00ocOX.png
Crop applied to large image (Step 9).

https://cldup.com/WGl2Ybq6O8.png
Cropped image displayed in preview (Step 10).

#9 @costdev
22 months ago

  • Milestone changed from 6.0 to 6.1

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

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


22 months ago

#11 follow-up: @adamsilverstein
22 months ago

@antonvlasenko / @andy786 - thanks for the bug report and patch...

to clarify, this is (only) about the image that is displayed in the media library in the "edit image" screen, right?

when you insert the cropped image into a post for example, does the correct cropped version get used?

worth noting that WordPress keeps the original image when making these crops, so displaying the original un-cropped image might be considered expected behavior (if consistent).

Last edited 22 months ago by adamsilverstein (previous) (diff)

#12 in reply to: ↑ 11 @ironprogrammer
22 months ago

  • Keywords has-testing-info added

Replying to adamsilverstein:

to clarify, this is (only) about the image that is displayed in the media library in the "edit image" screen, right?

👍🏻 Yes, after the crop has been applied and saved.

when you insert the cropped image into a post for example, does the correct cropped version get used?

👍🏻 Yes, the cropped image is what is ultimately inserted into the post.

worth noting that WordPress keeps the original image when making these crops, so displaying the original un-cropped image might be considered expected behavior (if consistent).

The issue was raised due to an inconsistency that was discovered. When a user crops an image that is larger than the configured "Large image" media size, the preview of the image accurately reflects the crop applied -- but under the reported conditions, the preview does not reflect the crop.

In terms of expected behavior, I believe we're following that when a user is ready to insert the image, that the preview displayed is an accurate representation of what will be inserted into the post.

Thank you for highlighting the fact that this is a display issue, but does not affect the final image or post. 🙌🏻

#13 @antonvlasenko
22 months ago

@adamsilverstein

to clarify, this is (only) about the image that is displayed in the media library in the "edit image" screen, right?

Yes, that's correct. It's only about the image that's displayed in the media library.

when you insert the cropped image into a post for example, does the correct cropped version get used?

Honestly, I don't know which version of the image is used when a user inserts an image into a post.
I've never investigated this because it's out of the scope of this trac ticket.
IMO If there is an issue with post images, there must be a separate trac issue and a new patch.

#14 @antonvlasenko
22 months ago

@ironprogrammer

Yes, the cropped image is what is ultimately inserted into the post.

I'd like to clarify this, so there is no misunderstanding.
This PR doesn't affect which version of the image gets inserted into the post.
That logic has remained unchanged as this trac ticket doesn't report any issues with displaying post images.

Version 0, edited 22 months ago by antonvlasenko (next)

#15 @adamsilverstein
21 months ago

  • Owner set to adamsilverstein
  • Status changed from new to assigned

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


18 months ago

#17 @antpb
18 months ago

  • Milestone changed from 6.1 to 6.2

The PR for this ticket seems to still be in progress. With Beta 1 in an hour I am going to move this to 6.2 to keep it on our radar for the next release. Feel free to move it back to 6.1 if it is feasible for this release!

#18 @antonvlasenko
18 months ago

@antpb Thanks for the triage.
In terms of development, this PR is complete.
It should be decided whether this approach to fixing the bug suits the community.

#20 @adamsilverstein
13 months ago

Hey @antonvlasenko - thanks for your patience here.

I reviewed your PR again and while it does fix the display issue, it feels overly complex.

The underlying issue is the large image isn't cropped in this scenario and the code currently uses the large image. I came up with an alternate approach in 55070.diff (also at https://github.com/WordPress/wordpress-develop/pull/3985) - switch to using the full sized image for the edit modal preview (instead of large).

In my testing this resolves the issue you raised and the full size always gets the crop. I also tested rotating and after saving the preview was consistently correct. This does cause a potentially larger image to load, however this is still capped at the large image threshold and is probably fine in the edit context regardless - the image will actually show with higher fidelity on high dpi screens if we use the full size.

Could you please review and verify this resolves the issue you reported. Also welcome any feedback on the approach of using the full size image here instead of large.

#21 @adamsilverstein
13 months ago

  • Keywords reporter-feedback added

#22 @antonvlasenko
13 months ago

Thanks for the review, @adamsilverstein.

I reviewed your PR again and while it does fix the display issue, it feels overly complex.

I'm afraid I fail to understand how this is overly complex.
My solution parses "crop ID" from both "large" and "full" images.
If these crop IDs don't match (meaning that the Large image is outdated), it replaces the Large image with the Full image.
The advantage of such an approach is that it allows WordPress to still use the Large image (where applicable).

Honestly, I'm not entirely happy with either of the proposed PRs, as both sweep the underlying issue under the carpet.
I think the proper solution will be to delete the Large image if the Full image has lesser dimensions than the Large image.
However, I've no idea if implementing it will break backward compatibility.
I think I will be able to submit a new PR sometime this week or at least investigate if it's possible to fix it this way.
What is your opinion on that?

The underlying issue is the large image isn't cropped in this scenario and the code currently uses the large image.

Yes, I understand it.

Could you please review and verify this resolves the issue you reported.

I've reviewed your PR. Thanks.

#23 @antonvlasenko
13 months ago

So, this issue can be fixed by replacing
$meta['sizes'] = array_merge( $meta['sizes'], $img->multi_resize( $_sizes ) );
with
$meta['sizes'] = $img->multi_resize( $_sizes ) );
in src/wp-admin/includes/image-edit.php.
But it's impossible to fix it this way because of https://core.trac.wordpress.org/ticket/22985.
It will break BC.
I've approved your PR, @adamsilverstein.
I will add my test report soon.

@sanketchodavadiya

Also, while cropping when make_subsize(...) > _resize(...) or resize(...) function run it throws WP_Error error_getting_dimensions.

IMO this is fine, as image_resize_dimensions() cannot calculate dimensions for a resized image if it's larger than the original image.

Last edited 13 months ago by antonvlasenko (previous) (diff)

#24 @antonvlasenko
13 months ago

Test Report

Patch tested: https://github.com/WordPress/wordpress-develop/pull/3985

Environment

  • OS: macOS 13.1
  • Web Server: Apache 2
  • PHP: 8.1.14
  • WordPress: 6.2-alpha-54642-src
  • Browser: Safari 16.2
  • Theme: Twenty Twenty-Three
  • No Active Plugins

Steps

  1. Upload a relatively large image to the Media Gallery (at least 2000x 1500 pixels). It's just easier to test this issue with a bigger image.
  2. Click on the image.
  3. Click on the "Edit Image" button below the image.
  4. Crop the image (using the "Crop" button above the image).
  5. A cropped image should be about 400 x 400 pixels in size.
  6. Save the cropped image (click on the "Save" button).

Actual Results

  1. ✅ WordPress displays the cropped image (and not the old version).
Last edited 13 months ago by antonvlasenko (previous) (diff)

#25 @antonvlasenko
13 months ago

  • Keywords reporter-feedback removed

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


13 months ago

#27 @costdev
12 months ago

  • Severity changed from critical to normal

@adamsilverstein As we're approaching 6.2 Beta 4, can you take a look at the latest comments in the next couple of days and see if this one is landable soon or if it should be moved to Future Release? Thanks!


  • Changing Severity from critical to normal, as this is an issue with which image is displayed, not with the cropping functionality itself.

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


12 months ago

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


12 months ago

#30 @adamsilverstein
12 months ago

  • Keywords commit added

#31 @adamsilverstein
12 months ago

We discussed this in the media chat today and reviewed the latest patch. I'll get this committed.

#32 @hellofromTonya
12 months ago

@adamsilverstein Beta 4, the last scheduled beta for this release, just happened. Is this fix essential for 6.2?

#33 @costdev
12 months ago

  • Milestone changed from 6.2 to 6.3

With Beta 4 released, and @adamsilverstein confirming via Slack that this isn't a critical fix for 6.2, moving this ticket to 6.3.

#34 @adamsilverstein
12 months ago

  • Milestone changed from 6.3 to 6.2

Since we are doing a beta 5 tomorrow I'm going to go ahead and commit this bug fix.

#35 @adamsilverstein
12 months ago

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

In 55470:

Media: improve display of cropped image in media editor.

Correctly display edits after you crop an image and return to the attachments page.

Prefer the full over the large size image on the edit image screen.

Props andy786, antpb, sanketchodavadiya, antonvlasenko, ironprogrammer.
Fixes #55070.

Note: See TracTickets for help on using tickets.