Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Full-height vertical crops not loading correctly in cropper #1194

Closed
IllyaMoskvin opened this issue Oct 13, 2021 · 1 comment · Fixed by #2297
Closed

Full-height vertical crops not loading correctly in cropper #1194

IllyaMoskvin opened this issue Oct 13, 2021 · 1 comment · Fixed by #2297
Labels
impact: low a minor issue for some people type: bug Something isn't working

Comments

@IllyaMoskvin
Copy link
Contributor

IllyaMoskvin commented Oct 13, 2021

Description

We have some crops with vertical aspect ratios, e.g. 9:16 instead of 16:9. We've noticed that the cropper tool in the CMS does not seem to load these crops correctly. Specifically, when these crops extend to cover the full height of the image, their x-position is not loaded correctly. Upon opening the cropper, it'll be reset to the default x-position.

We confirmed that this is the case both when repeatedly opening the cropper without refreshing the page, as well as when we first open the cropper after loading a resource to edit.

If the crop is downsized to cover less than the full height of the image, it works fine.

The crop appears to save correctly. We see the effect on the frontend and by looking at the cropped thumbnail in the CMS.

Full-width crops appear to be working fine.

I confirmed that the issue is not affecting crops that have horizontal aspect ratios that are smaller than the aspect ratio of the image (e.g. 4:3 crop on a 16:9 image). So it's not just about the crop being full-height, it has to be a vertical aspect ratio, too.

Steps to reproduce

  1. Create a model with the following media definition:
    public $mediasParams = [
        'hero' => [
            'default' => [
                [
                    'name' => 'default',
                    'ratio' => 9 / 16,
                ],
            ],
        ],
    ];
    
  2. Add a corresponding @formField('medias') field.
  3. Go to the CMS.
  4. Select an image for the field.
  5. Open the crop dialog.
  6. Move the crop, but do not downsize it. Make sure it covers the full height of the image.
  7. Click update.
  8. Open the crop dialog again.
  9. Note how the crop position has been reset.

Alternatively, instead of step 8:

  1. Save your changes.
  2. Reload the page.
  3. Open the crop dialog.

Here's a video of the issue:

https://drive.google.com/file/d/1e8WyF8TI7vjzq8cDQiZDPwGTkG0vWtpI/view?usp=sharing

(You may need to download it, Google Drive has a hard time with video previews.)

Expected result

Full-height vertical crops load correctly.

Actual result

As described above.

Versions

Twill: 2.4.0
Laravel: 6.20.34
PHP: 7.2
DB: Postgres 13

@pboivin
Copy link
Contributor

pboivin commented Oct 18, 2021

Hi @IllyaMoskvin, I can confirm this bug. Thank you for the well detailed description and video! 🙏️

@ifox ifox added impact: low a minor issue for some people type: bug Something isn't working labels Feb 6, 2022
13twelve added a commit that referenced this issue Jun 23, 2023
attempting to fix #1194

which seems to be an issue with cropper.js
(fengyuanchen/cropperjs#1057)

and ultimately is likely caused by rounding errors

It seems setting individual crop settings instead
of attempting to set them all in one go fixes it,
at least within Twill
ifox pushed a commit that referenced this issue Nov 29, 2023
attempting to fix #1194

which seems to be an issue with cropper.js
(fengyuanchen/cropperjs#1057)

and ultimately is likely caused by rounding errors

It seems setting individual crop settings instead
of attempting to set them all in one go fixes it,
at least within Twill
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact: low a minor issue for some people type: bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants