-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Fix canvas size to round up to container size #10936
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
Conversation
…eturn exact pixels instead of rounding
Makes sense. But hmm… the unit tests seem suspiciously far off for a subpixel difference in canvas size. (e.g. 584 --> 72 in the example belolw). I wonder if something works differently when the tests are run in node.
I'm also not quite 100% clear on what the original example does. Is the author setting the map to the full page size? Is it possible maybe the burden of scaling the canvas to fill the full page without a gap may be on the developer and not the library? |
@rreusser Thanks for taking a look! The PR is still in progress.. When I use the debug page getBoundingClientRect works but does not work in the tests so it is using the default values instead. I'm currently looking into why this is. |
Ah, now I see the draft status. Sorry to jump the gun. Carry on 😄👍 |
…or for round down to match previous unit tests before switching to roundup
@rreusser Thanks for checking it out though! That is a good thought to discuss though on if it is something we should be handling. I can only "see" this bug when the canvas isn't full-screen (I'm not sure what the specific use-cases are for someone having a canvas not full-screen). I fixed the unit tests (was missing a mock for getBoundingClient Rect). I discussed with @arindam1993 keeping the canvas.style.width and canvas.style.height as fractional values, but rounding up the canvas size. |
Looks good to me @avpeery ! Nice job. Since this changes a bit of behaviour in the interaction between the browser and GL-JS, it would be helpful to be able to test this manually. Especially eventually during when this lands in a future release. Happy to show you how to add a manual testing page. |
…to original state
Thanks @arindam1993 for checking out the PR - I added in a query test and manual test. Let me know what you think! Thanks again! |
"version": 8, | ||
"metadata": { | ||
"test": { | ||
"height": 128, |
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 think this needs to be a fractional value to exercise the new codepath?
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.
Looks good to me and the render test output is perfect, it demonstrates that the pixel grid snapping is working along with the rounding up of the image size!
Nice work!
Closes #9455.
Previous bug demonstrated that canvas pixel size would round down from container size, leaving a small gap between the difference of the two sizes (detectable on some retina and mobile displays as per the issue).
clientWidth and clientHeight were previously being used to calculate the size for the canvas which as per the documentation always rounds the value to an integer. Replacing these with getBoundingClientRect() allows decimal values.
EDITED LATER TO ADD: All references to clientWidth and clientHeight have been replaced with getBoundingClientRect in this PR. getBoundingClientRect provides a more exact value which will improve accuracy for calculations.
Launch Checklist
mapbox-gl-js
changelog:<changelog> Rounds canvas size up to container size </changelog>