Round canvas.style dimensions to avoid blurring #9015

Flupp · 2021-05-02T16:29:27Z

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.

With the following example page, depending on the size of the browser window, you get the following blurry result:

With this fix, the result looks as follows:

Example page:

<!doctype html>
<html lang="en">
<head>
	<meta charset="utf-8"/>
	<meta name="viewport" content="width=device-width, initial-scale=1">
	<meta name="referrer" content="no-referrer">
	<title>Chart.js Blur Test</title>
	<script src="https://cdn.jsdelivr.net/npm/chart.js@3.2.1/dist/chart.min.js"></script>
	<!-- <script src="dependencies/Chart.js/dist/chart.js"></script> -->
	<style>
		.chart-container {
			position: relative;
			width: 75vw;
			height: 75vh;
		}
	</style>
</head>
<body>
	<div class="chart-container">
		<canvas id="myChart" width="400" height="400"></canvas>
	</div>
    <table border="1">
        <tr><th></th><th>width</th><th>height</th></tr>
        <tr><td>canvas</td><td id="canvasWidth"></td><td id="canvasHeight"></td></tr>
        <tr><td>style</td><td id="styleWidth"></td><td id="styleHeight"></td></tr>
    </table>
    <script>
        Chart.register({
            id: 'info',
            afterUpdate: function(chart, options) {
                document.getElementById('canvasWidth').textContent = chart.canvas.width;
                document.getElementById('canvasHeight').textContent = chart.canvas.height;
                document.getElementById('styleWidth').textContent = chart.canvas.style.width;
                document.getElementById('styleHeight').textContent = chart.canvas.style.height;
            },
        });

        var data = [];
        for (let x = 0; x <= 20; ++x) {
            for (let y = 0; y <= 20; ++y) {
                data.push({x: x, y: y});
            }
        }

        var ctx = document.getElementById('myChart').getContext('2d');
        var myChart = new Chart(ctx, {
            type: 'scatter',
            data: {
                datasets: [{
                    label: "some random data",
                    data: data,
                }]
            },
            options: {
                animation: false,
                maintainAspectRatio: false
            }
        });
    </script>
</body>
</html>

PS: With npm test, some image-based tests fail for me. However, this already happens with master.

etimberg · 2021-05-02T16:37:33Z

src/helpers/helpers.dom.js

