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

Fix NgOptimizedImage distortion check for padded images #49889

Conversation

MatthiasKunnen
Copy link
Contributor

@MatthiasKunnen MatthiasKunnen commented Apr 17, 2023

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

NgOptimizedImage reports distortion of an image by comparing the intrinsic aspect ratio to the rendered aspect ratio, however, for padded images, the rendered aspect ratio is calculated incorrectly. This results in non-distorted images being reported as distorted and vice-versa. This is due to the rendered aspect ratio being calculated from the clientWidth and clientHeight which includes the padding of the element.

Issue Number: N/A

What is the new behavior?

clientWidth and clientHeight have been replaced by getComputedStyle(img).getPropertyValue('width') (and height equivalent) and depending on the box-sizing, the padding is subtracted prior to ratio calculation. This always results in the correct ratio.

An advantage of using computed width over clientWidth (and same for height) is that computed width returns a decimal, in contrast to the integer for clientWidth, which allows for greater accuracy in determining the ratio of the rendered image which prevents false positive distortion checks. It might even be possible to lower the ASPECT_RATIO_TOLERANCE (currently set to 0.1) and reduce the amount of false negatives.

Play around with the calculations here: https://stackblitz.com/edit/angular-image-ratio-playground?file=src/main.ts. Scenarios are defined in main.ts and rendered showing the results of the various ways to calculate the ratio and whether the particular calculation is correct.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Test added for rounding errors

I've added a test that checks for a false positive distortion check that occurs when using integers for the dimensions. This is fixed by using the computed dimensions which are returned as decimals. While this is, in my opinion, a useful addition, I could remove it if it is not desired.

Updating playground code in angular codebase

I could update the playground at packages/core/test/bundling/image-directive/playground.ts with the setup of my stackblitz if desired. This would probably be another PR though.

Extract to separate function

Perhaps the image dimensions extraction would be better in a utils lib, or exported for reuse elsewhere. If so, please let me know and I'll update it.

…added images

The image distortion detection performed uses clientWidth/clientHeight which includes the padding.
This leads to images with padding being detected as distorted while they are not and distortion being masked by padding.
Due to assertNoImageDistortion using clientWidth and clientHeight, and these properties returning integers, rounding errors occur that exceed the aspect ratio tolerance.
Increasing the tolerance could hide actual distortion so correcting the calculation to use floats would be best and could even allow for a lower tolerance.
The original code uses clientWidth and clientHeight which returns the
width and height of the element including the padding. This results in
the aspect ratio being determined incorrectly and the image distortion
warning triggering. The new code uses getComputedStyle which returns
the width and height without padding.

Another advantage of using getComputedStyle is that, unlike clientWidth
and clientHeight, the number returned is a decimal which provides
greater accuracy. This could allow for lowering the ASPECT_RATIO_TOLERANCE.
@jessicajaniuk jessicajaniuk added the area: common Issues related to APIs in the @angular/common package label Apr 17, 2023
@ngbot ngbot bot added this to the Backlog milestone Apr 17, 2023
@jessicajaniuk jessicajaniuk added action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release labels Apr 17, 2023
@AndrewKushnir
Copy link
Contributor

// cc @atcastle

Copy link
Contributor

@atcastle atcastle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. The Stackblitz is very clear and shows how this will help prevent some false positives on the distortion check. In general I'm less worried about false negatives--the main point of the check is to alert people of a common error where the (required) height and width properties can cause distortion, not to prevent people from doing weird stuff with CSS and making their images look squashed or stretched or whatever. In the future, it might be worth looking into ways to heuristically disable this check, to avoid inconveniencing people who are intentionally distorting their images.

The PR's great though--thanks!

@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Apr 27, 2023
@pkozlowski-opensource
Copy link
Member

This PR was merged into the repository by commit 5a37928.

pkozlowski-opensource pushed a commit that referenced this pull request Apr 27, 2023
…added images (#49889)

The image distortion detection performed uses clientWidth/clientHeight which includes the padding.
This leads to images with padding being detected as distorted while they are not and distortion being masked by padding.

PR Close #49889
pkozlowski-opensource pushed a commit that referenced this pull request Apr 27, 2023
…49889)

Due to assertNoImageDistortion using clientWidth and clientHeight, and these properties returning integers, rounding errors occur that exceed the aspect ratio tolerance.
Increasing the tolerance could hide actual distortion so correcting the calculation to use floats would be best and could even allow for a lower tolerance.

PR Close #49889
pkozlowski-opensource pushed a commit that referenced this pull request Apr 27, 2023
…49889)

The original code uses clientWidth and clientHeight which returns the
width and height of the element including the padding. This results in
the aspect ratio being determined incorrectly and the image distortion
warning triggering. The new code uses getComputedStyle which returns
the width and height without padding.

Another advantage of using getComputedStyle is that, unlike clientWidth
and clientHeight, the number returned is a decimal which provides
greater accuracy. This could allow for lowering the ASPECT_RATIO_TOLERANCE.

PR Close #49889
pkozlowski-opensource pushed a commit that referenced this pull request Apr 27, 2023
…added images (#49889)

The image distortion detection performed uses clientWidth/clientHeight which includes the padding.
This leads to images with padding being detected as distorted while they are not and distortion being masked by padding.

PR Close #49889
pkozlowski-opensource pushed a commit that referenced this pull request Apr 27, 2023
…49889)

Due to assertNoImageDistortion using clientWidth and clientHeight, and these properties returning integers, rounding errors occur that exceed the aspect ratio tolerance.
Increasing the tolerance could hide actual distortion so correcting the calculation to use floats would be best and could even allow for a lower tolerance.

PR Close #49889
pkozlowski-opensource pushed a commit that referenced this pull request Apr 27, 2023
…49889)

The original code uses clientWidth and clientHeight which returns the
width and height of the element including the padding. This results in
the aspect ratio being determined incorrectly and the image distortion
warning triggering. The new code uses getComputedStyle which returns
the width and height without padding.

Another advantage of using getComputedStyle is that, unlike clientWidth
and clientHeight, the number returned is a decimal which provides
greater accuracy. This could allow for lowering the ASPECT_RATIO_TOLERANCE.

PR Close #49889
pkozlowski-opensource pushed a commit that referenced this pull request Apr 27, 2023
…49889)

Due to assertNoImageDistortion using clientWidth and clientHeight, and these properties returning integers, rounding errors occur that exceed the aspect ratio tolerance.
Increasing the tolerance could hide actual distortion so correcting the calculation to use floats would be best and could even allow for a lower tolerance.

PR Close #49889
pkozlowski-opensource pushed a commit that referenced this pull request Apr 27, 2023
…49889)

The original code uses clientWidth and clientHeight which returns the
width and height of the element including the padding. This results in
the aspect ratio being determined incorrectly and the image distortion
warning triggering. The new code uses getComputedStyle which returns
the width and height without padding.

Another advantage of using getComputedStyle is that, unlike clientWidth
and clientHeight, the number returned is a decimal which provides
greater accuracy. This could allow for lowering the ASPECT_RATIO_TOLERANCE.

PR Close #49889
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators May 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: common Issues related to APIs in the @angular/common package common: image directive target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants