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: do not cache devicePixelRatio #3844

Merged
merged 14 commits into from Dec 19, 2023
Merged

fix: do not cache devicePixelRatio #3844

merged 14 commits into from Dec 19, 2023

Conversation

lsh
Copy link
Member

@lsh lsh commented Dec 13, 2023

Currently we cache the devicePixelRatio on the creation of the renderer and never check to update it again. The problem with this behavior is that zooming in or out of a page will change the devicePixelRatio (which can be observed via window.onresize). In turn, a render will be blurry on zoom. This PR makes us recalculate the devicePixelRatio on resize, which is negligibly expensive.

@lsh lsh requested a review from a team as a code owner December 13, 2023 18:22
Copy link
Member

@domoritz domoritz 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. Is there a unit test we could write for this?

@domoritz
Copy link
Member

From Slack @lsh

The caveat is that a user still needs to trigger a re-render whenever a window.onresize event is fired, but we currently don't listen for that event as far as I know

Should we add a listener to window.onresize?

@lsh
Copy link
Member Author

lsh commented Dec 13, 2023

I think that would be valuable, but do we have to worry about how that interacts with react?

@domoritz
Copy link
Member

Are you concerned about the global variable or which part? Vega already has a bunch of globals so I don't see any additional concerns. I'd focus on what's best for Vega for now and then worry about compatibility with React.

@tuner
Copy link
Contributor

tuner commented Dec 14, 2023

You could also listen to devicePixelRatio changes using media queries: https://developer.mozilla.org/en-US/docs/Web/API/Window/devicePixelRatio#monitoring_screen_resolution_or_zoom_level_changes

I think this solution would also cover cases where the browser window is dragged from a low-dpi display to a hi-dpi display.

@lsh lsh requested a review from domoritz December 18, 2023 20:34
Copy link
Member

@domoritz domoritz left a comment

Choose a reason for hiding this comment

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

Needs docs and ts typings but otherwise looks good.

packages/vega-view/src/View.js Outdated Show resolved Hide resolved
@lsh lsh merged commit ae698ee into main Dec 19, 2023
4 checks passed
@lsh lsh deleted the lsh/device-pixel-ratio-fix branch December 19, 2023 06:09
@lsh lsh mentioned this pull request Dec 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants