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

fix(android): respect requested location accuracy on Android 12+ #250

Merged
merged 1 commit into from
Oct 18, 2022

Conversation

dpa99c
Copy link
Contributor

@dpa99c dpa99c commented Aug 2, 2022

Platforms affected

Android

Motivation and Context

Prior to Android 12, both COARSE and FINE location permissions were treated synonymously - requesting one would grant the other.
Therefore this plugin currently checks/requests both permissions on Android.

However, when an app is built with an SDK version (and running on device with) Android 12 (API 31) or higher, Android differentiates between precise vs approximate accuracy using these permissions (similar to iOS 14+) - see the Android documentation.
So currently when the methods of this plugin are invoked on Android 12+, this results in both both COARSE and FINE location permissions being requested, regardless of the value set for enableHighAccuracy in PositionOptions, resulting in the user being asked to choose between precise vs approximate location permission:

Description

This PR changes the Android implementation to respect the value of enableHighAccuracy passed in PositionOptions so if enableHighAccuracy: false then only COARSE location permission is requested, resulting in the user only being asked to grant approximate location permission:

If the user grants this permission and enableHighAccuracy: true is subsequently requested, the user in prompted to grant an upgrade from approximate to precise location permission:

If the app's first permission request is for enableHighAccuracy: true, then the user will be presented with the choice between approximate and precise location:

I believe this PR resolves issues #188 and #235.

One further change this PR makes is to remove the check for location permissions on Android before invoking clearWatch() since this is unnecessary.

Issues with approximate location permission on Android

Note: As @jcesarmobile points out in this comment, there is a bug in the Chromium WebView which is present up until API 32 (empirical testing shows it to manifest in every API from 22 to 31 but not in 32+ where it appears to be fixed).

It happens when COARSE but not FINE location permission is granted and enableHighAccuracy: false is requested, and results in a {"code":3,"message":"Timeout"} error when getCurrentLocation() or watchPosition() is called, rather than returning the current location.
Due to this issue, this PR implements API version-conditional functionality so that even when enableHighAccuracy: false is specified, both COARSE and FINE location permissions will still requested on API 31 and below, while on API 32 and above only COARSE permission will be requested.

A further complication is specific to devices running API 31: even if the user grants precise (both COARSE and FINE) location permissions, if enableHighAccuracy: false is passed in the PositionOptions object to the W3C Geolocation API function on the original navigator.geolocation by this plugin, then this also results in a {"code":3,"message":"Timeout"} error.
To prevent this, this PR modifies the native getPermission method on Android to return the current API level of the device.
The web layer of the plugin www/android/geolocation.js then checks this returned value and if it's equal to 31, ensures enableHighAccuracy: true is passed to to the W3C Geolocation API function, even if enableHighAccuracy: false was specified by the app.

There is one further edge case: on Android 12+ (API 32+), if enableHighAccuracy: true is requested but the user grants only approximate (COARSE but not FINE) location permission, this will result in a {"code":1,"message":"Illegal Access"} error.
This is best handled by the app informing the user that precise location permission is required but they only granted approximate location permission.
The test harness app I created for this PR demonstrates this.

Testing

  • Ran the tests in cordova-paramedic with the same results between current plugin master branch and PR branch:

To run the test harness app with this PR branch of the plugin:

git clone https://github.com/dpa99c/cordova-plugin-geolocation-test && cd cordova-plugin-geolocation-test
cordova platform add android@latest
cordova run android

To run the test harness app with the apache/master branch of the plugin:

git clone https://github.com/dpa99c/cordova-plugin-geolocation-test && cd cordova-plugin-geolocation-test
git checkout apache_master
cordova platform add android@latest
cordova run android

Checklist

  • I've run the tests to see all new and existing tests pass
  • I added automated test coverage as appropriate for this change (NOT APPLICABLE)
  • Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
  • If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
  • I've updated the documentation if necessary (NOT APPLICABLE)

