630614 - Show an indicator when geolocation is in use

Status: RESOLVED FIXED

(Firefox :: Site Identity, defect, P3)

Description

[Alex Limi (:limi) — Firefox UX Team]

After giving a site permission to get your geolocation, the icon that indicates that you did so doesn't stick around — so it's hard to tell that the site has the permission, and hard to remove that permission later.

Nominating as blocker (requested by Beltzner).

Steps to reproduce:

1. http://maps.google.com
2. Click the "Show my location" dot in the map controls,
3. Answer "Share location" when asked,
4. Geolocation indicator disappears.

Expected result:

4. Geolocation stays around for as long as the site has this permission.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

What should the notification popup look like after the decision has been made? When should it finally disappear? Implementing this would require some new strings, so I don't think it's feasible to block on it at this point.

Comment 2

[Alex Faaborg [:faaborg] (Firefox UX)]

We can get away without using any new strings (although these interfaces are always kind of a hack).  I'll attach a mockup.

Comment 3

[Alex Faaborg [:faaborg] (Firefox UX)]

Attachment: UI for always and never share location

This shows how we can potentially reuse strings to build the UI.

Comment 4

[Alex Faaborg [:faaborg] (Firefox UX)]

Note that this bug should also cover the "never share" state, where the request icon is shown, but the panel is not displayed.

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

Do we have different icon/indicators for "am currently sharing" and "am being asked to share"?

Comment 6

[Alex Faaborg [:faaborg] (Firefox UX)]

Not yet, creating those two icons is dependent bug 630769

Comment 7

[Alex Limi (:limi) — Firefox UX Team]

Argh, didn't mean to do that. Damn form autocomplete. :)

Comment 8

[:Margaret Leibovic]

Updating bug summary to match proposed solution.

Comment 10

[Matthew N. [:MattN]]

Attachment: v.1 WIP for the always allow case

Adds a dismissed notification when geolocation is accessed and it is set to always allow for the site.  There was a mention of having it not be dismissed for the allow case but that seems like it may be too annoying without bug 596723 fixed.  It's easy to change though if that's still what is wanted.  Since we aren't required to re-use strings at this point, I went with the normal doorhanger layout with a menu-button rather than the checkbox.

The disallow case still needs to be implemented.  I'll wait for feedback on the approach before doing that since it's mostly the same.  That will also require an inactive geolocation icon (bug 630769) or applying a different style to the existing icon in that case.

Tests are also needed.

Comment 11

[Matthew N. [:MattN]]

Attachment: Screenshot of v.1 WIP

Now that we don't have a string freeze, what is the desired UI/strings for this notification (and the always disallow for this site case)?

Comment 12

[Matthew N. [:MattN]]

Attachment: Screenshot of v.1 WIP in private browsing mode

In private browsing mode we don't show the Always/Never allow options so it seems like we shouldn't allow revoking them either.  Does it make sense to just let the user know their location is shared?  Alternatively, we could just show the icon and not provide a message at all (using the "neverShow" option). 

Note that the notifications are dismissed by default in the patch but I'm showing them open for review.

Comment 13

[:Margaret Leibovic]

Comment on attachment 621196 [details] [diff] [review]
v.1 WIP for the always allow case

Pending UX input to make the strings sound better, this looks like a good start to me. I'm not sure what our customs are on using var/let in old files that already use var everywhere, but I'd think we'd prefer to just use let.

>diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js
>@@ -1606,39 +1611,85 @@ ContentPermissionPrompt.prototype = {

>+      let that = this;
>       if (!inPrivateBrowsing) {
>         secondaryActions.push({
>           label: browserBundle.GetStringFromName("geolocation.alwaysShareLocation"),
>           accessKey: browserBundle.GetStringFromName("geolocation.alwaysShareLocation.accesskey"),
>           callback: function () {
>             Services.perms.add(requestingURI, "geo", Ci.nsIPermissionManager.ALLOW_ACTION);
>             request.allow();
>+            that._alreadyAllowedPrompt(requestingURI, chromeWin, browser);

You could use .bind(this) instead of making a "that" variable.

>diff --git a/browser/locales/en-US/chrome/browser/browser.properties b/browser/locales/en-US/chrome/browser/browser.properties

>+geolocation.alwaysSharingWithSitePB=You are always sharing your location with %S.
>+geolocation.alwaysSharingWithFilePB=You are always sharing your location with the file %S.

Add a localization note about how these are used in private browsing mode, and that's why we can't offer to change the permission.

Comment 14

[Alex Limi (:limi) — Firefox UX Team]

Comment on attachment 621199 [details]
Screenshot of v.1 WIP

I think “Stop sharing location” is better as a button string, the “always” is implied, as well as clarified in the dialog body text. 

WFM with those changes.

Comment 15

[Matthew N. [:MattN]]

I still think we should do this as every other platform (desktop and mobile) has this already but I'm not actively working on it.

Comment 16

[Erin Lancaster [:elan]]

We want to do this but not right now.

Comment 17

[Paul Zühlcke [:pbz]]

This could be implemented with an additional prompt type similar to PermissionUI post prompts. If nobody else is working on this I'll give it a try.

Comment 18

[Paul Zühlcke [:pbz]]

Attachment: Bug 630614 - Show an indicator when geolocation is in use. r=johannh

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

There is a conflict applying the change for browser/base/content/test/siteIdentity/browser.ini, please fix it.

Comment 20

[Paul Zühlcke [:pbz]]

I've resolved the conflict.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3855d35999dd
Show an indicator when geolocation is in use. r=johannh

Comment 22

[Natalia Csoregi [:nataliaCs]]

Backed out for failing on browser_ext_tabs_sharingState.js

backout: https://hg.mozilla.org/integration/autoland/rev/13578a2de8a31a59d2eb0140b9107f528709e789

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=3855d35999ddb5fa13878829fb52a37caaaa0f89&group_state=expanded

Started failing tier 1 on a subsequent push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=9c2e6c4a65ec4f458e71cfe24b85b82b433654a8&selectedJob=258757009

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=258755023&repo=autoland&lineNumber=1173

[task 2019-07-29T10:19:32.302Z] 10:19:32 INFO - Console message: [JavaScript Error: "TypeError: state is undefined" {file: "moz-extension://2e9a2060-6358-8a42-b59f-91605138444b/%7Bdd85aeb4-fc3a-d048-b007-56d6dd6f5b32%7D.js" line: 6}]
[task 2019-07-29T10:19:32.303Z] 10:19:32 INFO - Buffered messages finished
[task 2019-07-29T10:19:32.304Z] 10:19:32 INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_tabs_sharingState.js | Test timed out -
[task 2019-07-29T10:19:32.308Z] 10:19:32 INFO - Not taking screenshot here: see the one that was previously logged
[task 2019-07-29T10:19:32.310Z] 10:19:32 INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_tabs_sharingState.js | no tasks awaiting on messages - Got ["ready"], expected []
[task 2019-07-29T10:19:32.310Z] 10:19:32 INFO - Stack trace:
[task 2019-07-29T10:19:32.310Z] 10:19:32 INFO - chrome://mochikit/content/browser-test.js:test_is:1591
[task 2019-07-29T10:19:32.310Z] 10:19:32 INFO - chrome://mochikit/content/tests/SimpleTest/ExtensionTestUtils.js:ExtensionTestUtils.loadExtension/<:31
[task 2019-07-29T10:19:32.310Z] 10:19:32 INFO - chrome://mochikit/content/browser-test.js:nextTest:856
[task 2019-07-29T10:19:32.311Z] 10:19:32 INFO - chrome://mochikit/content/browser-test.js:timeoutFn:1467
[task 2019-07-29T10:19:32.311Z] 10:19:32 INFO - setTimeout handler*chrome://mochikit/content/browser-test.js:Tester_execTest:1414
[task 2019-07-29T10:19:32.311Z] 10:19:32 INFO - chrome://mochikit/content/browser-test.js:nextTest/<:1203
[task 2019-07-29T10:19:32.311Z] 10:19:32 INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:803
[task 2019-07-29T10:19:32.311Z] 10:19:32 INFO - Not taking screenshot here: see the one that was previously logged
[task 2019-07-29T10:19:32.311Z] 10:19:32 INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_tabs_sharingState.js | Extension left running at test shutdown -

Other failure: TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_ext_geolocation.html | geolocation call [object PositionError] https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=258754984&repo=autoland&lineNumber=24688

Comment 23

[Paul Zühlcke [:pbz]]

Updated two tests and added a null check for browser.ownerGlobal.gBrowser.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d6b588f22b7e
Show an indicator when geolocation is in use. r=johannh

Comment 25

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/d6b588f22b7e

Comment 26

[Liz Henry (:lizzard) (relman/hg->git project)]

This might be worth mentioning in release notes. Can you suggest wording for it?

Comment 27

[Paul Zühlcke [:pbz]]

(In reply to Liz Henry (:lizzard) from comment #26)

This might be worth mentioning in release notes. Can you suggest wording for it?

I'd suggest something like "We now show an indicator when a website requests geo location".

Comment 28

[Jeff Pfaller]

I would recommend "When you've allowed a website to use your geolocation, an icon is shown in the address bar."

Is that correct? It's not shown when there is a request, it's shown when you've approved it?

Comment 29

[Paul Zühlcke [:pbz]]

(In reply to Jeff Pfaller from comment #28)

I would recommend "When you've allowed a website to use your geolocation, an icon is shown in the address bar."

Is that correct? It's not shown when there is a request, it's shown when you've approved it?

Good point, I meant request as in "access the api", but that doesn't seem suitable.
How about: " When a website uses your geolocation, an indicator is shown in the address bar."

Comment 30

[Jeff Pfaller]

That works for me!