fix: focal and cropping issues, adds test (#4039)

.vscode/launch.json

@@ -47,6 +47,13 @@
       "request": "launch",
       "type": "node-terminal"
     },
+    {
+      "command": "pnpm run dev uploads",
+      "cwd": "${workspaceFolder}",
+      "name": "Run Dev Uploads",
+      "request": "launch",
+      "type": "node-terminal"
+    },
     {
       "command": "PAYLOAD_BUNDLER=vite pnpm run dev fields",
       "cwd": "${workspaceFolder}",

packages/payload/src/admin/components/elements/EditUpload/index.tsx

@@ -83,7 +83,7 @@ export const EditUpload: React.FC<{
     setFormQueryParams({
       ...formQueryParams,
       uploadEdits: {
-        crop: crop ? crop : undefined,
+        crop: crop || undefined,
         focalPoint: pointPosition ? pointPosition : undefined,
       },
     })

packages/payload/src/uploads/generateFileData.ts

@@ -99,7 +99,7 @@ export const generateFileData = async <T>({
     let fileBuffer: { data: Buffer; info: OutputInfo }
     let ext
     let mime: string
-    const isSharpRequired =
+    const fileHasAdjustments =
       fileSupportsResize &&
       Boolean(resizeOptions || formatOptions || trimOptions || file.tempFilePath)
 
@@ -107,7 +107,7 @@ export const generateFileData = async <T>({
 
     if (fileIsAnimated) sharpOptions.animated = true
 
-    if (isSharpRequired) {
+    if (fileHasAdjustments) {
       if (file.tempFilePath) {
         sharpFile = sharp(file.tempFilePath, sharpOptions).rotate() // pass rotate() to auto-rotate based on EXIF data. https://github.com/payloadcms/payload/pull/3081
       } else {
@@ -174,7 +174,7 @@ export const generateFileData = async <T>({
     fileData.filename = fsSafeName
     let fileForResize = file
 
-    if (isSharpRequired && cropData) {
+    if (cropData) {
       const { data: croppedImage, info } = await cropImage({ cropData, dimensions, file })
 
       filesToSave.push({

packages/payload/src/uploads/imageResizer.ts

@@ -113,36 +113,83 @@ const createResult = (
  *
  * @param resizeConfig - object containing the requested dimensions and resize options
  * @param original - the original image size
- * @returns true if the image needs to be resized, false otherwise
+ * @returns true if resizing is not needed, false otherwise
  */
-const needsResize = (
+const preventResize = (
   { height: desiredHeight, width: desiredWidth, withoutEnlargement, withoutReduction }: ImageSize,
   original: ProbedImageSize,
 ): boolean => {
-  // allow enlargement or prevent reduction (our default is to prevent
-  // enlargement and allow reduction)
-  if (withoutEnlargement !== undefined || withoutReduction !== undefined) {
-    return true // needs resize
+  // default is to allow reduction
+  if (withoutReduction !== undefined) {
+    return false // needs resize
+  }
+
+  // default is to prevent enlargement
+  if (withoutEnlargement !== undefined) {
+    return false // needs resize
   }
 
   const isWidthOrHeightNotDefined = !desiredHeight || !desiredWidth
-
   if (isWidthOrHeightNotDefined) {
     // If with and height are not defined, it means there is a format conversion
     // and the image needs to be "resized" (transformed).
-    return true // needs resize
+    return false // needs resize
   }
 
-  const hasInsufficientWidth = original.width < desiredWidth
-  const hasInsufficientHeight = original.height < desiredHeight
+  const hasInsufficientWidth = desiredWidth > original.width
+  const hasInsufficientHeight = desiredHeight > original.height
   if (hasInsufficientWidth && hasInsufficientHeight) {
     // doesn't need resize - prevent enlargement. This should only happen if both width and height are insufficient.
     // if only one dimension is insufficient and the other is sufficient, resizing needs to happen, as the image
     // should be resized to the sufficient dimension.
-    return false
+    return true // do not create a new size
   }
 
-  return true // needs resize
+  return false // needs resize
+}
+
+/**
+ * Check if the image should be passed directly to sharp without payload adjusting properties.
+ *
+ * @param resizeConfig - object containing the requested dimensions and resize options
+ * @param original - the original image size
+ * @returns true if the image should passed directly to sharp
+ */
+const applyPayloadAdjustments = (
+  { height, width, withoutEnlargement, withoutReduction }: ImageSize,
+  original: ProbedImageSize,
+) => {
+  if (!isNumber(height) && !isNumber(width)) return false
+
+  const targetAspectRatio = width / height
+  const originalAspectRatio = original.width / original.height
+  if (originalAspectRatio === targetAspectRatio) return false
+
+  const skipEnlargement = withoutEnlargement && (original.height < height || original.width < width)
+  const skipReduction = withoutReduction && (original.height > height || original.width > width)
+  if (skipEnlargement || skipReduction) return false
+
+  return true
+}
+
+/**
+ * Sanitize the resize config. If the resize config has the `withoutReduction`
+ * property set to true, the `fit` and `position` properties will be set to `contain`
+ * and `top left` respectively.
+ *
+ * @param resizeConfig - the resize config
+ * @returns a sanitized resize config
+ */
+const sanitizeResizeConfig = (resizeConfig: ImageSize): ImageSize => {
+  if (resizeConfig.withoutReduction) {
+    return {
+      ...resizeConfig,
+      // Why fit `contain` should also be set to https://github.com/lovell/sharp/issues/3595
+      fit: resizeConfig?.fit || 'contain',
+      position: resizeConfig?.position || 'left top',
+    }
+  }
+  return resizeConfig
 }
 
 /**
@@ -176,66 +223,65 @@ export default async function resizeAndTransformImageSizes({
 
   const results: ImageSizesResult[] = await Promise.all(
     imageSizes.map(async (imageResizeConfig): Promise<ImageSizesResult> => {
+      imageResizeConfig = sanitizeResizeConfig(imageResizeConfig)
+
       // This checks if a resize should happen. If not, the resized image will be
       // skipped COMPLETELY and thus will not be included in the resulting images.
       // All further format/trim options will thus be skipped as well.
-      if (!needsResize(imageResizeConfig, dimensions)) {
+      if (preventResize(imageResizeConfig, dimensions)) {
         return createResult(imageResizeConfig.name)
       }
-      let resized = sharpBase.clone()
 
-      const hasEdits = req.query?.uploadEdits
+      const imageToResize = sharpBase.clone()
+      let resized = imageToResize.resize(imageResizeConfig)
 
-      if (hasEdits && imageResizeConfig.width && imageResizeConfig.height) {
-        const { height, width } = imageResizeConfig
-
-        const targetAspectRatio = width / height
+      if (
+        req.query?.uploadEdits?.focalPoint &&
+        applyPayloadAdjustments(imageResizeConfig, dimensions)
+      ) {
+        const { height: resizeHeight, width: resizeWidth } = imageResizeConfig
+        const resizeAspectRatio = resizeWidth / resizeHeight
         const originalAspectRatio = dimensions.width / dimensions.height
+        const prioritizeHeight = resizeAspectRatio < originalAspectRatio
 
-        if (originalAspectRatio === targetAspectRatio) {
-          resized = resized.resize(imageResizeConfig)
-        } else {
-          const focalPoint = {
-            x: 0.5,
-            y: 0.5,
-          }
+        // Scale the image up or down to fit the resize dimensions
+        const scaledImage = imageToResize.resize({
+          height: prioritizeHeight ? resizeHeight : null,
+          width: prioritizeHeight ? null : resizeWidth,
+        })
+        const { info: scaledImageInfo } = await scaledImage.toBuffer({ resolveWithObject: true })
 
-          if (req.query.uploadEdits?.focalPoint) {
-            if (isNumber(req.query.uploadEdits.focalPoint?.x)) {
-              focalPoint.x = req.query.uploadEdits.focalPoint.x
-            }
-            if (isNumber(req.query.uploadEdits.focalPoint?.y)) {
-              focalPoint.y = req.query.uploadEdits.focalPoint.y
-            }
-          }
-
-          const prioritizeHeight = originalAspectRatio > targetAspectRatio
-
-          const { info } = await resized
-            .resize({
-              height: prioritizeHeight ? height : null,
-              width: prioritizeHeight ? null : width,
-            })
-            .toBuffer({ resolveWithObject: true })
-
-          const maxOffsetX = Math.max(info.width - width, 0)
-          const maxOffsetY = Math.max(info.height - height, 0)
-
-          const focalPointX = Math.floor((info.width / 100) * focalPoint.x)
-          const focalPointY = Math.floor((info.height / 100) * focalPoint.y)
-
-          const offsetX = Math.min(Math.max(focalPointX - width / 2, 0), maxOffsetX)
-          const offsetY = Math.min(Math.max(focalPointY - height / 2, 0), maxOffsetY)
-
-          resized = resized.extract({
-            height,
-            left: offsetX,
-            top: offsetY,
-            width,
-          })
+        // Focal point adjustments
+        const focalPoint = {
+          x: isNumber(req.query.uploadEdits.focalPoint?.x)
+            ? req.query.uploadEdits.focalPoint.x
+            : 50,
+          y: isNumber(req.query.uploadEdits.focalPoint?.y)
+            ? req.query.uploadEdits.focalPoint.y
+            : 50,
         }
-      } else {
-        resized = resized.resize(imageResizeConfig)
+
+        const safeResizeWidth = resizeWidth ?? scaledImageInfo.width
+        const maxOffsetX = scaledImageInfo.width - safeResizeWidth
+        const leftFocalEdge = Math.round(
+          scaledImageInfo.width * (focalPoint.x / 100) - safeResizeWidth / 2,
+        )
+        const safeOffsetX = Math.min(Math.max(0, leftFocalEdge), maxOffsetX)
+
+        const safeResizeHeight = resizeHeight ?? scaledImageInfo.height
+        const maxOffsetY = scaledImageInfo.height - safeResizeHeight
+        const topFocalEdge = Math.round(
+          scaledImageInfo.height * (focalPoint.y / 100) - safeResizeHeight / 2,
+        )
+        const safeOffsetY = Math.min(Math.max(0, topFocalEdge), maxOffsetY)
+
+        // extract the focal area from the scaled image
+        resized = scaledImage.extract({
+          height: safeResizeHeight,
+          left: safeOffsetX,
+          top: safeOffsetY,
+          width: safeResizeWidth,
+        })
       }
 
       if (imageResizeConfig.formatOptions) {

test/uploads/config.ts

@@ -330,7 +330,6 @@ export default buildConfigWithDefaults({
             height: 80,
             formatOptions: { format: 'jpg', options: { quality: 90 } },
             withoutReduction: true,
-            fit: 'contain',
           },
           {
             name: 'resizedLarger',
@@ -341,9 +340,7 @@ export default buildConfigWithDefaults({
             name: 'resizedSmaller',
             width: 180,
             height: 50,
-            // Why fit `contain` should also be set to https://github.com/lovell/sharp/issues/3595
             withoutReduction: true,
-            fit: 'contain',
           },
         ],
       },

test/uploads/e2e.spec.ts

@@ -10,11 +10,13 @@ import wait from '../../packages/payload/src/utilities/wait'
 import { saveDocAndAssert } from '../helpers'
 import { AdminUrlUtil } from '../helpers/adminUrlUtil'
 import { initPayloadE2E } from '../helpers/configHelpers'
+import { RESTClient } from '../helpers/rest'
 import { adminThumbnailSrc } from './collections/admin-thumbnail'
 import { adminThumbnailSlug, audioSlug, mediaSlug, relationSlug } from './shared'
 
 const { beforeAll, describe } = test
 
+let client: RESTClient
 let mediaURL: AdminUrlUtil
 let audioURL: AdminUrlUtil
 let relationURL: AdminUrlUtil
@@ -27,6 +29,8 @@ describe('uploads', () => {
 
   beforeAll(async ({ browser }) => {
     const { serverURL } = await initPayloadE2E(__dirname)
+    client = new RESTClient(null, { serverURL, defaultSlug: 'users' })
+    await client.login()
 
     mediaURL = new AdminUrlUtil(serverURL, mediaSlug)
     audioURL = new AdminUrlUtil(serverURL, audioSlug)
@@ -47,7 +51,7 @@ describe('uploads', () => {
       },
     })
 
-    pngDoc = findPNG.docs[0] as Media
+    pngDoc = findPNG.docs[0] as unknown as Media
 
     const findAudio = await payload.find({
       collection: audioSlug,
@@ -55,7 +59,7 @@ describe('uploads', () => {
       pagination: false,
     })
 
-    audioDoc = findAudio.docs[0] as Media
+    audioDoc = findAudio.docs[0] as unknown as Media
   })
 
   test('should see upload filename in relation list', async () => {
@@ -88,10 +92,6 @@ describe('uploads', () => {
     await saveDocAndAssert(page)
   })
 
-  test('should update file upload', async () => {
-    await page.goto(mediaURL.edit(pngDoc.id))
-  })
-
   test('should show resized images', async () => {
     await page.goto(mediaURL.edit(pngDoc.id))
 
@@ -192,4 +192,79 @@ describe('uploads', () => {
     const audioUploadImage = page.locator('tr.row-2 .thumbnail img')
     expect(await audioUploadImage.getAttribute('src')).toContain(adminThumbnailSrc)
   })
+
+  describe('image manipulation', () => {
+    test('should crop image correctly', async () => {
+      const positions = {
+        'top-left': {
+          focalX: 25,
+          focalY: 25,
+          dragX: 0,
+          dragY: 0,
+        },
+        'bottom-right': {
+          focalX: 75,
+          focalY: 75,
+          dragX: 800,
+          dragY: 800,
+        },
+      }
+      const createFocalCrop = async (page: Page, position: 'bottom-right' | 'top-left') => {
+        const { focalX, focalY, dragX, dragY } = positions[position]
+        await page.goto(mediaURL.create)
+
+        // select and upload file
+        const fileChooserPromise = page.waitForEvent('filechooser')
+        await page.getByText('Select a file').click()
+        const fileChooser = await fileChooserPromise
+        await fileChooser.setFiles(path.join(__dirname, 'test-image.jpg'))
+        await page.locator('.file-field__edit').click()
+
+        // set crop
+        await page.locator('.edit-upload__input input[name="Width (px)"]').fill('400')
+        await page.locator('.edit-upload__input input[name="Height (px)"]').fill('400')
+        // set focal point
+        await page.locator('.edit-upload__input input[name="X %"]').fill('25') // init left focal point
+        await page.locator('.edit-upload__input input[name="Y %"]').fill('25') // init top focal point
+
+        // hover the crop selection, position mouse outside of focal point hitbox
+        await page.locator('.ReactCrop__crop-selection').hover({ position: { x: 100, y: 100 } })
+        await page.mouse.down() // start drag
+        await page.mouse.move(dragX, dragY) // drag selection to the lower right corner
+        await page.mouse.up() // release drag
+
+        // focal point should reset to center
+        await expect(page.locator('.edit-upload__input input[name="X %"]')).toHaveValue(`${focalX}`)
+        await expect(page.locator('.edit-upload__input input[name="Y %"]')).toHaveValue(`${focalY}`)
+
+        await page.locator('button:has-text("Apply Changes")').click()
+        await page.waitForSelector('button#action-save')
+        await page.locator('button#action-save').click()
+      }
+
+      await createFocalCrop(page, 'bottom-right') // green square
+      await wait(1000) // wait for edit view navigation (saving images)
+      // get the ID of the doc
+      const greenSquareMediaID = page.url().split('/').pop()
+      await createFocalCrop(page, 'top-left') // red square
+      await wait(1000) // wait for edit view navigation (saving images)
+      const redSquareMediaID = page.url().split('/').pop()
+
+      const { doc: greenDoc } = await client.findByID({
+        id: greenSquareMediaID,
+        slug: mediaSlug,
+        auth: true,
+      })
+
+      const { doc: redDoc } = await client.findByID({
+        id: redSquareMediaID,
+        slug: mediaSlug,
+        auth: true,
+      })
+
+      // green and red squares should have different sizes (colors make the difference)
+      expect(greenDoc.filesize).toEqual(1205)
+      expect(redDoc.filesize).toEqual(1207)
+    })
+  })
 })