#55070 closed defect (bug) (fixed)
Image Editing Cropping is not working properly
Reported by: | andy786 | Owned by: | 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)
Change History (38)
This ticket was mentioned in Slack in #core-media by antpb. View the logs.
3 years ago
#2
@
3 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
@
3 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
@
3 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.
3 years ago
#5
- Keywords has-patch added; needs-patch removed
Trac ticket: https://core.trac.wordpress.org/ticket/55070
#6
@
3 years ago
Steps to test this PR:
- 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.
- Click on the image.
- Click on the "Edit Image" button below the image.
- Crop the image (using the "Crop" button above the image).
- A cropped image should be about 400 x 400 pixels in size.
- Save the cropped image (click on the "Save" button).
- You should be able to see the cropped image. Before the fix, WordPress would display the original image and not the cropped one.
- [Additional step] You can crop the image again and observe that it's displayed correctly.
anton-vlasenko commented on PR #2489:
3 years ago
#7
Steps to test this PR: https://core.trac.wordpress.org/ticket/55070#comment:6
#8
@
3 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
- Under Settings > Media Settings, check the currently configured "Large size" image dimensions (default is 1024x1024).
- Upload a very large image to Media > Media Library (anything over the configured "Large size" above should work).
- After upload, with the selected image, click the Edit Image button under the preview.
- 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. ✅
- Click the Save button. Observe that the image refreshes and shows the original un-cropped version. ❌
- 🛠 Apply the patch, then refresh the Media Library page.
- 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.)
- ℹ️ 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.)
- 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. ✅
- 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
#9
@
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.
This ticket was mentioned in Slack in #core-media by antpb. View the logs.
2 years ago
#11
follow-up:
↓ 12
@
2 years 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).
#12
in reply to:
↑ 11
@
2 years 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
@
2 years 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
@
2 years 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.
My 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.
This ticket was mentioned in Slack in #core-media by antpb. View the logs.
2 years ago
#17
@
2 years 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
@
2 years 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.
This ticket was mentioned in PR #3985 on WordPress/wordpress-develop by @adamsilverstein.
20 months ago
#19
Trac ticket: https://core.trac.wordpress.org/ticket/55070
#20
@
20 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.
#22
@
20 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
@
20 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.
#24
@
20 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
The test steps are taken from https://github.com/WordPress/wordpress-develop/pull/3560 's desription.
Steps
- 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.
- Click on the image.
- Click on the "Edit Image" button below the image.
- Crop the image (using the "Crop" button above the image).
- A cropped image should be about 400 x 400 pixels in size.
- Save the cropped image (click on the "Save" button).
Actual Results
- ✅ WordPress displays the cropped image (and not the old version).
This ticket was mentioned in Slack in #core-media by antpb. View the logs.
20 months ago
#27
@
20 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
fromcritical
tonormal
, 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.
20 months ago
This ticket was mentioned in Slack in #core-media by antpb. View the logs.
20 months ago
#31
@
20 months ago
We discussed this in the media chat today and reviewed the latest patch. I'll get this committed.
#32
@
20 months ago
@adamsilverstein Beta 4, the last scheduled beta for this release, just happened. Is this fix essential for 6.2?
#33
@
20 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
@
19 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.
@antonvlasenko commented on PR #2489:
12 months ago
#36
Closed in favor of https://github.com/WordPress/wordpress-develop/pull/3985.
Screen recorded video of the issue of image cropping