Issue metadata
Sign in to add a comment
|
Update permission message for location uses message for multiple permissions |
||||||||||||||||||||||||
Issue descriptionChrome Version: 59.0.3063.0 OS: Android 7.1.1 Device: Nexus 5X What steps will reproduce the problem? (1) Search Google in Chrome Canary. Might need to be a location-sensitive search. What is the expected result? The prompt makes sense. What happens instead? "Chrome needs permissions access to share them with this site." (needs-permissions-infobar.png) 1) That sentence has poor grammar. 2) The sentence doesn't communicate that the site already has a permission granted. This is exposed in Page Info (page-info-search-engine-geolocation-disclosure.png), but it's extra confusing for the special case where the default search engine is granted geolocation. 3) It's unclear what permissions are affected. The plural word "permissions" implies multiple permissions, even if in this case it's just one. 4) "Update permissions" is a blue action button. This is *exactly* the same styling we use for the "Allow" button in a permission prompt (mic-prompt.png). Although there is a secondary modal prompt from the OS (os-prompt.png). If I didn't already know a lot about the Android and Chrome permission models, I'd be scared to press that button. (In this case, also consider that Google has a reputation for trying to siphon up all your data.) Note that this example uses the search engine geolocation case because that's how I triggered it. However, we should make sure to address this if it can ever happen for other permissions. I think this prompt falls way below Enamel's standards of clarity. Could we try to address it soon? Dom, could you triage?
,
Apr 6 2017
Yeah, I agree this prompt could use some improvement. kcarattini and benwells were looking at this as a part of the search geolocation project - I'm not exactly sure what the state of that is regarding default-granting. benwells can update. The tricky thing here is that the bar is a generic infobar that's triggered if: - Chrome doesn't have permission for location, mic, camera, or notifications - The user *grants* one (or more) of those permissions . For instance, getUserMedia() might grant two. The styling of the bar is mandated by the material design infobar spec - just like permissions infobars - which is why the action button is blue. I'm not sure if we can change the styling, but the text might be able to be clarified. That's a good start.
,
Apr 7 2017
We haven't changed / really thought about this dialog for either LSD or search geolocation consistency, but it has introduced many weird edge cases and code complexity. Agreed the wording is unnatural, probably grammatically incorrect, and not informative. +srahim who might have some ideas for language. Lucas - why P1? I agree we should fix but I'd put it to P2 given this dialog has been around since Android M.
,
Apr 7 2017
,
Apr 7 2017
,
Apr 7 2017
Feel free to move to P2, since Sydney owns the code. I would place this above a lot of other permission bugs I filed recently, though.
,
Apr 10 2017
Looks like this was introduced in M58. before that it said "Chrome needs location access to share it with this site. +tedchoc - I think this might be something to do with permissions related changes you made recently. WDYT?
,
Apr 10 2017
To the best of my knowledge, this string hasn't changed since pre Android m. I think it has only started appearing because we have pre granted location to the dse without chrome having it in the first place. In general, this is a state that Chrome was never designed to be in. This would previously have required a user to grant permissions to a site, then go to Android settings to revoke it. Now, I believe Chrome is granting site permission prior to app permission, which is exposing a UI that we really never expected any "normal" user to see. Sure the text could be improved, but that seems like a minor issue to the fact that users are seeing this at all in any large amount.
,
Apr 10 2017
The permission changes in 58 should only have changed how Chrome tracks whether we have previously requested permissions (and only on Android O). If that is wrong and Chrome is confused about whether we can request it, then that could also trigger this, and something we/I should definitely track down.
,
Apr 10 2017
There is another bug about this dialog showing up, this one is specifically for the confusing test. I should have been more clear about the change I was thinking of - it was this one: https://chromium.googlesource.com/chromium/src/+/653256d7f577693757276034d109ac77de359705 Anyway, I took a look and have a CL to fix it up here: https://codereview.chromium.org/2807093002/ We probably should still improve the text for asking for multiple permissions (although I'm not sure if that is possible to show normally).
,
Apr 10 2017
'There is another bug about this dialog showing up' should read 'There is another bug about this dialog showing up TOO MUCH'
,
Apr 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d3a2a83fbff70f8beb2f1bbdf691fc2cbb27ea0d commit d3a2a83fbff70f8beb2f1bbdf691fc2cbb27ea0d Author: benwells <benwells@chromium.org> Date: Mon Apr 10 22:19:46 2017 Fix the Update Permissions infobar for geolocation. Due to some earlier changes to do with mapping chrome to android permissions, this infobar incorrectly thinks there are multiple permissions being requested for geolocation. BUG= 709206 Review-Url: https://codereview.chromium.org/2807093002 Cr-Commit-Position: refs/heads/master@{#463418} [modify] https://crrev.com/d3a2a83fbff70f8beb2f1bbdf691fc2cbb27ea0d/chrome/browser/permissions/permission_update_infobar_delegate_android.cc
,
Apr 10 2017
I agree with lgarron@ that the text is quite confusing. Ben, please advise on when you'd like a new string, and I'll schedule the work accordingly.
,
Apr 10 2017
tedchoc - do you think we should merge the fix in #12? srahim - there was a bug which caused the message to be a bit garbled. It should say 'Chrome needs location access to share it with this site.' but due to the bug it instead says 'Chrome needs permissions access to share them with this site.' Do you mean the version with 'location' is confusing, or just the 'permissions' version?
,
Apr 10 2017
Yeah, let's merge #12 in. The fix is nice, clean, and small and the bug is subtle but gross.
,
Apr 10 2017
Re #12: oof, subtle! nice find/fix
,
Apr 11 2017
OK ... let's keep this bug for the recent bug. I've logged issue 710362 for the dodgy string (which I just investigated and found can occur with normal use).
,
Apr 12 2017
Tested on Canary. amineer: feel free to reject as I think you're only taking stability fixes for M58 now.
,
Apr 12 2017
This bug requires manual review: We are only 12 days from stable. Please contact the milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Apr 12 2017
benwells@, both strings are confusing because of the use of "it" and "them". Ideally, the location one would say something along the lines of: 'Chrome needs location access to share your location with this site' 'For this site to work properly, share your location with Chrome and the site' Strings don't block the associated eng fixes that have been made, but this is an example of a string that will probably trip up Loc. It would be nice to clarify the location string as well as the permissions one.
,
Apr 12 2017
FWIW, I think that the second string suggests an error condition and does not reflect that it's the user's choice whether to grant permission or not.
,
Apr 13 2017
OK. let's discuss strings in the issue 710362 (assigned to srahim to sort out the strings) and keep this bug for the recent regression that caused the multiple message to be used for location.
,
Apr 13 2017
I'm comfortable taking a minor fix like this for a gross bug in a key workflow - less so for merges for interventions (like your other request I rejected). Approved for M58 branch 3029.
,
Apr 17 2017
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible! If all merges have been completed, please remove any remaining Merge-Approved labels from this issue. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Apr 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/eebbac03043ca5ee7eccf0c40285b9fbcf3f4e22 commit eebbac03043ca5ee7eccf0c40285b9fbcf3f4e22 Author: Ben Wells <benwells@chromium.org> Date: Tue Apr 18 04:28:22 2017 Fix the Update Permissions infobar for geolocation. Due to some earlier changes to do with mapping chrome to android permissions, this infobar incorrectly thinks there are multiple permissions being requested for geolocation. BUG= 709206 Review-Url: https://codereview.chromium.org/2807093002 Cr-Commit-Position: refs/heads/master@{#463418} (cherry picked from commit d3a2a83fbff70f8beb2f1bbdf691fc2cbb27ea0d) Review-Url: https://codereview.chromium.org/2828443002 . Cr-Commit-Position: refs/branch-heads/3029@{#739} Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471} [modify] https://crrev.com/eebbac03043ca5ee7eccf0c40285b9fbcf3f4e22/chrome/browser/permissions/permission_update_infobar_delegate_android.cc
,
Apr 18 2017
Apologies for the delay in merging, we had a super-long 4 day weekend in Sydney.
,
Apr 19 2017
To verify this issue could you please attach the expected screen shots? |
|||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||
Comment 1 by lgar...@chromium.org
, Apr 6 2017