take Geolocation accuracy into account with the Geolocate control

[andrewharvey]

Launch Checklist

This is still a WIP but I wanted to get some feedback on the approach I've used so far before gonig too far down the wrong path.

I still need to ensure I cover corner cases, consider if fit bounds should behave like jumpto or flyto, write tests.

I've used turf libraries for abstraction, using existing robust tested and documented code rather than copy pasting the turf.destination code, but I'm not sure about this since there are no other uses of turf in mapbox-gl-js as far as I can see.

• briefly describe the changes in this PR

The Geolocate Control uses the Geolocation API, which can include accuracy information. Currently the Geolocate Control ignores the accuracy but I think it should take it into account.

This PR takes the Geolocation accuracy into account so when the accuracy is large (large uncertain location) the map will use a lower zoom and when the accuracy is small (small fairly centain location) the map will use a higher zoom.

• write tests for all new functionality
• document any changes to public APIs
• post benchmark scores
• manually test the debug page

[lucaswoj]

My understanding of this new functionality is that we set the map's bounds to fit a circle with the radius of the GPS's accuracy. Is that correct?

Is this the behaviour we want in the case where the accuracy is 1 meter? Would it be possible to get similar behaviour without adding Turf as a dependency?

[mourner]

You could borrow the logic from Leaflet to get rid of the Turf dependencies: https://github.com/Leaflet/Leaflet/blob/master/src/geo/LatLng.js#L79-L86

[andrewharvey]

My understanding of this new functionality is that we set the map's bounds to fit a circle with the radius of the GPS's accuracy. Is that correct?

Exactly. getCurrentPosition can return an accuracy radius. The motivation for this PR is often this location isn't coming from a GPS device so the location might only be city level, so the map should be zoomed out to cover the city, or whatever the accuracy is.

Is this the behaviour we want in the case where the accuracy is 1 meter?

I think the logic makes sense when the accuracy is say 1km, but not for 1m. I'm thinking of just setting zoom 18 as the upper limit. How does this sound?

You could borrow the logic from Leaflet to get rid of the Turf dependencies: https://github.com/Leaflet/Leaflet/blob/master/src/geo/LatLng.js#L79-L86

Perfect.

[andrewharvey]

Using the code from Leaflet is a lot simpler, rather than adding it as a util function, I think it makes sense to add it as a method on LngLat, however I'm running into an issue with this. I think it's because "Static methods are ... not callable when the class is instantiated.". I'm stuck here, so any guidance would be much appreciated.

Then I'll just need to clean up the commented code/XXX bits/verify tests/add more tests/squash commits.

[lucaswoj]

running into an issue with this

What's the issue? Are you referring to the tests failing with Error: TypeError: LngLat.convert is not a function?

[mourner]

This might be caused by the circular dependency between LngLat and LngLatBounds, although it's not immediately apparent why.

[andrewharvey]

What's the issue? Are you referring to the tests failing with Error: TypeError: LngLat.convert is not a function?

Yes exactly.

This might be caused by the circular dependency between LngLat and LngLatBounds, although it's not immediately apparent why.

I thought so too, but I'm not sure the best way forward.

[andrewharvey]

I thought so too, but I'm not sure the best way forward.

Having LngLat.toBounds() return a LngLatBoundsLike rather than a LngLatbounds works around this issue as per my recent commit.

I've added a debug page which I put together while testing which uses turf to draw a circle with a radius from a point, and compares this with the bounds obtained with LngLat.toBounds(). However eslint doesn't like including turf from the CDN. However I think for a simple debug page it's much simpler than worrying about building a bundle.

[andrewharvey]

Updated Launch Checklist

• resolve eslint error with turf cdn include in debug page
• Unify the differences between jumpTo and fitBounds. One animates the other doesn't. Setting the camera should look the same regardless if accuracy is present or not.
• rebase to take into account 2d10449 and squash commits

Future PRs:

• expose camera options within the Geolocate Control options, perhaps wait for Unify camera API into a single method #2801 ?
• Geolocate Control option to add a dot for the user's location -> add positionMarker option to Geolocate Control and improve watchPosition with an active lock and background mode #3781

