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: avoid resize loop when browser zoom is set to 90% #10971

Merged
merged 10 commits into from Dec 17, 2022

Conversation

gbaron
Copy link
Contributor

@gbaron gbaron commented Dec 14, 2022

Following up on the discussion in #10951
I opened this PR to demonstrate the issue and hopefully fix it.

The PR includes a new test that reproduces the issue.
The test currently fails with the following error:

[test-ci-*karma]     ✗ retinaScale should not change chart size
[test-ci-*karma] 	Error: Expected 798.8889100522176 to be 800.
[test-ci-*karma] 	    at <Jasmine>
[test-ci-*karma] 	    at UserContext.<anonymous> (test/specs/helpers.dom.tests.js:276:25)
[test-ci-*karma] 	    at <Jasmine>
[test-ci-*karma]
[test-ci-*karma] 	Error: Expected 398.88889945583605 to be 400.
[test-ci-*karma] 	    at <Jasmine>
[test-ci-*karma] 	    at UserContext.<anonymous> (test/specs/helpers.dom.tests.js:277:26)
[test-ci-*karma] 	    at <Jasmine>

@kurkle - is it safe to assume that helpers.retinaScale shouldn't change the chart size or did I misunderstand the function purpose?

…elRatio the assignment would cause the size to reduce by 1px. Since it's called from the ResizeObserver it will be stuck in a loop that constantly reduce the size of the chart and canvas.
@gbaron
Copy link
Contributor Author

gbaron commented Dec 15, 2022

Should resolve #10951.
This PR solves one of the scenarios that created a constant resize loop and decrease the chart size.
There could be other scenarios that I'm not aware of.

@gbaron gbaron marked this pull request as ready for review December 15, 2022 19:24
@kurkle
Copy link
Member

kurkle commented Dec 15, 2022

The other scenario is here: #9015
No tests were added there, so this change could make the chart blurry again.

Would you be able to consider that case here too? I'd think the fix is actually something else.

@etimberg
Copy link
Member

I'm a little worried about this even with a test added. Would it be better to consider reverting the PR that broke this in v4?

@gbaron
Copy link
Contributor Author

gbaron commented Dec 16, 2022

Reading #9015:

When canvas.height and canvas.width are set, the numbers are rounded to
integers. The same rounding must be applied to canvas.style.height and
canvas.style.width to avoid scaling of the canvas which would lead to
blurring.

Based on my experiments, I suspect that the current implementation doesn't always achieve this.
My interpretation of the current code is:

canvas.style.height = `${Math.floor(chart.height * pixelRatio) / pixelRatio}px`;
canvas.style.width = `${Math.floor(chart.width * pixelRatio) / pixelRatio}px`;

When Zoom is 90%, pixelRatio is 0.8999999761581421 resulting in a non-integer value assigned to canvas.style.height.

@gbaron
Copy link
Contributor Author

gbaron commented Dec 16, 2022

In general, there are a few different ways to fix this issue. The reason I choose this change is that I couldn't think of a reason for the assignment back to chart.width and chart.height.

Same as above, the current code is effectively setting chart.height and chart.width as follows:

chart.height = Math.floor(chart.height * pixelRatio) / pixelRatio;
chart.width = Math.floor(chart.width * pixelRatio) / pixelRatio;

I couldn't figure out a reason to do that. Curious if someone can think of one.

@etimberg I agree with your concerns.
Based on what I found the PR that introduced this issue is #10646. It was a breaking change so reverting it might be a breaking change.

I'll keep digging.

@kurkle
Copy link
Member

kurkle commented Dec 16, 2022

I think we should skip a step, basically:

chart.height = Math.floor(maxSize.height);
chart.width = Math.floor(maxSize.width);
canvas.height = Math.floor(maxSize.height * pixelRatio);
canvas.width = Math.floor(maxSize.width * pixelRatio);

But my take on what really happens here is (edited):

  • We get a container size, we size chart to be a little bit smaller than the container.
  • Based on the new size, the aspect ratio changes a little.
  • At the next calculation, because of the aspect ratio change, the other of width / height is considered limiting and we end up with a pixel smaller chart. Rinse and repeat.

One fix could be to apply some rounding to the aspect ratio here (but does not feel correct):

return height ? width / height : null;

…s of pixelRatio the assignment would cause the size to reduce by 1px. Since it's called from the ResizeObserver it will be stuck in a loop that constantly reduce the size of the chart and canvas."

This reverts commit ed7a348.
@gbaron
Copy link
Contributor Author

gbaron commented Dec 17, 2022

@kurkle, After updating the implementation I noticed a different issue.
When the page loads the chart briefly shrinks.
See the video below, just after the "Reload" button is clicked.

Chart-Shrinks.mov

After some exploration, I noticed that getMaximumSize returns incorrect height value (399 instead of 400).
Return values are printed below the chart in the video.

I noticed

height = Math.max(0, aspectRatio ? Math.floor(width / aspectRatio) : height - margins.height);

Math.floor is the root cause IMO. It floors non-integers unnecessarily.
Two lines later the value is rounded.

height = round1(Math.min(height, maxHeight, containerSize.maxHeight));

The same example after removing Math.floor, doesn't briefly shrink and getMaximumSize return values look accurate.
Video after the change:

Removed-Math-Floor.mov

I did my best to update the code changes and add a test accordingly.

Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

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

@gbaron thank you for the good investigation and spent time!

src/helpers/helpers.dom.ts Show resolved Hide resolved
@gbaron
Copy link
Contributor Author

gbaron commented Dec 17, 2022

@kurkle Nice catch!
The code has been updated.

@kurkle kurkle linked an issue Dec 17, 2022 that may be closed by this pull request
@etimberg etimberg merged commit 9306d7f into chartjs:master Dec 17, 2022
@etimberg etimberg added this to the Version 4.1.1 milestone Dec 17, 2022
@gbaron
Copy link
Contributor Author

gbaron commented Dec 17, 2022

This one may also fix #10932, #10925 and #10890

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

All chart examples in documentation are decreasing in size when hover
3 participants