@dpa99c dpa99c requested review from breautek and jcesarmobile August 2, 2022 14:13
@dpa99c dpa99c force-pushed the android_permission_types branch from 12784a8 to 75bc6f5 Compare August 2, 2022 14:25
bhandaribhuminpfizer added a commit to bhandaribhuminpfizer/cordova-plugin-geolocation-12 that referenced this pull request Aug 23, 2022
@bhandaribhumin
Copy link

When do you plan to merge these changes?

Copy link

@totalpave totalpave left a comment

Choose a reason for hiding this comment

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

lgtm

ios unit test failures are unrelated to this PR.

Copy link
Contributor

@breautek breautek left a comment

Choose a reason for hiding this comment

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

@totalpave user was me -- accidentally reviewed under the wrong chrome tab...

@expcapitaldev
Copy link

hi all!, waiting update

@dpa99c
Copy link
Contributor Author

dpa99c commented Oct 5, 2022

@breautek are we good to merge this?

@expcapitaldev
Copy link

@breautek Can I help here , because waiting that changes?

Copy link
Contributor

@breautek breautek left a comment

Choose a reason for hiding this comment

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

My apologies. My initial review I guess wasn't sufficient, I did a another re-review and actually ran a test app against this PR and found a couple of issues which are noted below (or above?)

Once these issues are resolved, then I think we are good to merge.

@expcapitaldev
Copy link

@dpa99c can you check your MR ?

…esting permissions on Android 12+

Handle bug on API < 32 when requesting COARSE permission results in TIMEOUT error.
@dpa99c dpa99c force-pushed the android_permission_types branch from 75bc6f5 to 73719d9 Compare October 18, 2022 09:28
@dpa99c dpa99c requested a review from breautek October 18, 2022 09:29
@dpa99c
Copy link
Contributor Author

dpa99c commented Oct 18, 2022

@breautek I've made the requested changes - can you re-review and confirm you are happy to merge?

@erisu erisu changed the title (android) fix: respect requested location accuracy on Android 12+ fix(android): respect requested location accuracy on Android 12+ Oct 18, 2022
Copy link
Contributor

@breautek breautek left a comment

Choose a reason for hiding this comment

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

Re-tested the changes and appears to work as expected now.

All android tests passes. Confirmed that the failing iOS/Chrome test weren't the cause of this PR (they have been failing with commits on current master)

I think we are good to merge @dpa99c 👍

Thanks

@expcapitaldev
Copy link

thanks a lot

@mobiliseapplabllp
Copy link

My geolocation plugin working fine before 2 day suddenly my one plus nord and many client compalnt application location not detect. it was previously working fine

"cordova-plugin-geolocation": "^4.1.0",
"@ionic-native/geolocation": "^5.36.0",

@js-kulkarni
Copy link

All of a sudden "cordova-plugin-geolocation" stopped working on some models of VIVO, OPPO, and some MI mobile phones from 17 Nov 2022. When can we expect a quick fix?

@gohrms
Copy link

gohrms commented Nov 25, 2022

@js-kulkarni , @mobiliseapplabllp I am also facing the same issue. Since few days suddenly it stopped working on few mobile phone and in few others it is working properly. Tried so damn hard to get it working. Lets stay connected if someone finds a solutions then lets update here. Thank you.

@gohrms
Copy link

gohrms commented Nov 30, 2022

Guys, Any update pls ? Any help for this topic will be appreciated.

@KishanVaishnani
Copy link

I am facing the same issues in VIVO, OPPO, and MI

I have enabled enableHighAccuracy: true and it will ask for 1 popup with both permissions If I choose Approximate then the code breaks.

@gohrms
Copy link

gohrms commented Dec 1, 2022

Good news, seems like Android have some update on Play Service which got download on VivoPhone automatically. I did restart my phone and the GPS is working fine now which earlier was not working on Vivo.
Can anyone please confirm on their phone, where GPS was not working.

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.

9 participants