fix: avoid resize loop when browser zoom is set to 90%

[gbaron]

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?

[gbaron]

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.

[kurkle]

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]

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]

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]

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]

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):

Chart.js/src/core/core.controller.js

Line 223 in ae264e1

return height ? width / height : null;

[gbaron]

@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

Chart.js/src/helpers/helpers.dom.ts

Line 191 in ae264e1

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.

Chart.js/src/helpers/helpers.dom.ts

Line 193 in ae264e1

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.

[kurkle]

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

[gbaron]

@kurkle Nice catch!
The code has been updated.

[gbaron]

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