Closed Bug 630614 Opened 9 years ago Closed 4 months ago

Show an indicator when geolocation is in use

Categories

(Firefox :: Site Identity, defect, P3)

defect

Tracking

()

RESOLVED FIXED
Firefox 70
Tracking Status
relnote-firefox --- 70+
firefox70 --- fixed

People

(Reporter: limi, Assigned: pbz)

References

(Depends on 1 open bug, Regressed 1 open bug)

Details

(Whiteboard: [fxprivacy] )

Attachments

(5 files)

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.
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.
blocking2.0: ? → -
We can get away without using any new strings (although these interfaces are always kind of a hack).  I'll attach a mockup.
This shows how we can potentially reuse strings to build the UI.
Note that this bug should also cover the "never share" state, where the request icon is shown, but the panel is not displayed.
Do we have different icon/indicators for "am currently sharing" and "am being asked to share"?
Not yet, creating those two icons is dependent bug 630769
blocking2.0: - → ?
Whiteboard: [strings]
Target Milestone: --- → Firefox 4.0b11
Argh, didn't mean to do that. Damn form autocomplete. :)
blocking2.0: ? → ---
Whiteboard: [strings]
Target Milestone: Firefox 4.0b11 → ---
Depends on: 641892
Updating bug summary to match proposed solution.
Component: General → Location Bar
QA Contact: general → location.bar
Summary: Geolocation indicator doesn't stick around → Create persistent indicator for geolocation sharing
Duplicate of this bug: 572972
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.
Assignee: nobody → mnoorenberghe+bmo
Status: NEW → ASSIGNED
Attachment #621196 - Flags: feedback?(margaret.leibovic)
Attached image 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)?
Attachment #621199 - Flags: ui-review?(ux-review)
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 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.
Attachment #621196 - Flags: feedback?(margaret.leibovic) → feedback+
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.
Attachment #621199 - Flags: ui-review?(ux-review) → ui-review+
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
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.
Assignee: MattN+bmo → nobody
Status: ASSIGNED → NEW
Component: Location Bar → Site Identity and Permission Panels
Summary: Create persistent indicator for geolocation sharing → Show an indicator when geolocation is in use
Whiteboard: p=0
Whiteboard: [fxprivacy] [triage]
We want to do this but not right now.
Priority: -- → P4
Whiteboard: [fxprivacy] [triage] → [fxprivacy]
Priority: P4 → P3

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.

Assignee: nobody → pbz
Status: NEW → ASSIGNED
Keywords: checkin-needed

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

Keywords: checkin-needed

I've resolved the conflict.

Keywords: checkin-needed

Pushed by btara@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3855d35999dd
Show an indicator when geolocation is in use. r=johannh

Keywords: checkin-needed

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

Flags: needinfo?(pbz)

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

Flags: needinfo?(pbz)
Keywords: checkin-needed

Pushed by rmaries@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d6b588f22b7e
Show an indicator when geolocation is in use. r=johannh

Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 70
Depends on: 1571243
No longer depends on: 1571243

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

relnote-firefox: --- → ?
Flags: needinfo?(pbz)
Regressions: 1580189

(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".

Flags: needinfo?(pbz)

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?

(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."

Regressions: 1577480

That works for me!

You need to log in before you can comment on or make changes to this bug.