-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
Round canvas.style dimensions to avoid blurring #9015
Conversation
src/helpers/helpers.dom.js
Outdated
@@ -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'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 thisdiv
while rounding the dimensions for thecanvas
. - 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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. |
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. |
Maybe it has to do with hidden canvas returning zeros for dimensions? Another thing that might need consideration is the |
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 // ...
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`;
} |
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. |
@simonbrunel would you like to review this as well? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@etimberg sorry, I missed the update. A few trivial comments (feel free to ignore if you want this PR merged promptly).
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.
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:
PS: With
npm test
, some image-based tests fail for me. However, this already happens withmaster
.