@@ -162,8 +162,8 @@ export function retinaScale(chart, forceRatio, forceStyle) {
  // making the chart visually bigger, so let's enforce it to the "correct" values.
  // See https://github.com/chartjs/Chart.js/issues/3575
  if (canvas.style && (forceStyle || (!canvas.style.height && !canvas.style.width))) {
-    canvas.style.height = height + 'px';
-    canvas.style.width = width + 'px';
+    canvas.style.height = (canvas.height / pixelRatio) + 'px';


I don't think this is the correct fix as it will break retina screens. In the case of a retina display, pixelRatio will be a number greater than 1. If we need to round, it feels like it should be Math.floor(canvas.height) but I suspect there will be cases where this introduces oscillation during resizing. We saw that a lot in previous implementations of the chart resizing because when the height is reduced from say 300.8 to 300, the need for a scrollbar may disappear triggering another resize which increases the width slightly triggering the scrollbar to re-appear which changes the width leading to an infinite loop.

When canvas.height is assigned, the value is already implicitly rounded. So no additional flooring should be required.

Regarding potential oscillations: I understand your concerns, however, I do not see a good solution. The following naive ideas come to my mind:

Wrap the canvas in yet another div and apply the unrounded dimensions to this div while rounding the dimensions for the canvas.

Add the discrepancy between unrounded and rounded values as padding to the canvas.

Trade the blurring problem for the potential oscillation problem while assuming the blurring does occur far more often than the oscillation would occur.

Interesting. So the width/height are multiplied with pixel ratio and implicitly converted to integers. Using that integer value and dividing it with the ratio should actually result in more accurate result even for retina displays.

Not quite sure if this case has been tested previously.

Ah, ok that makes more sense. I don't know how we'd get test coverage on this, but the explanation makes sense. If we could adjust to add a comment here explaining why we're doing this, I'm ok with the change

kurkle · 2021-05-02T19:32:12Z

DPR: 1.25

master:

263 X 262 / 210.4px X 210px

pr:

263 X 262 / 210.4px x 209.6px

not really noticing any difference.

kurkle · 2021-05-02T19:56:10Z

master

pr

kurkle · 2021-05-02T20:06:50Z

Not sure if #7351 can be closed with this. And not sure if this should be marked as breaking, since it could affect layouts and resizing in some edge cases.

simonbrunel · 2021-05-03T07:12:04Z

I seem to remember that deriving the style directly from the canvas values causes some other issues. But that was looong time ago, I can't remember why and the suggested fix seems to make sense indeed because the style and canvas dimensions should be proportional, which is currently not necessarily the case.

kurkle · 2021-05-03T09:45:24Z

I seem to remember that deriving the style directly from the canvas values causes some other issues.

Maybe it has to do with hidden canvas returning zeros for dimensions?

Another thing that might need consideration is the width and height of the chart itself that would be slightly different in these cases. Maybe those should also be updated or the fix applied to the chart size computation already?

simonbrunel · 2021-05-03T19:05:43Z

Maybe it has to do with hidden canvas returning zeros for dimensions?

Could be the reason and you are right, that may not work correctly if the canvas is hidden. That's why we should pre-compute the truncated dimensions. Good point on the chart size, maybe retinaScale should also update the chart size:

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

chart.height = height / pixelRatio;
chart.width = width / pixelRatio;
canvas.height = height;
canvas.width = width;

// ...

if (canvas.style && (forceStyle || (!height && !width))) {
  canvas.style.height = `${chart.height}px`;
  canvas.style.width= `${chart.width}px`;
}

Flupp · 2021-05-05T20:30:38Z

I updated the PR following @simonbrunel’s suggestions. Additionally, I changed the detection of the need for redrawing so redraw is only requested if the framebuffer parameters change.

etimberg · 2021-05-07T19:55:22Z

@simonbrunel would you like to review this as well?

simonbrunel

@etimberg sorry, I missed the update. A few trivial comments (feel free to ignore if you want this PR merged promptly).

src/helpers/helpers.dom.js

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. Acknowledging that canvas.height and canvas.width are integers, the framebuffer is now only redrawn if those integer values change.

etimberg reviewed May 2, 2021

View reviewed changes

kurkle requested a review from simonbrunel May 3, 2021 04:31

Flupp force-pushed the blur-fix branch from 00d3c52 to 7ed4c11 Compare May 5, 2021 20:30

Flupp force-pushed the blur-fix branch from 7ed4c11 to 0bcc788 Compare May 5, 2021 21:19

etimberg previously approved these changes May 5, 2021

View reviewed changes

kurkle previously approved these changes May 6, 2021

View reviewed changes

simonbrunel reviewed May 7, 2021

View reviewed changes

src/helpers/helpers.dom.js Outdated Show resolved Hide resolved

src/helpers/helpers.dom.js Outdated Show resolved Hide resolved

src/helpers/helpers.dom.js Outdated Show resolved Hide resolved

Flupp dismissed stale reviews from kurkle and etimberg via 6f6b515 May 7, 2021 21:33

Flupp force-pushed the blur-fix branch from 0bcc788 to 6f6b515 Compare May 7, 2021 21:33

etimberg approved these changes May 7, 2021

View reviewed changes

simonbrunel approved these changes May 7, 2021

View reviewed changes

kurkle approved these changes May 8, 2021

View reviewed changes

kurkle merged commit c955ffa into chartjs:master May 8, 2021

kurkle added the type: bug label May 8, 2021

kurkle changed the title ~~round canvas.style dimensions to avoid blurring~~ Round canvas.style dimensions to avoid blurring May 8, 2021

kurkle linked an issue May 8, 2021 that may be closed by this pull request

Blurry chart lines #7351

Closed

kurkle linked an issue May 19, 2021 that may be closed by this pull request

Chart is blurry when put inside an element with "weird" width #9130

Closed

kurkle mentioned this pull request May 26, 2021

Blury display #9161

Closed

kurkle mentioned this pull request Dec 15, 2022

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Round canvas.style dimensions to avoid blurring #9015

Round canvas.style dimensions to avoid blurring #9015

Flupp commented May 2, 2021

etimberg May 2, 2021

Flupp May 2, 2021

kurkle May 2, 2021

etimberg May 2, 2021

kurkle commented May 2, 2021 •

edited

kurkle commented May 2, 2021

kurkle commented May 2, 2021

simonbrunel commented May 3, 2021

kurkle commented May 3, 2021

simonbrunel commented May 3, 2021

Flupp commented May 5, 2021

etimberg commented May 7, 2021

simonbrunel left a comment

Round canvas.style dimensions to avoid blurring #9015

Round canvas.style dimensions to avoid blurring #9015

Conversation

Flupp commented May 2, 2021

etimberg May 2, 2021

Choose a reason for hiding this comment

Flupp May 2, 2021

Choose a reason for hiding this comment

kurkle May 2, 2021

Choose a reason for hiding this comment

etimberg May 2, 2021

Choose a reason for hiding this comment

kurkle commented May 2, 2021 • edited

kurkle commented May 2, 2021

kurkle commented May 2, 2021

simonbrunel commented May 3, 2021

kurkle commented May 3, 2021

simonbrunel commented May 3, 2021

Flupp commented May 5, 2021

etimberg commented May 7, 2021

simonbrunel left a comment

Choose a reason for hiding this comment

kurkle commented May 2, 2021 •

edited