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

consider replacing offset{Height/Width} with client{Height/Width} for map canvas sizing #6848

Closed
mollymerp opened this issue Jun 21, 2018 · 5 comments
Labels
breaking change ⚠️ Requires a backwards-incompatible change to the API needs discussion 💬

Comments

@mollymerp
Copy link
Contributor

We currently use the map container's offsetHeight and offsetWidth to size the map canvas element, which leads to odd behavior when using a border, including Map#getCenter, Map#getBounds, etc. returning incorrect values for what is visible to the user because some of the map canvas will be hidden by the container.

mapbox-gl-js/src/ui/map.js

Lines 1474 to 1477 in bf88cd7

if (this._container) {
width = this._container.offsetWidth || 400;
height = this._container.offsetHeight || 300;
}

image

image

If we used clientHeight/clientWidth to size the canvas, we would eliminate this behavior, but I'm not sure what the original reasoning was behind the choice to use offset*

This would qualify as a breaking change. If we decide not to make this change, we could specify in the documentation that a map's container cannot have a border.

Expected Behavior

with client{Height/Width} sizing, canvas remains within the main content area of the container element

expected

Actual Behavior

with current behavior, canvas overflows the main content area and is partially hidden due to a border being used on the container element

actual

cc @mourner @jfirebaugh @kkaefer

@mollymerp mollymerp added needs discussion 💬 breaking change ⚠️ Requires a backwards-incompatible change to the API labels Jun 21, 2018
@mourner
Copy link
Member

mourner commented Jun 22, 2018

I think it's save to switch to clientWidth/Height. offsetWidth/Height was there pretty much since the first commit. I would guess that the original reason for the choice is that those two get mixed up all the time. :)

@jfirebaugh
Copy link
Contributor

Besides the canvas sizing code, is there any other code that would need to be adjusted, e.g. map center calculations, projection, event handling... ?

@ryanhamley
Copy link
Contributor

Popups are using the container's offsetWidth and offsetHeight here

@mollymerp
Copy link
Contributor Author

@ryanhamley I think those lines are referring to the Popup element's container, but we probably want to make the change there as well because users may want to use borders on their popups as well.

@jfirebaugh as far as I can tell the projection/transform/mouse event code all relies on the canvas container size

@raysuelzer
Copy link

I spent a few hours trying to figure out why the canvas was not respecting the style properties of the container and setting the height="480" for no apparent reason. IT would be ideal to use client height.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change ⚠️ Requires a backwards-incompatible change to the API needs discussion 💬
Projects
None yet
Development

No branches or pull requests

5 participants