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

Show a disabled geolocate button if user or system denies geolocation #8871

Merged
merged 1 commit into from
Oct 16, 2019

Conversation

kkaefer
Copy link
Member

@kkaefer kkaefer commented Oct 15, 2019

Fixes #8870

image

This PR adds a new icon to be shown when the user or the system denied permission to use the location. It's shown when geolocation is disabled outright in Firefox and Chrome, or when the user or the system explicitly denies location after being prompted.

There are a few caveats:

  • It's not possible to detect whether the user has denied location to begin with when loading a page. Safari doesn't implement navigator.permissions detection. Once the user has click on the geolocate button, the state is set correctly though.
  • When macOS location services are disabled on a system-wide level, Firefox never calls any of the callbacks, and the location indicator keeps spinning. This is a bug in Firefox and tracked in https://bugzilla.mozilla.org/show_bug.cgi?id=1529591
  • When macOS location services are disabled, Chrome sneakily circumvents the OS setting and generates a location from different sources (e.g. IP address or Wifi networks; in my case, the location was approximate, but still in the same neighborhood).

Before this patch, the geolocate button was hidden when geolocation was disabled. It is now being shown, but in the new disabled state.

I'm a bit unsure how to test this correctly, since it depends on complex interaction between the OS, and the user clicking on various browser UIs.

/cc @andrewharvey

@kkaefer kkaefer added this to the release-sangria milestone Oct 15, 2019
@kkaefer kkaefer requested a review from a team October 15, 2019 12:51
Copy link
Contributor

@ryanhamley ryanhamley left a comment

Choose a reason for hiding this comment

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

This looks good to me. How did you test on Safari? I just get an error that geolocation is disabled over http.

src/ui/control/geolocate_control.js Outdated Show resolved Hide resolved
@ryanhamley
Copy link
Contributor

I'm a bit unsure how to test this correctly, since it depends on complex interaction between the OS, and the user clicking on various browser UIs.

Another argument for adding an end-to-end test suite

@andrewharvey
Copy link
Collaborator

Could we update the title/aria-label when disabled to be something like "Location not available"?

@andrewharvey
Copy link
Collaborator

This is good!

We should make sure to include a note in the changelog since the behaviour has changed. Anyone who prefers to hide the control when not available can still override the css .mapboxgl-ctrl-geolocate:disabled { display: none }.

Copy link
Collaborator

@andrewharvey andrewharvey left a comment

Choose a reason for hiding this comment

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

Could we update the title/aria-label when disabled to be something like "Location not available"?

@kkaefer
Copy link
Member Author

kkaefer commented Oct 16, 2019

I tested in Safari in the following way:

brew install mkcert
mkcert -install
mkcert localhost

python -c "import BaseHTTPServer, SimpleHTTPServer, ssl; \
           handler = SimpleHTTPServer.SimpleHTTPRequestHandler; \
           httpd = BaseHTTPServer.HTTPServer(('localhost', 4443), handler); \
           httpd.socket = ssl.wrap_socket(httpd.socket, \
                                          certfile='./localhost.pem', \
                                          keyfile='./localhost-key.pem', \
                                          server_side=True); \
           httpd.serve_forever()"

Then accessed the server at https://localhost:4443/debug/debug.html

@kkaefer kkaefer force-pushed the geolocate-disabled branch from f6f5941 to 625cbd2 Compare October 16, 2019 08:54
Copy link
Member Author

@kkaefer kkaefer left a comment

Choose a reason for hiding this comment

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

Added the title and aria-label, and addressed all review feedback.

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.

geolocation error handling not working
3 participants