[lucaswoj]

Unify the differences between jumpTo and fitBounds. One animates the other doesn't. Setting the camera should look the same regardless if accuracy is present or not.

This is all going to become much clearer with #2801 😄

[andrewharvey]

This is all going to become much clearer with #2801 😄

Agreed, but should this PR wait for that to land, or should we just accept the difference (or change the currentjumpTo to flyTo to match fitBounds and then when #2801 lands follow up with an additional PR which picks a default mode and then just "expose camera options within the Geolocate Control options" to let users customise the camera changes."?

[lucaswoj]

I'd feel comfortable with either of these options for now

• accept the difference
• change the current jumpTo to flyTo to match fitBounds

and planning to follow up when #2801 lands

[andrewharvey]

1. I've updated the branch to base off master and squashed commits so far, and added some more.

2. Doing more research according to the spec accuracy "must be supported by all implementations. The value of the accuracy attribute must be a non-negative real number."
   With that in mind I don't see any reason to support the two code paths for where accuracy is provided and where it isn't, since it should always be. I've tested this on Chrome Desktop & Mobile, Firefox Desktop & Mobile, Safari on Mobile.

3. I've added an option to pass in your own fitBounds options which lets you set your own maxZoom (defaults to 17 as anything beyond this you basically have no context as the map is too zoomed in when the accuracy is down to a few meters). When Unify camera API into a single method #2801 comes this would change, but is fine for now I think.

So I believe we just need to:

• resolve eslint error with turf cdn include in debug page

I could just scrap that debug page, but I did find it useful for testing fitBounds, so not sure if we should leave it in or not.

[mourner]

This is looking good!

Before we merge, I'd love to figure out the cause of the circular dependency issue. 1) we used circular deps in other places and it mostly worked OK, 2) having toBounds return an array is inconsistent with other API methods.

Other option would be to move the bounds conversion to the geolocation control code instead of adding it to public API.

[andrewharvey]

Before we merge, I'd love to figure out the cause of the circular dependency issue. 1) we used circular deps in other places and it mostly worked OK, 2) having toBounds return an array is inconsistent with other API methods.

I agree. I did some research but couldn't work out a solution. 😞

Other option would be to move the bounds conversion to the geolocation control code instead of adding it to public API.

It would be nice to have it in the public API, but I'm not fussed, especially since I can't get ^ to work. If you think I should go ahead and change to this, let me know and I'll implement it.

[andrewharvey]

Before we merge, I'd love to figure out the cause of the circular dependency issue. 1) we used circular deps in other places and it mostly worked OK, 2) having toBounds return an array is inconsistent with other API methods.

Other option would be to move the bounds conversion to the geolocation control code instead of adding it to public API.

I believe I've managed to fix this by only requireing LngLatBounds within the toBounds function rather than at the top of the file. 🎉

I've scrapped the debug page I made since I couldn't resolve the eslint error and it's not critical now that I've done further testing.

With that I believe I've address everything from my end. Should I squash and rebase this now?

[lucaswoj]

Should I squash and rebase this now?

Go for it! This should fix the failing CI too.

[andrewharvey]

Go for it! This should fix the failing CI too.

Done

[andrewharvey]

I'd like to take advantage of the new testing framework for the Geolocate Control in #3781, but I don't want to end up duplicating a lot of the codebase which would get nulled by a rebase. Do you think I should just close this PR and include the changes directly within #3781, or is it better to keep them separate and follow up with tests after?

[mollymerp]

@andrewharvey Its up to you whether you'd like to combine the PRs or keep them separate. If you want to copy over some of the testing code you wrote for #3781 to add additional tests here that's fine. It will all get sorted out on rebase+merge so I'm not too concerned about that.

[andrewharvey]

Thanks @mollymerp. I've combined this with #3781. I didn't want #3781 to hold this up but it's easier to merge it across then duplicate all the new test code back here, and it's not wise to merge this without tests.