Make WordPress Core

Opened 9 years ago

Last modified 12 days ago

#32282 reopened defect (bug)

WordPress image cropping is buggy

Reported by: jossnaz's profile Jossnaz Owned by: joedolson's profile joedolson
Milestone: 6.7 Priority: normal
Severity: normal Version: 4.2.2
Component: Media Keywords: has-patch commit
Focuses: administration Cc:

Description

see here

http://wordpress.stackexchange.com/questions/187577/cropping-image-results-in-image-being-too-small?noredirect=1#comment273421_187577

text copied from there:

This is the wordpress version 4.2.2 powell

I crop 1500 x 1000 and then press on crop, then save.

Resulting image size:

1497 x 1000

I don't know, I somehow hoped that wordpress is able to crop correctly.

The original image can be found here:

http://img.youtube.com/vi/zXgyoDAuaH0/maxresdefault.jpg

What I did is downsize to .... x 1000 then try to crop with selection manually entering 1500 x 1000

You can actually see the bug earlier, when you enter 1500 x 1000 and then click on the selection to drag it around (right, drag the selection, not change size) it results in the number 1500 falling down to 1498.

Attachments (2)

32282.diff (7.2 KB) - added by johnillo 3 years ago.
32282.patch (7.3 KB) - added by shailu25 6 weeks ago.
Patch Refreshed.

Download all attachments as: .zip

Change History (42)

#1 @rachelbaker
8 years ago

  • Focuses administration added
  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

I successfully reproduced this bug using the latest trunk from 4.5 and the provided image. After manually setting a crop size of 1500x1000 the image size then changed to 1497x998 once I pressed the crop button, or tried to move the selection area.

https://cldup.com/-tX2uK8Tcp.png

#3 @desrosj
7 years ago

  • Summary changed from Wordpress image cropping is buggy to WordPress image cropping is buggy

@johnillo
3 years ago

#4 @Presskopp
3 years ago

  • Keywords has-patch added; needs-patch removed

#5 @johnillo
3 years ago

  • Crop the image in 100% scale to avoid rounding errors.
  • Don't recompute selection values when selection is dragged only.
  • Fix the issue where the selection values are off after the image is scaled down or restored.
  • Keep aspect ratio when holding the shift key.
Last edited 3 years ago by johnillo (previous) (diff)

#6 @joedolson
9 months ago

  • Milestone changed from Future Release to 6.5

Confirmed still an issue.

#7 @joedolson
9 months ago

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

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


6 months ago

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


5 months ago

#10 @joedolson
5 months ago

  • Keywords needs-refresh added

#11 @swissspidy
5 months ago

  • Milestone changed from 6.5 to 6.6

Punting due to lack of activity & new patch.

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


7 weeks ago

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


6 weeks ago

@shailu25
6 weeks ago

Patch Refreshed.

#14 @shailu25
6 weeks ago

  • Keywords needs-refresh removed

Refreshed the Previous patch.

Last edited 6 weeks ago by shailu25 (previous) (diff)

#15 @hmbashar
5 weeks ago

  • Keywords needs-testing added

#16 @sudipatel007
4 weeks ago

  • Keywords needs-testing removed

Test Report


Description

Now, you can crop image using WordPress core functionality

Patch has been tested and working as expected.

Environment

  • WordPress: 6.5.5-alpha-58404
  • PHP: 8.1.23
  • Server: nginx/1.16.0
  • Database: mysqli (Server: 8.0.16 / Client: mysqlnd 8.1.23)
  • Browser: Chrome 126.0.0.0 (macOS)
  • Theme: Twenty Nineteen 2.8

Test Results

Crop image is working as expected.

Video - https://app.screencast.com/fIjJ5ziAjCcqg

Last edited 4 weeks ago by sudipatel007 (previous) (diff)

#18 @joedolson
3 weeks ago

PR updates the patch to meet coding standards, correct a couple errors, & adjust some code styling for consistency.

Tested and confirmed this resolves the functional issue with cropping.

#19 @joedolson
3 weeks ago

  • Keywords commit added

#20 @joedolson
3 weeks ago

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

In 58456:

Media: Fix admin image cropping calculations.

The admin image editor crop function introduced rounding errors by using a scaled image to calculate values. Fix uses the image at 100% scale for calculations. Also avoid recalculating selection when the selection position is changed, and prevent incorrect values after scaling or restoration.

Props Jossnaz, johnillo, shailu25, rachelbaker, sudipatel007, joedolson.
Fixes #32282.

@joedolson commented on PR #6875:


3 weeks ago
#21

In r58456

@kevin940726 commented on PR #6875:


3 weeks ago
#22

Hi @joedolson!

Recently, after this PR (commit 25e7dc6c10 and 4f175e167e), the e2e tests for image cropping in gutenberg started failing. I tested it manually and it seems like a legit bug. Here's a recording of the reproduction:

https://github.com/WordPress/wordpress-develop/assets/7753001/6163efe2-600b-441f-9ed3-f554eb12eccc

Testing instructions:

  1. Add an image block to a post and upload an image.
  2. Select "Crop" in the block toolbar.
  3. Select "Aspect ratio" in the block toolbar, then select an option from the dropdown list.
  4. Hit "Apply" in the block toolbar.
  5. Expect the image to be cropped, but it doesn't.

