901383 - Some permissions dialogs should have a 'more info' capability.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[Doug Turner (:dougt)]

Attachment: Firefox desktop geolocation doorhanger

For geolocation, it is important that we offer the user a way to see more information about the permission they are granting.

On ff desktop, the door hanger for geolocation we offer an informational link.

Comment 1

[Doug Turner (:dougt)]

Attachment: B2G permission dialog with the 'More info...' link

Comment 2

[Doug Turner (:dougt)]

Attachment: B2G permission dialog after the user clicks on the 'More info...' link.

Comment 3

[Doug Turner (:dougt)]

Attachment: rough draft of gaia changes to support this.

Comment 4

[Doug Turner (:dougt)]

jachen, can you review for UI.  I'd like feedback.

Comment 5

[Jason Smith [:jsmith]]

Stephany - Who on the UX team would be good to review a permission prompt change?

Comment 6

[[:fabrice] Fabrice Desré]

Comment on attachment 785555 [details] [diff] [review]
rough draft of gaia changes to support this.

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

::: apps/system/index.html
@@ +723,5 @@
>            <div class="inner">
>              <h2 id="permission-message"></h2>
> +            <div id="permission-more-info">
> +              <a id="permission-more-info-link" data-l10n-id="permission-more-info-link">More Info...</a>
> +              <iframe id="permission-more-info-frame" frameborder="0" scrolling=auto></iframe> 

minor nit: trailing whitespace.

::: apps/system/js/permission_manager.js
@@ +55,4 @@
>        str = _(permissionID + '-webRequest', { 'site': detail.origin });
>      }
>  
> +    var moreInfoUrl = "";

s/""/

@@ +57,5 @@
>  
> +    var moreInfoUrl = "";
> +    if (detail.permission = "geolocation") {
> +      moreInfoUrl = "http://dougt.org";
> +    }

In general it would be better to not hard code these permission -> url mapping, but that can be done in a followup.

