anchor Markers at their center

[andrewharvey]

Closes #2900 (breaking change!)

Not sure if this is the best approach (tweaking transform translate values) or a CSS approach is best.

Currently if you change the HTML Element's size, it will loose centering until the map camera changes and triggers Marker._update.

Launch Checklist

• briefly describe the changes in this PR
  anchor Markers at their center by default per Make markers anchored at their center by default #2900
• write tests for all new functionality

 - [x] document any changes to public APIs
 - [ ] post benchmark scores
Need to check this extra calculation at each camera change doesn't add much overhead.
 - [x] manually test the debug page
looks okay (including when the element has a border).

[jfirebaugh]

Seems good to me. We could switch to a CSS approach later (including adding a anchor option like Popup) if it's needed.

[mollymerp]

Thanks @andrewharvey ! Can you also round the offsets to the nearest pixel? We had issues w subpixel transforms causing Popups to render blurry on some screens.

[andrewharvey]

Can you also round the offsets to the nearest pixel? We had issues w subpixel transforms causing Popups to render blurry on some screens.

Actually they are already rounded at

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

Line 166 in a266c43

// because rounding the coordinates at every `move` event causes stuttered zooming

I've pulled out this test to get it passing, for some reason in the testing infrastructure the DOM element.offsetWidth isn't working so the test fails.

t.test('marker centered by default', (t) => {
    const map = createMap();
    const element = window.document.createElement('div');
    element.style.width = '10px';
    element.style.height = '10px';
    element.style.border = '6px solid black';

    const marker = new Marker(element).setLngLat([0, 0]).addTo(map);
    const translate = Math.round((map.getContainer().offsetWidth / 2) - ((6 + 10 + 6) / 2));
    t.equal(marker.getElement().style.transform, `translate(${translate}px, ${translate}px)`, 'Marker centered');
    t.end();
});

I've manually tested and it works well even when the Marker has a padding or border.

[mollymerp]

@andrewharvey I believe the failing test is due to the changes from upgrading JSDOM (16c75b8). You'll have to use Object.defineProperty as in https://github.com/mapbox/mapbox-gl-js/blob/master/test/unit/ui/control/attribution.test.js#L45

[andrewharvey]

@andrewharvey I believe the failing test is due to the changes from upgrading JSDOM (16c75b8). You'll have to use Object.defineProperty as in https://github.com/mapbox/mapbox-gl-js/blob/master/test/unit/ui/control/attribution.test.js#L45

Thanks for the pointer @mollymerp. I've updated the unit test now, though it is far away from a real world browser but it's better than nothing I suppose.

[mollymerp]

Thanks, @andrewharvey!

[mollymerp]

Ooo one thing I missed @andrewharvey – I think we should pass the offset to the popup when its bound to a Marker with setPopup, but allow it to be overridden if the user specifies different offsets.

[andrewharvey]

Ooo one thing I missed @andrewharvey – I think we should pass the offset to the popup when its bound to a Marker with setPopup, but allow it to be overridden if the user specifies different offsets.

I think I have this working now, but I'd like to test it more and I'll need to look into a unit test.

[mollymerp]

Ooh sorry for not being more clear – I meant offset the popup by the same additional amount as the marker {x: -offsetWidth / 2, y: -offsetHeight / 2} unless they specify alternate offsets for the Popup, so that its also anchored at the Marker's center