1069882 - Ability to override the auto-detected geolocation with a custom one

Status: NEW

(DevTools :: Responsive Design Mode, enhancement, P3)

Description

[Lucas Verney [:Phyks]]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:32.0) Gecko/20100101 Firefox/32.0
Build ID: 20140901122935

Steps to reproduce:

Hi,

Chromium is offering some advanced features in its dev tools, that I'd really like to see in Firefox.

One of them is the ability to use a fake geolocation. It's really handy to test a website using heavily geolocation abilities (especially mobile webapps). I did not find any discussion about this feature in Bugzilla.

Comment 1

[YF (Yang)]

bug 891796?

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Thanks for the report!  We are actively working on this, stay tuned!

Bug 891796 is very similar, but that might end up being Simulator specific.  So, I'll keep this one open and have it depend on it.

Comment 3

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Assigning P1 priority for some devedition-40 bugs. 

Filter on '148DD49E-D162-440B-AE28-E22632FC20AC'

Comment 4

[Jan Keromnes [:janx]]

Setting difficulty to hard because it needs platform work, semi-complex tools and UX work. Also removing devedition-40 because it's too large of a feature for that.

Regarding the platform size, in bug 891796, I have a patch that can spoof geo coords across the whole browser, but ideally we'll want per-target spoofing and not browser-wide. The security team has also landed a privacy feature (Adjustable Location Accuracy, bug 1057676) which allows per-domain (per-app actually) spoofing, but that's still not as good as per-devtools-target spoofing, and it's a little bit strange to use a privacy feature for devtools.

Comment 5

[Jan Keromnes [:janx]]

s/platform size/platform side/

Comment 6

[Stewart Gordon]

It isn't just for dev testing that this is useful.  Sometimes I have found that the mechanism for determining my location is very inaccurate, and I would like to be able to just tell it my location.

Comment 7

[Benoit Chabod [:bigben]]

Alright, as we've discussed with Ryan and Helen during the workweek in London, we're gonna implement this in the RDM itself for the time being. It should live in a toolbar near the viewport(s), where we might add accelerometer and other settings afterwards.

The first iteration should let the user input latitude and longitude, and then maybe we'll add precision, altitude, something to enter a trajectory over time, etc. There are still lots of unknowns! It might even be moved to the toolbox later on. I'll try to come up with a prototype so that we can started :)

Comment 8

[Benoit Chabod [:bigben]]

Attachment: Part 1: Add platform support for geolocation spoofing

Alright, here is a prototype to add a geolocation spoofing tool in RDM.

