1319112 - Adjust strings for the WebRTC permission notifications

Status: VERIFIED FIXED

(Firefox :: Site Identity, defect, P1)

Description

[:Paolo Amadini]

We landed a first version of the strings for the permission notifications in bug 1282768 in order to have the new design available in Nightly, but we still need to refine them, for example addressing the issues in bug 1282768 comment 97 and bug 1282768 comment 98.

Comment 1

[:Paolo Amadini]

The reason for not implementing the "Will you allow [domain name] to access your:" + "Microphone" suggestion (see bug 1282768) is that I'm not sure about its localizability. I think we want to use whole phrases wherever possible?

I also wonder if we should have multiple string IDs for the "Allow" / "Don't Allow" answers for each question (i.e. they may need to be modified to match the question because of some grammar rule in some language) or we can reuse the same string ID like we do for all the "Yes" / "No" or "OK" / "Cancel" answers in standard dialogs.

Comment 2

[Francesco Lodolo [:flod]]

Reusing labels for buttons: it's safer to have separate labels, as bad as it sounds in terms of number of strings. 

For example you might need to adapt to the gender of the preceding noun, e.g. in Italian (unrealistic example): "Condividere il microfono?" -> "Condividilo", "fotocamera" -> "condividila". There are ways to work around it, but the result might be a less natural translation. l20n will solve that…

Note that there are languages where YES/NO answers don't exist, you need to repeat the verb in the question (e.g. Irish), so having shared YES/NO might be annoying for them.

Can you point me to an example for the first question? In general, if the noun is variable (number, gender) it might be problematic, and you need full strings.

Comment 3

[Lina Butler [:lina]]

I'm not sure if this is the right bug to suggest this, but would something like "site.com wants to send you notifications" or "site.com wants to store data on your computer" work for the English string? (Phrasing it as a statement instead of a question).

The "Will you allow..." string feels a bit awkward because it's asking a yes-no question, but the answers aren't "Yes" or "No". It might also help clarify the notification permission: "allow to send notifications" sounds like it's asking if you want to let the site send notifications to other people.

Comment 4

[mheubusch]

Hello - for camera and microphone where a user is required to select a specific device, the message should read as follows:
If one device:
Will you allow [domain name] to access this device:
Video Camera
X Remember this decision
[Don’t Allow] [Allow]

Will you allow [domain name] to access this device:
Microphone 
X Remember this decision 
[Don’t Allow] [Allow]

If more than one device: 
Will you allow [domain name] to access these devices:
Video Camera
Microphone 
X Remember this decision
[Don’t Allow] [Allow]

Also made these updates to the documentation here: https://docs.google.com/document/d/143nEfWfIvFZD2-pFqE8GD85inFjFIhE2J8QeegQufc0/edit#

Comment 5

[Florian Quèze [:florian]]

https://public.etherpad-mozilla.org/p/bug1319112 is a summary of the various proposals we've had so far, and I've tried to list the issues with each of them.

Comment 6

[:Paolo Amadini]

Attachment: Bug 1319112 - Define strings for the permission notifications.

Review commit: https://reviewboard.mozilla.org/r/104554/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104554/

Comment 7

[:Paolo Amadini]

I tested this for camera, microphone, and basic screen sharing, and it works fine.

Comment 8

[:Paolo Amadini]

Comment on attachment 8826630 [details]
Bug 1319112 - Define strings for the permission notifications.

Requesting general feedback from Francesco about this patch.

Also, for consistency, I've removed the extra spaces around the equal sign for some strings. It looks like leading spaces are already ignored when reading the strings from code, but if removing the spaces can cause issues to localization tools then I can revert this change for the strings that have not changed.

Comment 9

[Francesco Lodolo [:flod]]

Comment on attachment 8826630 [details]
Bug 1319112 - Define strings for the permission notifications.

In this specific case I wouldn't duplicate the button labels "Allow" and "Don't allow", since all the messages associated have the same structure.

Removing spaces around the equal sign is not a problem, they're ignored.
https://dxr.mozilla.org/mozilla-central/source/xpcom/ds/nsPersistentProperties.cpp#411

Comment 10

[:Paolo Amadini]

Comment on attachment 8826630 [details]
Bug 1319112 - Define strings for the permission notifications.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104554/diff/1-2/

Comment 11

[:Paolo Amadini]