#23 @andrewserong
3 weeks ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I'm just reopening this ticket as a bug has been flagged in the block editor's image cropping tools. @joedolson would it be worth reverting this change for 6.6, or is there another fix that could be done? Thanks!

#24 @hmbashar
3 weeks ago

Test Report

Patch tested: 32282.patch

Environment

  • WordPress: 6.6-beta1-58337-src
  • PHP: 8.3.8
  • Server: nginx/1.25.4
  • Database: mysqli (Server: 8.3.0 / Client: mysqlnd 8.3.8)
  • Browser: Firefox 127.0
  • OS: macOS
  • Theme: Twenty Twenty-Four 1.1
  • MU Plugins: None activated
  • Plugins:
    • Test Reports 1.1.0

Actual Results

  1. ✅ Issue resolved with patch.

Check Video https://www.berrycast.com/conversations/ae908bc4-8b0c-5814-b85a-089851a11bee

#25 @oglekler
3 weeks ago

@audrasjb can you take a look at this one, please?

#26 @oglekler
3 weeks ago

This ticket is staying in the milestone due to the commit that most likely needs to be reverted.

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


3 weeks ago

#28 follow-up: @jorbin
3 weeks ago

In 58571:

Media: unfix admin image cropping calculations.

[58456] introduced some failures to the automated test system that indicate this fix is incomplete.

See #32282.
Props hellofromtonya, audrasjb, andrewserong, kevin940726, oglekler.

#29 @sabernhardt
3 weeks ago

  • Keywords commit removed
  • Milestone changed from 6.6 to 6.7

Moving to next milestone after commit was reverted

This ticket was mentioned in Slack in #core-editor by andrewserong. View the logs.


3 weeks ago

#31 @andrewserong
3 weeks ago

Now that 6.6 RC 1 is released, I've retested image cropping in the block editor, and it appears to still be failing. I've opened an issue in Github with reproduction steps: https://github.com/WordPress/gutenberg/issues/62855

As far as I can tell, 6.6 RC 1 included the above revert, and unfortunately I haven't been able to find anything obvious that could be the culprit. The values being sent to the media/edit REST API endpoint appear to be unchanged from the block editor side.

Are there any other recent server-side image editing changes that could have affected this?

This ticket was mentioned in Slack in #core-editor by andrewserong. View the logs.


3 weeks ago

#33 @andrewserong
3 weeks ago

Update: after testing locally, I think reverting a different (but similar) change from #59782 (https://github.com/WordPress/wordpress-develop/pull/6876) seems to get the cropping working in the block editor again. I'll comment over on that ticket, too.

This ticket was mentioned in PR #6909 on WordPress/wordpress-develop by @andrewserong.


3 weeks ago
#34

## What

Fixes cropping tools in the block editor. Related Gutenberg issue: https://github.com/WordPress/gutenberg/issues/62855

## Why

As of https://github.com/WordPress/wordpress-develop/commit/4f175e167ebd33d68ebe347fcc376f1342d9e873 / https://github.com/WordPress/wordpress-develop/pull/6876, the width and height values are rounded and cast to an int before the comparison to see if the target width and height differ from the original width and height.

Since they are now ints, it exposes a bug where the && of the if conditional meant that if you were only cropping in one dimension, the check wouldn't pass, and cropping would not occur.

In the block editor, the cropping tools are aspect ratio based, so one of the dimensions will always match that of the source image. Therefore, now that the values are cast as ints, we need to update the condition that allows a cropping to occur. In this case, I believe it should be || instead of &&. If either width or height is different from the source image, then we should allow a crop.

## How to test

  1. Add an image block to a post in the block editor
  2. Click on the Crop tool in the block toolbar
  3. Select a Crop from the aspect ratio dropdown
  4. Click Apply
  5. The aspect ratio should be applied in the final image
  6. Save and load the post on the site frontend and double check that the crop is correct

Trac ticket: https://core.trac.wordpress.org/ticket/32282

#35 @joedolson
3 weeks ago

  • Keywords commit added

Given the further research since the revert, it sounds like this change is actually fine, and can go back in at any time. If that's not how you see it, @andrewserong, let me know. Otherwise, I'm marking this for commit again.

#36 in reply to: ↑ 28 @hmbashar
3 weeks ago

Replying to jorbin:

In 58571:

Media: unfix admin image cropping calculations.

[58456] introduced some failures to the automated test system that indicate this fix is incomplete.

See #32282.
Props hellofromtonya, audrasjb, andrewserong, kevin940726, oglekler.

Hello @jorbin
I'm new to contributing, so I may have made a mistake. Can you explain why I didn't receive the props? If you clarify, I can ensure and follow the instructions in the future.

@andrewserong commented on PR #6909:


3 weeks ago
#37

Thanks for the feedback @jrfnl! I've added a unit test, copying the existing one for image cropping, but updating it to include cropping on only one axis.

@andrewserong commented on PR #6909:


2 weeks ago
#38

@jrfnl or @joedolson this PR is ready for another review and commit if it's all looking okay. It'd be great to get this fix in for the next 6.6 RC if we can.

@SergeyBiryukov commented on PR #6909:


2 weeks ago
#39

Thanks for the PR! Merged in r58612.

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


12 days ago

Note: See TracTickets for help on using tickets.