@@ +173,5 @@
>      // Note plain text since this may include text from
>      // untrusted app manifests, for example.
>      message.textContent = msg;
>  
> +    if (moreInfoUrl != "") {

if (moreInfoUrl) { ...

@@ +175,5 @@
>      message.textContent = msg;
>  
> +    if (moreInfoUrl != "") {
> +      moreInfo.style.display = "block";
> +      moreInfo.addEventListener('click', function(event) { 

nit: trailing ws

@@ +178,5 @@
> +      moreInfo.style.display = "block";
> +      moreInfo.addEventListener('click', function(event) { 
> +          event.preventDefault();
> +
> +          // I am sure there is a better way.

changing the class instead of explicitly setting the display value may be better, but you'll still need to switch to about:blank

::: apps/system/style/permission_manager/permission_manager.css
@@ +82,5 @@
> +
> +#permission-more-info iframe {
> +  display: none;
> +  overflow: hidden;
> +  frameboarder: 0;

I guess you meant border?

Comment 7

[[:fabrice] Fabrice Desré]

Attachment: prompt-more-info.patch

Updated patch, with generic support for "More info..." links if the appropriate l10n strings are there.

The two "new" strings are just copied from https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/ftu/locales/ftu.en-US.properties#L96 so we should not have to ask anything to localizers.

Comment 8

[[:fabrice] Fabrice Desré]

Attachment: screenshot

The new geolocation prompt

Comment 9

[Jason Smith [:jsmith]]

(In reply to Fabrice Desré [:fabrice] from comment #7)
> Created attachment 786616 [details] [diff] [review]
> prompt-more-info.patch
> 
> Updated patch, with generic support for "More info..." links if the
> appropriate l10n strings are there.
> 
> The two "new" strings are just copied from
> https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/ftu/
> locales/ftu.en-US.properties#L96 so we should not have to ask anything to
> localizers.

Aren't those strings in combination too long for the permission prompt? The screenshot seems to imply the strings exceed the length of the prompt.

Comment 10

[[:fabrice] Fabrice Desré]

(In reply to Jason Smith [:jsmith] from comment #9)

> 
> Aren't those strings in combination too long for the permission prompt? The
> screenshot seems to imply the strings exceed the length of the prompt.

The text area is scrollable to let you read the full story.

Comment 11

[Andreas Gal :gal]

Whats the localization story here? Anything new to localize here? I am not good at reviewing this. Pick anyone else. Feedback+ from me.

Comment 12

[Axel Hecht [:Pike]]

Comment on attachment 786616 [details] [diff] [review]
prompt-more-info.patch

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

There's significant l10n impact in this patch.

::: apps/system/js/permission_manager.js
@@ +60,5 @@
> +    if (moreInfoText) {
> +      var evenMoreText = _(permissionID + '-more-info2');
> +      if (evenMoreText) {
> +        moreInfoText += " " + evenMoreText;
> +      }

hard-coding the " " is not a good idea, not all languages use whitespace.

::: shared/locales/permissions/permissions.en-US.properties
@@ +41,5 @@
>  perm-geolocation=Geolocation
>  perm-geolocation-appRequest={{app}} would like to know your location.
>  perm-geolocation-webRequest={{site}} would like to know your location.
> +perm-geolocation-more-info = {{brandShortName}} uses data such as GPS, Wi-Fi, and mobile networks near you to help determine your approximate location.
> +perm-geolocation-more-info2=Data is sent to Mozilla and other service providers and will be used in the aggregate to improve those location databases.  Data may be stored on your device and data collection may occur even when no apps are running.

I'd not use '2' here, it's often used to mean something else.

Comment 13

[[:fabrice] Fabrice Desré]

(In reply to Axel Hecht [:Pike] from comment #12)

> ::: apps/system/js/permission_manager.js
> @@ +60,5 @@
> > +    if (moreInfoText) {
> > +      var evenMoreText = _(permissionID + '-more-info2');
> > +      if (evenMoreText) {
> > +        moreInfoText += " " + evenMoreText;
> > +      }
> 
> hard-coding the " " is not a good idea, not all languages use whitespace.

What is the usual way to do that then?


> 
> I'd not use '2' here, it's often used to mean something else.

And... what will you use?

Comment 14

[Doug Turner (:dougt)]

These strings were reused from the FTU application.

See: http://hg.mozilla.org/gaia-l10n/en-US/file/default/apps/communications/ftu/ftu.properties#l97

Comment 15

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 786616 [details] [diff] [review]
prompt-more-info.patch

Etienne is the permission dialog guy. He is already a fan of your trailing whitespaces :)

Comment 16

[Doug Turner (:dougt)]

required for 1.1 to ship geolocation w/ agps.  let's move.

Comment 17

[Etienne Segonzac (:etienne)]

Comment on attachment 786616 [details] [diff] [review]
prompt-more-info.patch

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

::: apps/system/js/permission_manager.js
@@ +36,4 @@
>        // The message to be displayed on the approval UI.
>        var message =
>          _('fullscreen-request', { 'origin': detail.fullscreenorigin });
> +      fullscreenRequest = requestPermission(message, "",

single quotes, or the linter won't let you commit :)

@@ +60,5 @@
> +    if (moreInfoText) {
> +      var evenMoreText = _(permissionID + '-more-info2');
> +      if (evenMoreText) {
> +        moreInfoText += " " + evenMoreText;
> +      }

I really wish we'd just use one |perm-geolocation-more-info| l10n key here.

We need to add the |more-info| key anyway, and the l10n team is advising against the |*-more-info2| so it sounds like a no-brainer :)

@@ +119,5 @@
>      yes.callback = null;
>      no.removeEventListener('click', clickHandler);
>      no.callback = null;
> +
> +    moreInfoLink.removeEventListener('click', clickHandler);

we need to |moreInfo.classList.add('hidden');| here so the next permission request doesn't automatically get a more info link.

@@ +186,5 @@
>  
> +    if (moreInfoText) {
> +      // Show the "More info…" link.
> +      moreInfo.classList.remove('hidden');
> +      moreInfoLink.classList.add('visible');

and we don't need to toggle the class on the link

Comment 18

[[:fabrice] Fabrice Desré]

Attachment: updated patch

Addressing review comments, and switching back to using a single string.

Comment 19

[Axel Hecht [:Pike]]

FTR, this is still two added strings to the product. There's little confidence that I can give about those being localized by monday.

Comment 20

[Etienne Segonzac (:etienne)]

Comment on attachment 787043 [details] [diff] [review]
updated patch

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

All good, thanks!

Comment 21

[[:fabrice] Fabrice Desré]

Pushed to master: https://github.com/mozilla-b2g/gaia/commit/0b1404818d1d1069ff79320c5fce7708be0b2f20

Comment 22

[John Ford [:jhford] CET/CEST Berlin Time]

[v1-train b4c80f5]

Comment 23

[Tim Guan-tin Chien [:timdream] (please needinfo)]

v1.1.0hd: b4c80f5f124ac10a320312d5c636f47f8e1042cb

Comment 24

[Jason Smith [:jsmith]]

Sanity checks look okay on b2g18 on a leo comm ril device on 8/9.

Comment 25

[sarsenyev]

After going through all locales for this bug, seems like Turkish language "More Info" dialog is not localized on Leo B2G18 COM RIL, due to this a new bug was opened
https://bugzilla.mozilla.org/show_bug.cgi?id=905421

Environmental Variables
Build ID: 20130814041202
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/15813d776a69
Gaia: dd3959fa74e356a528daa76ffee14c2c728a4b56
Platform Version: 18.1
RIL Version: 01.01.00.019.190 
Firmware verision: D300f080