(In reply to Francesco Lodolo [:flod] from comment #9)
> In this specific case I wouldn't duplicate the button labels "Allow" and
> "Don't allow", since all the messages associated have the same structure.

Maybe I've interpreted your previous comment 2 too literally, anyways here is a new patch that keeps the existing structure for the button labels.

Comment 12

[:Paolo Amadini]

Comment on attachment 8826630 [details]
Bug 1319112 - Define strings for the permission notifications.

https://reviewboard.mozilla.org/r/104554/#review105432

::: browser/modules/webrtcUI.jsm:345
(Diff revision 2)
>    let chromeDoc = aBrowser.ownerDocument;
>    let stringBundle = chromeDoc.defaultView.gNavigatorBundle;
> -  let stringId = "getUserMedia.share" + requestTypes.join("And") + "2.message";
> +
> +  // Mind the order, because for simplicity we're iterating over the list using
> +  // "includes()". This allows the rotation of string identifiers. We list the
> +  // full identifiers here so they can be cross-references more easily.

nit: I've fixed this typo in the comment locally.

Comment 13

[Francesco Lodolo [:flod]]

(In reply to :Paolo Amadini from comment #11)
> Maybe I've interpreted your previous comment 2 too literally, anyways here
> is a new patch that keeps the existing structure for the button labels.

Sorry, I can totally see why it's confusing. 

In this case all sentences have the same structure "Will you allow X to DO SOMETHING?". Having the same button labels should be relatively safe, and it's a risk I'm willing to take given the number of extra strings introduced if we separate them.

I would only add a short comment before the button labels, explaining that the buttons are used for all getUserMedia.* permission requests.

Comment 14

[:Paolo Amadini]

Comment on attachment 8826630 [details]
Bug 1319112 - Define strings for the permission notifications.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104554/diff/2-3/

Comment 15

[:Paolo Amadini]

Makes sense, thanks. I've added the new comment as a proper localization note.

Comment 16

[Florian Quèze [:florian]]

Comment on attachment 8826630 [details]
Bug 1319112 - Define strings for the permission notifications.

https://reviewboard.mozilla.org/r/104554/#review105652

Thanks!

::: browser/modules/webrtcUI.jsm:357
(Diff revision 3)
> +    // Combinations of the above request types last.
> +    "getUserMedia.shareCameraAndMicrophone2.message",
> +    "getUserMedia.shareCameraAndAudioCapture2.message",
> +    "getUserMedia.shareScreenAndMicrophone3.message",
> +    "getUserMedia.shareScreenAndAudioCapture3.message",
> +  ].find(id => id.includes(requestTypes.join("And")));

nit: I would put requestTypes.join("And") in a temporary variable to avoid doing it up to 8 times.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3015d6154ec5
Adjust strings for the WebRTC permission notifications. r=florian

Comment 18

[:Paolo Amadini]

Edited bug summary to reflect what we ended up implementing.

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3015d6154ec5

Comment 20

[Stefan Plewako [:stef]]

https://hg.mozilla.org/mozilla-central/diff/3015d6154ec5/browser/locales/en-US/chrome/browser/browser.properties:
> -getUserMedia.reasonForNoPermanentAllow.screen=%S can not allow permanent access to your screen or application without asking which one to share.
> +getUserMedia.reasonForNoPermanentAllow.screen2=%S can not allow permanent access to your screen without asking which one to share.

This new string will be displayed in situations with more than one screen present only or it doesn't make sense and will confuse users?

Comment 21

[:Paolo Amadini]

(In reply to Stefan Plewako [:stef] from comment #20)
> https://hg.mozilla.org/mozilla-central/diff/3015d6154ec5/browser/locales/en-
> US/chrome/browser/browser.properties:
> > -getUserMedia.reasonForNoPermanentAllow.screen=%S can not allow permanent access to your screen or application without asking which one to share.
> > +getUserMedia.reasonForNoPermanentAllow.screen2=%S can not allow permanent access to your screen without asking which one to share.
> 
> This new string will be displayed in situations with more than one screen
> present only or it doesn't make sense and will confuse users?

If Florian doesn't disagree, and doesn't know of other changes already in the roadmap that would solve this, I believe we could file a bug for having a better wording for Firefox 54.

In my opinion, we may either want a message that better conveys the reason why the option to allow is disabled, or we shouldn't mention the reason at all. After all, being detailed about the reason doesn't change the fact that the permanent permission cannot be granted. Differently from the HTTP/HTTPS case, where the fact the connection is insecure can potentially be actionable, here knowing the reason doesn't make a real difference.

That's also why I think that while user confusion is possible, it's not a blocker for the Firefox 53 release.

Comment 22

[Florian Quèze [:florian]]

(In reply to Stefan Plewako [:stef] from comment #20)
> https://hg.mozilla.org/mozilla-central/diff/3015d6154ec5/browser/locales/en-
> US/chrome/browser/browser.properties:
> > -getUserMedia.reasonForNoPermanentAllow.screen=%S can not allow permanent access to your screen or application without asking which one to share.
> > +getUserMedia.reasonForNoPermanentAllow.screen2=%S can not allow permanent access to your screen without asking which one to share.
> 
> This new string will be displayed in situations with more than one screen
> present only or it doesn't make sense and will confuse users?

It's displayed for all screen sharing cases. This includes:
- only one screen (and yes it's confusing in that case)
- a lost of several screens that the user has to select from
- a list of windows that the user has to select from
- a lost of applications that the user has to select from.

That message is inaccurate for that first case, but the problem still exists even in that case: it's not because there's only one screen at the time you first grant permission that you won't have plugged an additional external screen by the time the same site requests a screen share again.

Paolo, we can file a bug, sure. I hope someone can come up with a wording that doesn't cause confusion. I'm reluctant to just hiding the message completely, as it's surprising to users to disable a button when they check the checkbox, without explaining why we are doing it. I agree it's not a 53 blocker.

Comment 23

[:Paolo Amadini]

(In reply to Florian Quèze [:florian] [:flo] from comment #22)
>  I'm reluctant to just hiding the message completely

To clarify, one of the options I'm suggesting is to just say something like "%S can not allow permanent access to your screen.", but I'm not suggesting to hide the label completely.

Comment 24

[Florian Quèze [:florian]]

(In reply to :Paolo Amadini from comment #23)

> say something like
> "%S can not allow permanent access to your screen."

Seems OK to me.

Comment 25

[Hani Yacoub, Desktop QA]

Build ID: 20170207030214
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0

Verified as fixed on on Windows 10 x 64, Mac OS X 10.11 and Ubuntu 16.04 x64 on Firefox Nightly 54.0a1, Firefox Nightly 53.0a1 and Aurora 53.0a2.

Comment 26

[Hani Yacoub, Desktop QA]

According to the previous comments I logged this bug 1339848.