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

Drop usage of backingStorePixelRatio API #9698

Merged
merged 1 commit into from Feb 21, 2020

Conversation

mattpap
Copy link
Contributor

@mattpap mattpap commented Feb 20, 2020

This is unlikely to improve anything with regards to issue #8478, but it's a good thing to do anyway.

addresses #8478

@mattpap mattpap added this to the 2.0 milestone Feb 20, 2020
this.bbox = new BBox({left: 0, top: 0, width, height})

this.el.style.width = `${width}px`
this.el.style.height = `${height}px`

const pixel_ratio = get_scale_ratio(this.ctx, this.model.use_hidpi, this.model.output_backend)
const {use_hidpi, output_backend} = this.model
const pixel_ratio = use_hidpi && output_backend != "svg" ? devicePixelRatio : 1
Copy link
Member

Choose a reason for hiding this comment

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

Where is the value of devicePixelRatio coming from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

window, or the global scope in general. Given that we don't rely on jsdom and nodejs for testing anymore, we can refer to all globals equally.

Copy link
Member

Choose a reason for hiding this comment

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

What specifically has changed that we can drop the old method of computing the pixel ratio?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

backingStorePixelRatio was never part of canvas specification, so it shouldn't have been used in the first place. Its usage is deprecated. As far as I can tell, if it was available at all (as a prefixed property), its value was usually 1, thus not contributing anything. Perhaps some older variants of some browsers (Safari?) messed with this setting to some extent.

@mattpap
Copy link
Contributor Author

mattpap commented Feb 20, 2020

The test failure is unrelated, so I won't be restarting CI.

@bryevdv
Copy link
Member

bryevdv commented Feb 20, 2020

I'll test locally on osx tonight then merge

@bryevdv
Copy link
Member

bryevdv commented Feb 21, 2020

Looks good on FF, Chrome, and Safari on OSX. Will test on windows with rc1

@bryevdv bryevdv merged commit a1f3d3c into master Feb 21, 2020
@bryevdv bryevdv deleted the mattpap/8478_backingStorePixelRatio branch February 21, 2020 02:36
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.

None yet

2 participants