Make WordPress Core

Opened 2 weeks ago

Closed 4 days ago

Last modified 3 days ago

#61514 closed defect (bug) (fixed)

Media attachments REST API endpoint: Fix cropping on one axis only

Reported by: andrewserong's profile andrewserong Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.6 Priority: normal
Severity: normal Version: trunk
Component: REST API Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

As discussed in #59782, a fix for casting values as int in the attachments REST API endpoint revealed a bug in the endpoint for image cropping where if requesting a crop on only one axis, the crop would fail to take effect.

A proposed solution is to update the logic in the endpoint so that a crop is valid if at least one dimension of the target image size is different from the source image size, rather than requiring both dimensions to be different.

To reproduce the issue, see the reproduction steps in https://github.com/WordPress/gutenberg/issues/62855 — in the block editor, in 6.6 RC 1, cropping the image block fails because the block editor uses an aspect-ratio based crop where one dimension is always the same as the source image.

Change History (12)

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


2 weeks ago
#1

  • Keywords has-patch has-unit-tests added

## What

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

## Why

Currently, the cropping tool in the block editor doesn't apply the selected crop.

As of https://github.com/WordPress/wordpress-develop/commit/4f175e167ebd33d68ebe347fcc376f1342d9e873 / https://github.com/WordPress/wordpress-develop/pull/6876, the width and height cropping 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/61514

@andrewserong commented on PR #6909:


2 weeks ago
#2

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:


12 days ago
#3

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

#4 @SergeyBiryukov
11 days ago

  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#5 @SergeyBiryukov
11 days ago

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

In 58612:

REST API: Correct image cropping tools in the block editor.

As of [58457], the width and height cropping values are cast to an integer before the comparison to see if the target width and height differ from the original width and height.

Since they are now integers, 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 integers, the condition that allows a cropping to occur needs to be updated. If either width or height is different from the source image, then a crop should be allowed.

Follow-up to [50124], [58457].

Props andrewserong, jrf, kevin940726.
Fixes #61514. See #59782.

@SergeyBiryukov commented on PR #6909:


11 days ago
#6

Thanks for the PR! Merged in r58612.

#7 @SergeyBiryukov
11 days ago

  • Keywords dev-feedback added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backporting to the 6.6 branch after a second committer's review.

#8 @davidbaumwald
10 days ago

  • Keywords dev-reviewed added; dev-feedback removed

@andrewserong Sorry this didn't get picked up in time for RC2. This can go into RC3 though. Signing off so it can be backported to the 6.6 branch.

#9 @andrewserong
4 days ago

Thanks @davidbaumwald! Are you or @SergeyBiryukov able to commit this to the 6.6 branch? Just double checking to make sure this fix gets in for RC3.

#10 @davidbaumwald
4 days ago

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

In 58692:

REST API: Correct image cropping tools in the block editor.

As of [58457], the width and height cropping values are cast to an integer before the comparison to see if the target width and height differ from the original width and height.
Since they are now integers, 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 integers, the condition that allows a cropping to occur needs to be updated. If either width or height is different from the source image, then a crop should be allowed.

Follow-up to [50124], [58457].

Reviewed by davidbaumwald.
Merges [58612] to the 6.6 branch.

Props andrewserong, jrf, kevin940726.
Fixes #61514. See #59782.

#11 @davidbaumwald
4 days ago

  • Keywords dev-reviewed removed

#12 @ellatrix
3 days ago

#61612 was marked as a duplicate.

Note: See TracTickets for help on using tickets.