The platform side of this patch is based on Jan's work in Bug 925761, so it's not complete yet: it changes the geolocation for the whole browser, and we need to find a way to set it for the current tab only. As discussed in today's visual tools meeting, simply enabling/disabling the mechanism when entering/leaving the tab isn't sufficient because it could still impact other tabs/windows using live geolocation (even when they don't have focus).

Comment 9

[Benoit Chabod [:bigben]]

Attachment: Part 2: UI in RDM for geolocation spoofing

Regarding the RDM itself, I just added some kind of "navigation drawer" that can contain other simulation tools in the future (such as accelerometer). There is a new "settings" button near the screenshot one, and the geolocation panel is very simple for now (it just contains input boxes for latitude and longitude). All of this is subject to debate!

Besides, the new ToolsMenu component module that I created for this new feature is very primitive.
I guess we will should use the React state and actions to implement this in a cleaner way.

Ryan, Helen, what do you think?

Comment 10

[Benoit Chabod [:bigben]]

Attachment: Screencast of the prototype

By the way, here's a GIF of the prototype if you want a quick preview without having to rebuild!

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8768812 [details]
Screencast of the prototype

At a high level, I think it seems like you are heading in a reasonable direction.

Like we discussed, we'll want to work on a way to override geolocation only for a specific tab at a time.

I think there are lots of UX questions to work out, such as:

* is the gear icon enough to suggest what's behind it?
* do we like the style of the drawer?
* should it overlay the viewport like you have, or allow you to work with the viewport while open?

Hoping to hear some feedback from Helen on these points and more.

Comment 12

[Helen V. Holmes (:helenvholmes) (:✨)]

Comment on attachment 8768812 [details]
Screencast of the prototype

I actually don't mind the drawer at all—I think my only holdup is that it would be nice to keep these global settings interactions the same, and currently we're using the modal for that. If were to translate this into a modal, I'm imagining it would look something like this: https://cl.ly/053h1O0z1V17

Ryan brings up a good point that the gear icon should probably be swapped out. We have a globe icon that I think would work here—I'll attach it in a moment.

My first instinct is that we shouldn't allow users to interact with the viewports while they're changing geolocation data for simplicity, but I'd actually like to hear from Benoit on this one—do you have workflows where this isn't the case?

Comment 13

[Helen V. Holmes (:helenvholmes) (:✨)]

Attachment: globe.svg

Comment 14

[Helen V. Holmes (:helenvholmes) (:✨)]

Attachment: error-expanded.svg

Here's the ":(" icon you see in the input error state.

Comment 15

[Tim Nguyen :ntim]

It would be rad to have a map to easily select your location in the popup.

Comment 16

[Benoit Chabod [:bigben]]

Alright, as we've discussed with Ryan and Helen, we're going to keep a drawer because it's more convenient than a modal. I've changed the icon to a globe one and I've added shaking and red outline in case of input error, I'll post a new patch soon.

@Tim: I agree, it would be cool! We thought about this during the London workweek. However, we would need to solve other issues to implement that (which map provider should we use? would this slow down the tool?).

Comment 17

[Tim Nguyen :ntim]

(In reply to Benoit Chabod [:bigben] from comment #16)
> @Tim: I agree, it would be cool! We thought about this during the London
> workweek. However, we would need to solve other issues to implement that
> (which map provider should we use? would this slow down the tool?).

I guess we could use open street map. I don't think it would slow down the tool if we initialise the map dynamically.

Comment 18

[Stewart Gordon]

Amending the summary line to be more general.  See comment 6.

Comment 19

[Benoit Chabod [:bigben]]

Attachment: Part 2: UI in RDM for geolocation spoofing

Comment 20

[Benoit Chabod [:bigben]]

Attachment: Part 3: Make the spoofing tab-specific in platform

Comment 21

[Benoit Chabod [:bigben]]

Attachment: Part 4: Create an emulation actor and send spoofing requests from content process

Comment 22

[Benoit Chabod [:bigben]]

Here's the latest version of my work :)

@Helen, as said earlier, I've followed your advice regarding the icon and the input error state, and I've made the drawer resizable for practical purposes. What do you think?
I will post a GIF shortly after if you want to take a look, but feel free to apply the patches if you want to test it yourself! 

@Ryan, I've implemented an EmulationActor that does the spoofing requests, as we discussed last week.
I've also refactored the geolocation menu, so now it's a separate React component with its own actions, props and state.
The spoofing is also tab-specific, so there won't be any problem with two or more RDM tabs spoofing different locations. This will even support multiple viewports in the future, if needed. Can you give me some feedback on the RDM part of these patches? I will also need someone to take a look at the platform changes.

For the moment, if anyone wants to test this, you will need to use the latest patch from bug 1278763, as permissions doorhangers currently don't appear inside RDM (so you won't be able to test any geolocation website without it).

Comment 23

[Benoit Chabod [:bigben]]

Attachment: Quick demo of the changes in the geolocation UI for RDM

Comment 24

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8778864 [details] [diff] [review]
Part 3: Make the spoofing tab-specific in platform

Review of attachment 8778864 [details] [diff] [review]:
-----------------------------------------------------------------

As for finding a reviewer, take a look at the modules list[1] and also past reviewers for this directory in the commit history.

[1]: https://wiki.mozilla.org/Modules/All

::: dom/geolocation/nsGeolocation.cpp
@@ +924,5 @@
>    for (uint32_t i = 0; i< mGeolocators.Length(); i++) {
> +    //Spoof this geolocator if it matches a spoofing request
> +    nsCOMPtr<nsPIDOMWindowInner> innerWindow = mGeolocators[i]->GetParentObject();
> +    if(innerWindow) {
> +      uint64_t windowID = innerWindow->GetOuterWindow()->WindowID();

I think we ideally want the spoof to be active for all frames in a tab.  One way to do this is only assoicate the spoof with a root docshell (either using it's window ID like you are here or by changing to a docshell attribute).  Then, when you're looking for spoofs, first call GetSameTypeRootTreeItem() on the current docshell to reach the root docshell, and then check for a spoof associated with that.

@@ +981,5 @@
>      }
>    }
>  
> +  // Get the window ID to record which caller wants a spoofed location
> +  JS_GetProperty(cx, obj, "identifier", &val);

It might be more appropriate to the set the spoof as a docshell attribute (instead of using observers like this), which we've done for a number of other emulation changes like this, since you are trying to associate it with a tab anyway.

However, I am not sure what would be more desirable to platform, so perhaps it's best to check with whoever will review this part.

Comment 25

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8778863 [details] [diff] [review]
Part 2: UI in RDM for geolocation spoofing

Review of attachment 8778863 [details] [diff] [review]:
-----------------------------------------------------------------

Overall this seems reasonable at a high level, but we should definitely get UX feedback from Helen.

::: devtools/client/responsive.html/app.js
@@ +97,5 @@
>  
> +  onUpdateGeolocationStatus(isSpoofing, latitude, longitude) {
> +    this.props.dispatch(
> +      updateGeolocationStatus(isSpoofing, latitude, longitude));
> +    if (isSpoofing) {

So far we've mostly avoided directly accessing privileged APIs inside the RDM UI like this.

...but then I remember you probably are removing this in a later part.

::: devtools/client/responsive.html/components/geolocation-menu.js
@@ +19,5 @@
> +}
> +
> +module.exports = createClass({
> +
> +  displayName: "GeolocationMenu",

I think it would be better to separate this into a general "drawer" component that does the side mounting, width handling, etc.  Then there would a separate component for the geolocation bits that is a child of the drawer.  That way we separate the layout concerns of the drawer from the geolocation feature.

Comment 26

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8778865 [details] [diff] [review]
Part 4: Create an emulation actor and send spoofing requests from content process

Review of attachment 8778865 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/responsive.html/app.js
@@ +96,5 @@
>  
>    onUpdateGeolocationStatus(isSpoofing, latitude, longitude) {
> +    let location = { latitude, longitude };
> +
> +    if (isSpoofing != this.props.geolocation.isSpoofing) {

This only posts a message if spoofing toggles between enabled and disabled.  What about if the location is changed while enabled?

::: devtools/client/responsive.html/manager.js
@@ +16,5 @@
>  const TOOL_URL = "chrome://devtools/content/responsive.html/index.xhtml";
>  
> +loader.lazyRequireGetter(this, "DebuggerClient",
> +                         "devtools/shared/client/main", true);
> +

Nit: Remove extra blank line

@@ +428,5 @@
> +    if (isSpoofing) {
> +      location.identifier = identifier;
> +      this.client.request({
> +        to: this.emulationActor,
> +        type: "spoofGeolocation",

Maybe something like `setGeolocationOverride` is a better name at the emulation actor level?  This would give a common naming pattern for other feature like `setTouchEventsOverride`, etc.

@@ +434,5 @@
> +      });
> +    } else {
> +      this.client.request({
> +        to: this.emulationActor,
> +        type: "unspoofGeolocation",

To reset back to normal, maybe something like `resetGeolocationOverride` or `clearGeolocationOverride`.

::: devtools/server/actors/emulation.js
@@ +8,5 @@
> +const protocol = require("devtools/shared/protocol");
> +const { emulationSpec } = require("devtools/shared/specs/emulation");
> +
> +let EmulationActor = protocol.ActorClassWithSpec(emulationSpec, {
> +  initialize(conn) {

Pretty sure this can be removed since the default behavior is to call the super class like you are doing.  If you had custom work to do at init time, it would make sense to keep and do it here.

@@ +22,5 @@
> +    Services.obs.notifyObservers(null, "geolocation-unspoof",
> +      JSON.stringify({ identifier }));
> +  },
> +
> +  destroy() {

Since you are adding a new tab-level actor, you need to add `disconnect` that just calls `this.destroy`.  It's silly.  See https://wiki.mozilla.org/DevTools/Actor_Best_Practices.

@@ +23,5 @@
> +      JSON.stringify({ identifier }));
> +  },
> +
> +  destroy() {
> +    protocol.Actor.prototype.destroy.call(this);

It is critical that destroy correctly resets the page back to normal.  It is fine to also offer clear / reset / unspoof methods, since the user may certainly want to toggle emulation things on and off and on many times.

However, the DevTools connection can be destroyed at any moment, especially with a remote device, so we must try to clean up when this happens without requiring the client to say "please reset now".

So, for the case of geolocation, `destroy` needs to clean up things.

Also, we should mostly likely copy the philosophy of `_restoreDocumentSettings` from webbrowser.js.  The part that brings the most complexity is that nothing guarantees there is only _one_ emulation actor for a given tab.  Maybe it is some day accessed from the toolbox and from RDM separately.  That will create two different emulation actors both trying to manipulate the state of a single tab.  To resolve this problem for user agent, we used this procedure:

1. Create a `_previousGeolocationOverride` (maybe there is a better name) property in the actor that starts out null
2. The first time `setGeolocationOverride` is called, record the current geolocation override state in `_previousGeolocationOverride`.  This implies it is possible to read the geolocation overrides state, which is another nudge towards the docshell attribute instead of observer notifications.
3. When the actor is being destroyed, check if _this actor_ ever set a geolocation override by checking if `_previousGeolocationOverride` is not null.  If it did set one, restore it back to `_previousGeolocationOverride`.  If it did not set one, do nothing.

This may not apply exactly right for the model of geolocation, but we still want something close I suspect.

Comment 27

[Helen V. Holmes (:helenvholmes) (:✨)]

(In reply to Benoit Chabod [:bigben] from comment #22)
> Here's the latest version of my work :)
> 
> @Helen, as said earlier, I've followed your advice regarding the icon and
> the input error state, and I've made the drawer resizable for practical
> purposes. What do you think?
> I will post a GIF shortly after if you want to take a look, but feel free to
> apply the patches if you want to test it yourself! 

This looks really good!

I have a few visual nits and one UX question:

1. (visual) I think adding more left-padding on the drawer will look better—currently the spacing seems very tight and then very arbitrarily wide.
2. (visual) I forget where we landed with the geolocation icon—I think you had concerns about it not being expressive enough?
3. (visual) We've been using drop-shadows to express depth as things open and close, although they're faint. I can't tell from the gif alone, but if there isn't a drop shadow on the drawer, can we use the same style we apply to the frame of the phones, pointing to the left? https://cl.ly/0Y3I3M3t0b2d

4. (ux) to close the drawer, you click the geolocation button again, and it turns off. Are you turning off geolocation as you do this, or is geolocation still enabled?

Comment 28

[Benoit Chabod [:bigben]]

(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #27)
> This looks really good!

Thanks! :D

> 1. (visual) I think adding more left-padding on the drawer will look
> better—currently the spacing seems very tight and then very arbitrarily wide.

I agree.

> 2. (visual) I forget where we landed with the geolocation icon—I think you
> had concerns about it not being expressive enough?

I started with a "gear" icon because I thought we would put other emulation tools in that panel, but eventually we agreed that there will be only geolocation there (for now) so you gave me that "globe" icon. I think it's nice!

> 3. (visual) We've been using drop-shadows to express depth as things open
> and close, although they're faint. I can't tell from the gif alone, but if
> there isn't a drop shadow on the drawer, can we use the same style we apply
> to the frame of the phones, pointing to the left? https://cl.ly/0Y3I3M3t0b2d

Sure, I'll add this in my next iteration.

> 4. (ux) to close the drawer, you click the geolocation button again, and it
> turns off. Are you turning off geolocation as you do this, or is geolocation
> still enabled?

Yeah... I kept hesitating about that. In the current version, the button just opens/closes the drawer and if turns blue just because the drawer is opened, it has no effect whatsoever on the spoofing itself. It's really the checkbox inside the panel that controls and shows the real geolocation status. Do you think this is wrong?

Comment 29

[Helen V. Holmes (:helenvholmes) (:✨)]

(In reply to Benoit Chabod [:bigben] from comment #28)
> (In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #27)
> > 4. (ux) to close the drawer, you click the geolocation button again, and it
> > turns off. Are you turning off geolocation as you do this, or is geolocation
> > still enabled?
> 
> Yeah... I kept hesitating about that. In the current version, the button
> just opens/closes the drawer and if turns blue just because the drawer is
> opened, it has no effect whatsoever on the spoofing itself. It's really the
> checkbox inside the panel that controls and shows the real geolocation
> status. Do you think this is wrong?

I think adding a button within the drawer, to close the drawer, would help. I would recommend two buttons side-by-side:

[ Turn off Geolocation ]   [ Turn on Geolocation ]

Clicking either would close the drawer, but turning it on would turn the icon blue; turning it off would turn it gray.

Comment 30

[Tim Nguyen :ntim]

Benoit, are you planning to finish this?

Comment 31

[Benoit Chabod [:bigben]]

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #30)
> Benoit, are you planning to finish this?

Yes, I would like to!
I don't have enough time right now, but I may able to continue this bug in a few weeks.
I'll reach out to you if I can't.

Comment 32

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I am going to assume this is not actively worked on for now.  Feel free to reassign again if that is incorrect.

Comment 33

[techieshark]

Would it make sense to start by adding two configuration fields for hard coding your lat/lon via about:config?

Right now you can already spoof your location in a browser-wide fashion by overriding the browser setting geo.wifi.uri but it isn't very obvious (see https://security.stackexchange.com/a/147176).

Sidenote: It also seems like this could be a possible privacy-enhancing feature, since currently on desktop browser, Firefox is asking Google to give us the Lat/Long based on the IP address.