Closed Bug 1316996 Opened 8 years ago Closed 7 years ago

Create initial text for webextensions permissions

Categories

(WebExtensions :: General, defect, P1)

Product:
WebExtensions
Component:
General
Version:
51 Branch
Type:
defect

Tracking

(firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed
Blocking Flags:
webextensions +

People

(Reporter: aswan, Assigned: aswan)

References

Details

(Whiteboard: triaged)

Attachments

(1 file)

Bug 1316996 Text for webextensions permissions
59 bytes, text/x-review-board-request
florian
: review+
Details 
Permissions that may appear in webextension manifests are documented here: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/permissions

We need a way to systematically convert the permissions from a webextension into text suitable for presenting to a user.
One thing to take in consideration when writing the text is too make sure it's not too scaremongering for users to become paranoid/afraid when it comes to add-ons.
Priority: -- → P1
Whiteboard: triaged
Here is a Permissions copy deck:

https://docs.google.com/document/d/1-aLncxcKpinCNpvksmknwWZhrM2bEChDmQ3C422C6Vc/edit

There are a lot of messaging touch points so it's possible I accidentally omitted something. Please just let me know if you need anything more, or if anything's unclear.
Assignee: sdevaney → aswan
Attachment #8826781 - Flags: review?(francesco.lodolo)
Comment on attachment 8826781 [details]
Bug 1316996 Text for webextensions permissions

https://reviewboard.mozilla.org/r/104662/#review105418

I'll be happy to take another look at strings after a first pass of fixes, but you should ask for review from a browser dev given the amount of code..

::: browser/locales/en-US/chrome/browser/browser.properties:41
(Diff revision 1)
> +# LOCALIZATION NOTE (webextPerms.header)
> +# This string is used as a header in the webextension permissions dialog,
> +# {ADDON} is replaced with the localized name of the extension being installed.
> +# See https://bug1308309.bmoattachments.org/attachment.cgi?id=8814612
> +# for an example of the full dialog.
> +webextPerms.header=Add {ADDON}?

Is there a reason to use str.replace here instead of getFormattedString? In any case, I would stick to a known format like %S, no point in introducing a risk for people to localize this.

::: browser/locales/en-US/chrome/browser/browser.properties:46
(Diff revision 1)
> +webextPerms.header=Add {ADDON}?
> +
> +# LOCALIZATION NOTE (webextPerms.listIntro)
> +# This string will be followed by a list of permissions requested
> +# by the webextension.
> +webextPerms.listIntro=It's able to:

You need to replace straight single/double quote with proper quotes, otherwise this will fail tests.

It’s able to:

::: browser/locales/en-US/chrome/browser/browser.properties:47
(Diff revision 1)
> +
> +# LOCALIZATION NOTE (webextPerms.listIntro)
> +# This string will be followed by a list of permissions requested
> +# by the webextension.
> +webextPerms.listIntro=It's able to:
> +webextPerms.accept.label=Add

What's the relation between string ID (accept) and "Add"?

::: browser/locales/en-US/chrome/browser/browser.properties:61
(Diff revision 1)
> +
> +# LOCALIZATION NOTE (webextPerms.sideloadHeader)
> +# This string is used as a header in the webextension permissions dialog
> +# when the extension is side-loaded.
> +# {ADDON} is replaced with the localized name of the extension being installed.
> +webextPerms.sideloadHeader={ADDON} added

Same note about {ADDON}

::: browser/locales/en-US/chrome/browser/browser.properties:62
(Diff revision 1)
> +# LOCALIZATION NOTE (webextPerms.sideloadHeader)
> +# This string is used as a header in the webextension permissions dialog
> +# when the extension is side-loaded.
> +# {ADDON} is replaced with the localized name of the extension being installed.
> +webextPerms.sideloadHeader={ADDON} added
> +webextPerms.sideloadText=Another program on your computer installed an add-on that may affect your browser.  Please review this add-on's permissions requests and choose to Enable or Disable.

Do we use double spaces after a period anywhere in Firefox? I've never seen it.

::: browser/locales/en-US/chrome/browser/browser.properties:80
(Diff revision 1)
> +# has been updated.
> +webextPerms.updateMenuItem=%1$S requires new permissions
> +
> +# LOCALIZATION NOTE (webextPerms.updateText)
> +# {ADDON} is replaced with the localized name of the updated extension.
> +webextPerms.updateText={ADDON} has been updated.  You must approve new permissions before the updated version will install.

Plaeholder style and double space.

::: browser/locales/en-US/chrome/browser/browser.properties:98
(Diff revision 1)
> +# %1$S will be replaced with the name of the application (e.g., Firefox)
> +webextPerms.description.nativeMessaging=Exchange messages with programs other than %1$S
> +webextPerms.description.notifications=Display notifications to you
> +webextPerms.description.sessions=Access browser history to restore tabs
> +webextPerms.description.tabs=Access browser tabs
> +webextPerms.description.topSites=Access browsing history

Two strings above it's "browser history", it doesn't seem consistent.

::: browser/locales/en-US/chrome/browser/browser.properties:103
(Diff revision 1)
> +webextPerms.description.topSites=Access browsing history
> +webextPerms.description.webNavigation=Access browser activity during navigation
> +webextPerms.description.webRequest=Access browser during Web activity
> +webextPerms.description.clipboardRead=Access text clipboard
> +
> +webextPerms.hostDescription.allUrls=Access your data for all web sites

Firefox seems to use "website" consistently, not "web site".

::: browser/locales/en-US/chrome/browser/browser.properties:113
(Diff revision 1)
> +webextPerms.hostDescription.wildcard=Access your data for sites in the %1$S domain
> +
> +# LOCALIZATION NOTE (webextPerms.hostDescription.tooManyWildcards)
> +# %1$S will be replaced by an integer (>= 2) indicating a number of domains
> +# for which this webextension is requesting permission.
> +webextPerms.hostDescription.tooManyWildcards=Access your data in %1$S other domains

Use a proper plural form, please
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Use_proper_plural_forms

::: browser/locales/en-US/chrome/browser/browser.properties:123
(Diff revision 1)
> +webextPerms.hostDescription.oneSite=Access your data for %1$S
> +
> +# LOCALIZATION NOTE (webextPerms.hostDescription.tooManySites)
> +# %1$S will be replaced by an integer (>= 2) indicating a number of hosts
> +# for which this webextension is requesting permission.
> +webextPerms.hostDescription.tooManySites=Access your data on %1$S other sites

Same here, proper plural form.
Comment on attachment 8826781 [details]
Bug 1316996 Text for webextensions permissions

https://reviewboard.mozilla.org/r/104662/#review105694

Clearing review flag per previous command. Note that we need to land this in m-c by the end of the week if you want to ship with 53 and have the strings localized.
Attachment #8826781 - Flags: review?(francesco.lodolo)
Comment on attachment 8826781 [details]
Bug 1316996 Text for webextensions permissions

https://reviewboard.mozilla.org/r/104662/#review105738

::: browser/locales/en-US/chrome/browser/browser.properties:46
(Diff revision 1)
> +webextPerms.header=Add {ADDON}?
> +
> +# LOCALIZATION NOTE (webextPerms.listIntro)
> +# This string will be followed by a list of permissions requested
> +# by the webextension.
> +webextPerms.listIntro=It's able to:

It's quite unusual to use contractions like "it's" in product. How about "It is able to" or "It can"?
Since below, you're using "It can", I would simply go with that.

::: browser/locales/en-US/chrome/browser/browser.properties:67
(Diff revision 1)
> +webextPerms.sideloadText=Another program on your computer installed an add-on that may affect your browser.  Please review this add-on's permissions requests and choose to Enable or Disable.
> +
> +# LOCALIZATION NOTE (webextPerms.sideloadListIntro)
> +# This string will be followed by a list of permissions requested
> +# by the webextension.
> +webextPerms.sideloadListIntro=It can:

This is inconsistent with the string above (webextPerms.listIntro)

::: browser/modules/ExtensionsUI.jsm:244
(Diff revision 1)
> -          doc.getElementById("addon-webext-perm-header").textContent = header;
> +          doc.getElementById("addon-webext-perm-header").innerHTML = header;
> +
> +          if (text) {
> +            doc.getElementById("addon-webext-perm-text").innerHTML = text;

It's safer to use textContent to avoid XSS attacks (if the name is <script>...</script> for example).
The security team says we should avoid it when we can.
I think getFormattedString as :flod suggested helps with what you're trying to achieve.
webextensions: --- → +
(In reply to Francesco Lodolo [:flod] from comment #5)
> Note that we need to land this in m-c by the end of the week 
> if you want to ship with 53 and have the strings localized.

Just making sure this doesn't go unseen.
Flags: needinfo?(aswan)
Right, I understand the deadline.
One question, the logic for the "on N other sites" message is written to only show that message when the value is 2 or greater.  That is, it will never say "on 1 other site".  What's the right way to write a plural form rule that doesn't have an entry for the value 1?
Flags: needinfo?(aswan) → needinfo?(francesco.lodolo)
Scott, questions about the copy in comment 4 and comment 6 above, specifically:
- browser history versus browsing history
- website versus web site
- Use of a contraction (It’s)
Flags: needinfo?(sdevaney)
(In reply to Andrew Swan [:aswan] from comment #8)
> Right, I understand the deadline.
> One question, the logic for the "on N other sites" message is written to
> only show that message when the value is 2 or greater.  That is, it will
> never say "on 1 other site".  What's the right way to write a plural form
> rule that doesn't have an entry for the value 1?

You could use ";and #1 other sites", but it's extremely confusing. 

It's much clearer to add the singular form, even if it's going to be unused in English "and #1 other site;and #1 other sites".
Flags: needinfo?(francesco.lodolo)
(In reply to Tim Nguyen :ntim from comment #6)
> ::: browser/locales/en-US/chrome/browser/browser.properties:67
> (Diff revision 1)
> > +webextPerms.sideloadText=Another program on your computer installed an add-on that may affect your browser.  Please review this add-on's permissions requests and choose to Enable or Disable.
> > +
> > +# LOCALIZATION NOTE (webextPerms.sideloadListIntro)
> > +# This string will be followed by a list of permissions requested
> > +# by the webextension.
> > +webextPerms.sideloadListIntro=It can:
> 
> This is inconsistent with the string above (webextPerms.listIntro)

Scott requested this explicitly, I don't remember the exact reasoning offhand.

> ::: browser/modules/ExtensionsUI.jsm:244
> (Diff revision 1)
> > -          doc.getElementById("addon-webext-perm-header").textContent = header;
> > +          doc.getElementById("addon-webext-perm-header").innerHTML = header;
> > +
> > +          if (text) {
> > +            doc.getElementById("addon-webext-perm-text").innerHTML = text;
> 
> It's safer to use textContent to avoid XSS attacks (if the name is
> <script>...</script> for example).
> The security team says we should avoid it when we can.
> I think getFormattedString as :flod suggested helps with what you're trying
> to achieve.

I've switched for getFormattedString but that is orthogonal to how the content is inserted.  innerHTML is only used where we have markup in the content being inserted...  Actually, :flod is there something we should put in the LOCALIZATION NOTE for strings that will end up containing markup to let translators know that they should not have markup characters in the translated strings?
(In reply to Andrew Swan [:aswan] from comment #11)
> Actually, :flod is there something we should put
> in the LOCALIZATION NOTE for strings that will end up containing markup to
> let translators know that they should not have markup characters in the
> translated strings?

Can you give me an example? Is the original string going to contain markup, or do you want to make sure localization doesn't introduce markup?

As for Tim's comment, I wonder if the XSS problem is more about the fact that text is not fully controlled by the application (e.g. the name of the add-on comes from the outside).
Attachment #8826781 - Flags: review?(florian)
Comment on attachment 8826781 [details]
Bug 1316996 Text for webextensions permissions

https://reviewboard.mozilla.org/r/104662/#review106568

::: browser/locales/en-US/chrome/browser/browser.properties:94
(Diff revisions 1 - 2)
>  webextPerms.description.bookmarks=Access bookmarks
>  webextPerms.description.downloads=Download files and read and modify the browser's download history
>  webextPerms.description.history=Access browser history
>  # LOCALIZATION NOTE (webextPerms.description.nativeMessaging)
> -# %1$S will be replaced with the name of the application (e.g., Firefox)
> -webextPerms.description.nativeMessaging=Exchange messages with programs other than %1$S
> +# {APPLICATION} will be replaced with the name of the application
> +1webextPerms.description.nativeMessaging=Exchange messages with programs other than {APPLICATION}

Not sure why this switched to {VAR}, but it should use %1$S like other strings.

::: browser/locales/en-US/chrome/browser/browser.properties:111
(Diff revisions 1 - 2)
>  # %1$S will be replaced by the DNS domain for which a webextension
>  # is requesting access (e.g., mozilla.org)
>  webextPerms.hostDescription.wildcard=Access your data for sites in the %1$S domain
>  
>  # LOCALIZATION NOTE (webextPerms.hostDescription.tooManyWildcards)
> -# %1$S will be replaced by an integer (>= 2) indicating a number of domains
> +# Semicolon separated list of plural forms.  

You need the full comment, link included (weird, but it's used by parsers to identify plural forms).

# LOCALIZATION NOTE (webextPerms.hostDescription.tooManyWildcards): 
# Semi-colon list of plural forms.
# See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
# #1 will be replaced by an integer a number of domains
# for which this webextension is requesting permission.

::: browser/locales/en-US/chrome/browser/browser.properties:122
(Diff revisions 1 - 2)
>  # %1$S will be replaced by the DNS host name for which a webextension
>  # is requesting access (e.g., www.mozilla.org)
>  webextPerms.hostDescription.oneSite=Access your data for %1$S
>  
>  # LOCALIZATION NOTE (webextPerms.hostDescription.tooManySites)
> -# %1$S will be replaced by an integer (>= 2) indicating a number of hosts
> +# Semicolon separated list of plural forms.  

Same here, need the full comment.
Attachment #8826781 - Flags: review?(francesco.lodolo)
Comment on attachment 8826781 [details]
Bug 1316996 Text for webextensions permissions

https://reviewboard.mozilla.org/r/104662/#review106572

::: browser/locales/en-US/chrome/browser/browser.properties:112
(Diff revision 2)
> +# is requesting access (e.g., mozilla.org)
> +webextPerms.hostDescription.wildcard=Access your data for sites in the %1$S domain
> +
> +# LOCALIZATION NOTE (webextPerms.hostDescription.tooManyWildcards)
> +# Semicolon separated list of plural forms.  
> +# #1 will be replaced by an integer a number of domains

(bad copy and paste): #1 will be replaced by the number of domains (or "by an integer indicating a number of domains" for consistency with the following one).
Comment on attachment 8826781 [details]
Bug 1316996 Text for webextensions permissions

https://reviewboard.mozilla.org/r/104662/#review106686

::: browser/base/content/browser-addons.js:506
(Diff revision 2)
>  
>      while (container.firstChild) {
>        container.firstChild.remove();
>      }
>  
>      // Strings below to be properly localized in bug 1316996

remove this comment

::: browser/locales/en-US/chrome/browser/browser.properties:41
(Diff revision 2)
> +# LOCALIZATION NOTE (webextPerms.header)
> +# This string is used as a header in the webextension permissions dialog,
> +# %1$S is replaced with the localized name of the extension being installed.
> +# See https://bug1308309.bmoattachments.org/attachment.cgi?id=8814612
> +# for an example of the full dialog.
> +webextPerms.header=Add %1$S?

%1$S can be simplified to %S whenever's only one parameter in a formatted string.

::: browser/locales/en-US/chrome/browser/browser.properties:46
(Diff revision 2)
> +webextPerms.header=Add %1$S?
> +
> +# LOCALIZATION NOTE (webextPerms.listIntro)
> +# This string will be followed by a list of permissions requested
> +# by the webextension.
> +webextPerms.listIntro=It’s able to:

I agree with comment 6 about the contraction being uncommon in our strings, and about the inconsistency with the other string ("It can").

::: browser/locales/en-US/chrome/browser/browser.properties:62
(Diff revision 2)
> +# LOCALIZATION NOTE (webextPerms.sideloadHeader)
> +# This string is used as a header in the webextension permissions dialog
> +# when the extension is side-loaded.
> +# %1$S is replaced with the localized name of the extension being installed.
> +webextPerms.sideloadHeader=%1$S added
> +webextPerms.sideloadText=Another program on your computer installed an add-on that may affect your browser. Please review this add-on's permissions requests and choose to Enable or Disable.

' -> ’

::: browser/locales/en-US/chrome/browser/browser.properties:67
(Diff revision 2)
> +webextPerms.sideloadText=Another program on your computer installed an add-on that may affect your browser. Please review this add-on's permissions requests and choose to Enable or Disable.
> +
> +# LOCALIZATION NOTE (webextPerms.sideloadListIntro)
> +# This string will be followed by a list of permissions requested
> +# by the webextension.
> +webextPerms.sideloadListIntro=It can:

It's not clear to me why this isn't just reusing the webextPerms.listIntro string. Same comment for webextPerms.updateListIntro

::: browser/locales/en-US/chrome/browser/browser.properties:89
(Diff revision 2)
> +# by the webextension.
> +webextPerms.updateListIntro=It can:
> +webextPerms.updateAccept.label=Update
> +webextPerms.updateAccept.accessKey=U
> +
> +webextPerms.description.bookmarks=Access bookmarks

What does 'Access' mean here? Is it a read only or a read/write access?

::: browser/locales/en-US/chrome/browser/browser.properties:90
(Diff revision 2)
> +webextPerms.updateListIntro=It can:
> +webextPerms.updateAccept.label=Update
> +webextPerms.updateAccept.accessKey=U
> +
> +webextPerms.description.bookmarks=Access bookmarks
> +webextPerms.description.downloads=Download files and read and modify the browser's download history

I'm not a native English speaker, but the double 'and' feels awkward to me here.

::: browser/locales/en-US/chrome/browser/browser.properties:94
(Diff revision 2)
> +webextPerms.description.bookmarks=Access bookmarks
> +webextPerms.description.downloads=Download files and read and modify the browser's download history
> +webextPerms.description.history=Access browser history
> +# LOCALIZATION NOTE (webextPerms.description.nativeMessaging)
> +# {APPLICATION} will be replaced with the name of the application
> +1webextPerms.description.nativeMessaging=Exchange messages with programs other than {APPLICATION}

- the '1' prefix of this string seems to be an accident.
- like flod said, this should just use %S

::: browser/locales/en-US/chrome/browser/browser.properties:96
(Diff revision 2)
> +webextPerms.description.history=Access browser history
> +# LOCALIZATION NOTE (webextPerms.description.nativeMessaging)
> +# {APPLICATION} will be replaced with the name of the application
> +1webextPerms.description.nativeMessaging=Exchange messages with programs other than {APPLICATION}
> +webextPerms.description.notifications=Display notifications to you
> +webextPerms.description.sessions=Access browser history to restore tabs

Maybe "Access recently closed tabs"?

::: browser/locales/en-US/chrome/browser/browser.properties:100
(Diff revision 2)
> +webextPerms.description.notifications=Display notifications to you
> +webextPerms.description.sessions=Access browser history to restore tabs
> +webextPerms.description.tabs=Access browser tabs
> +webextPerms.description.topSites=Access browsing history
> +webextPerms.description.webNavigation=Access browser activity during navigation
> +webextPerms.description.webRequest=Access browser during Web activity

I don't understand the meaning of this string.

::: browser/locales/en-US/chrome/browser/browser.properties:101
(Diff revision 2)
> +webextPerms.description.sessions=Access browser history to restore tabs
> +webextPerms.description.tabs=Access browser tabs
> +webextPerms.description.topSites=Access browsing history
> +webextPerms.description.webNavigation=Access browser activity during navigation
> +webextPerms.description.webRequest=Access browser during Web activity
> +webextPerms.description.clipboardRead=Access text clipboard

Is this restricted to the plain text data format from the clipboard? Will the add-on be able to access images that are in the clipboard?

::: browser/locales/en-US/chrome/browser/browser.properties:103
(Diff revision 2)
> +webextPerms.description.topSites=Access browsing history
> +webextPerms.description.webNavigation=Access browser activity during navigation
> +webextPerms.description.webRequest=Access browser during Web activity
> +webextPerms.description.clipboardRead=Access text clipboard
> +
> +webextPerms.hostDescription.allUrls=Access your data for all web sites

I don't understand this string. What does 'access your data' mean? Which kind of data?

::: browser/locales/en-US/chrome/browser/browser.properties:111
(Diff revision 2)
> +# %1$S will be replaced by the DNS domain for which a webextension
> +# is requesting access (e.g., mozilla.org)
> +webextPerms.hostDescription.wildcard=Access your data for sites in the %1$S domain
> +
> +# LOCALIZATION NOTE (webextPerms.hostDescription.tooManyWildcards)
> +# Semicolon separated list of plural forms.  

trailing whitespace

::: browser/locales/en-US/chrome/browser/browser.properties:122
(Diff revision 2)
> +# %1$S will be replaced by the DNS host name for which a webextension
> +# is requesting access (e.g., www.mozilla.org)
> +webextPerms.hostDescription.oneSite=Access your data for %1$S
> +
> +# LOCALIZATION NOTE (webextPerms.hostDescription.tooManySites)
> +# Semicolon separated list of plural forms.  

trailing whitespace

::: browser/modules/ExtensionsUI.jsm:156
(Diff revision 2)
>      let win = target.ownerGlobal;
>  
>      let name = info.addon.name;
>      if (name.length > 50) {
>        name = name.slice(0, 49) + "…";
>      }

Escape <, > and & characters from 'name' here?

Do we want to trust the localizers to not include any of these characters in the strings they translate (if so, maybe we should mention it in localization notes?), or do we want to escape these strings to be safe? If the latter, we can't use getFormattedString, and will need to use .replace("#1", addonLabel) instead.

::: browser/modules/ExtensionsUI.jsm:192
(Diff revision 2)
>  
> -    let formatPermission = perm => {
> +    let msgs = [];
> +    for (let permission of perms.permissions) {
>        try {
> -        // return bundle.getString(`webextPerms.description.${perm}`);
> -        return `localized description of permission ${perm}`;
> +        let string = bundle.getString(`webextPerms.description.${permission}`);
> +        if (string.includes("{APPLICATION}")) {

Is there a way we could use getFormattedString instead of this custom replacement?

::: browser/modules/ExtensionsUI.jsm:226
(Diff revision 2)
> -      return `localized description of single host permission for ${match[1]}`;
> -    };
> +      msgs.push(bundle.getString("webextPerms.hostDescription.allUrls"));
> +    } else {
> +      // Formats a list of host permissions.  If we have 4 or fewer, display
> +      // them all, otherwise display the first 3 followed by an item that
> +      // says "...plus N others"
> +      function format(list, key, key2) {

Please replace 'key2' with a more explicit name, eg. keyNMore

::: browser/modules/ExtensionsUI.jsm:228
(Diff revision 2)
> +      // Formats a list of host permissions.  If we have 4 or fewer, display
> +      // them all, otherwise display the first 3 followed by an item that
> +      // says "...plus N others"
> +      function format(list, key, key2) {
> +        if (list.length < 5) {
> +          msgs.push(...list.map(item => bundle.getFormattedString(key, [item])));

You can reduce code duplication with another helper function:

function formatItems(list) {
  msgs.push(...list.map(item => bundle.getFormattedString(key, [item])))
}

::: browser/modules/ExtensionsUI.jsm:233
(Diff revision 2)
> +          msgs.push(...list.map(item => bundle.getFormattedString(key, [item])));
> +        } else {
> +          msgs.push(...list.slice(0, 3).map(item => bundle.getFormattedString(key, [item])));
> +          let remaining = list.length - 3;
> +          let msg = PluralForm.get(remaining, bundle.getString(key2))
> +            .replace("#1", remaining);

- You don't need the msg variable and can inline:
msg.push(PluralForm.get...)

- .replace should be aligned with .get

- the 'remaining' variable doesn't seem very useful either.
Attachment #8826781 - Flags: review?(florian) → review-
Comment on attachment 8826781 [details]
Bug 1316996 Text for webextensions permissions

https://reviewboard.mozilla.org/r/104662/#review106766

::: browser/locales/en-US/chrome/browser/browser.properties:46
(Diff revision 2)
> +webextPerms.header=Add %1$S?
> +
> +# LOCALIZATION NOTE (webextPerms.listIntro)
> +# This string will be followed by a list of permissions requested
> +# by the webextension.
> +webextPerms.listIntro=It’s able to:

Talked to Scott about this, this is part of the overall push to use a more casual conversational tone in in-product strings, he would like to keep this.
Comment on attachment 8826781 [details]
Bug 1316996 Text for webextensions permissions

https://reviewboard.mozilla.org/r/104662/#review105418

> What's the relation between string ID (accept) and "Add"?

This reflects an older string that got changed from accept to add, I'll change it to add.
Comment on attachment 8826781 [details]
Bug 1316996 Text for webextensions permissions

https://reviewboard.mozilla.org/r/104662/#review105738

> It's quite unusual to use contractions like "it's" in product. How about "It is able to" or "It can"?
> Since below, you're using "It can", I would simply go with that.

Discussed this with Scott and this is part of the efforts to use a more casual conversational style in these strings, he would like to keep this as is.

> This is inconsistent with the string above (webextPerms.listIntro)

This was also deliberate based on the surrounding text in the corresponding dialogs.
Quick comment on the case where we use "It's able to"... that was chosen in one particular flow because the copy preceding it was: "Would you like to add [name of add-on] to Firefox?"

In this case, when the above message was followed by "It can" we felt it could be misconstrued as a setup to detail the functionality and features of the add-on (as opposed to its permissions). So, in this case we went with "It's able to" to better distinguish approach a list of permissions.
Flags: needinfo?(sdevaney)
Comment on attachment 8826781 [details]
Bug 1316996 Text for webextensions permissions

https://reviewboard.mozilla.org/r/104662/#review106686

> I'm not a native English speaker, but the double 'and' feels awkward to me here.

Hmph, I could swear I replied to this before but I seem to have managed to lose it in reviewboard.
Proxying Scott's repsonse again, he agreed this sounds a little awkward but it doesn't break any grammar rules and we couldn't come up with anything else that isn't just as unwieldy.
Comment on attachment 8826781 [details]
Bug 1316996 Text for webextensions permissions

https://reviewboard.mozilla.org/r/104662/#review106686

> I don't understand this string. What does 'access your data' mean? Which kind of data?

Arg, another comment lost in the reviewboard ether.
My paraphrasing of the conclusion of discussing with Scott is this is probably the single hardest item to write a message for since it covers so much (including webRequest and content scripts as prominent examples but also a number of other things).  There's a tension between providing a description that is too vague versus one that is so detailed and technical that many users can't understand it (which would undermine the usefulness of permissions overall).  Given that tradeoff, it was a simple choice for the first option...
(In reply to sdevaney from comment #20)
> Quick comment on the case where we use "It's able to"... that was chosen in
> one particular flow because the copy preceding it was: "Would you like to
> add [name of add-on] to Firefox?"
> 
> In this case, when the above message was followed by "It can" we felt it
> could be misconstrued as a setup to detail the functionality and features of
> the add-on (as opposed to its permissions). So, in this case we went with
> "It's able to" to better distinguish approach a list of permissions.

I still don't see a meaningful difference between "It's able to" and "It can". They both have exactly the problem you describe: it seems to be a list of features rather than a permission request. What about being explicit and using "It needs your permission to:"?
Flags: needinfo?(sdevaney)
(In reply to Florian Quèze [:florian] [:flo] from comment #23)

> I still don't see a meaningful difference between "It's able to" and "It
> can". They both have exactly the problem you describe: it seems to be a list
> of features rather than a permission request. What about being explicit and
> using "It needs your permission to:"?

I'm okay with that, but could we swap "needs" for "requires"?:

"I requires your permission to:"
Flags: needinfo?(sdevaney)
Comment on attachment 8826781 [details]
Bug 1316996 Text for webextensions permissions

https://reviewboard.mozilla.org/r/104662/#review106858

::: browser/locales/en-US/chrome/browser/browser.properties:91
(Diff revisions 2 - 3)
> -webextPerms.description.sessions=Access browser history to restore tabs
> +webextPerms.description.sessions=Access browser recently closed tabs
>  webextPerms.description.tabs=Access browser tabs
>  webextPerms.description.topSites=Access browsing history
>  webextPerms.description.webNavigation=Access browser activity during navigation

I do find "browser" a bit redundant here. Maybe "browser" could be replaced by "your". I do find it more conversational that way.
Comment on attachment 8826781 [details]
Bug 1316996 Text for webextensions permissions

https://reviewboard.mozilla.org/r/104662/#review106860

r=me with the following comments addressed (I didn't write my comments about the content of showInstallNotification, so if this was intentionally included, please re-request review).

::: browser/locales/en-US/chrome/browser/browser.properties:47
(Diff revisions 2 - 3)
> +webextPerms.header=Add %S?
>  
>  # LOCALIZATION NOTE (webextPerms.listIntro)
>  # This string will be followed by a list of permissions requested
>  # by the webextension.
> -webextPerms.listIntro=It’s able to:
> +webextPerms.listIntro=It needs your permission to:

This string is fine with me, but Scott wanted to replace 'needs' with 'requires' in commen 24.

::: browser/modules/ExtensionsUI.jsm:161
(Diff revisions 2 - 3)
>      if (name.length > 50) {
>        name = name.slice(0, 49) + "…";
>      }
> +    name = name.replace(/</g, "&lt;")
> +               .replace(/>/g, "&gt;")
> +               .replace(/&/g, "&amp;");

Replace & first, or < will become &amp;lt;

::: browser/modules/ExtensionsUI.jsm:196
(Diff revisions 2 - 3)
>  
>      let msgs = [];
>      for (let permission of perms.permissions) {
> +      let key = `webextPerms.description.${permission}`;
> +      if (permission == "nativeMessaging") {
> +        msgs.push(bundle.getFormattedString(key, [appName]));

This is the only use of appName, so move the brandBundle and appName declarations into this if block.

::: browser/modules/ExtensionsUI.jsm:242
(Diff revisions 2 - 3)
> -          msgs.push(...list.map(item => bundle.getFormattedString(key, [item])));
> +          formatItems(list);
>          } else {
> -          msgs.push(...list.slice(0, 3).map(item => bundle.getFormattedString(key, [item])));
> +          formatItems(list.slice(0, 3));
> +
>            let remaining = list.length - 3;
> -          let msg = PluralForm.get(remaining, bundle.getString(key2))
> +          msgs.push(PluralForm.get(remaining, bundle.getString(key2))

key2 -> moreKey

::: browser/modules/ExtensionsUI.jsm:316
(Diff revisions 2 - 3)
>                                      },
>                                    ], popupOptions);
>      });
>    },
> +
> +  showInstallNotification(target, addon) {

Has this block been intentionally included here? It doesn't seem related, and doesn't seem finished either.
Attachment #8826781 - Flags: review?(florian) → review+
Comment on attachment 8826781 [details]
Bug 1316996 Text for webextensions permissions

https://reviewboard.mozilla.org/r/104662/#review106860

> Has this block been intentionally included here? It doesn't seem related, and doesn't seem finished either.

Arg, rebase error, this wasn't meant to be in this patch
Comment on attachment 8826781 [details]
Bug 1316996 Text for webextensions permissions

https://reviewboard.mozilla.org/r/104662/#review106860

> key2 -> moreKey

*facepalm*  We've got a lot more eslint rules turned on in toolkit/ including no-unused-var, I think I've gotten too reliant on it!
Comment on attachment 8826781 [details]
Bug 1316996 Text for webextensions permissions

https://reviewboard.mozilla.org/r/104662/#review106858

> I do find "browser" a bit redundant here. Maybe "browser" could be replaced by "your". I do find it more conversational that way.

Proxying Scott again: "Your" was omitted primarily because then it would look weird not to include it in all similar cases, resulting in "your" repeated throughout a list of points.  Further, it's simply extraneous copy in a place where we're trying to keep it streamlined.
Comment on attachment 8826781 [details]
Bug 1316996 Text for webextensions permissions

https://reviewboard.mozilla.org/r/104662/#review106932

::: browser/locales/en-US/chrome/browser/browser.properties:79
(Diff revision 5)
> +webextPerms.updateMenuItem=%S requires new permissions
> +
> +# LOCALIZATION NOTE (webextPerms.updateText)
> +# %S is replaced with the localized name of the updated extension.
> +# Note, this string will be used as raw markup. Avoid characters like <, >, &
> +webextPerms.updateText=%S has been updated. You must approve new permissions before the updated version will install.  Choosing “Cancel” will maintain your current add-on version.

There's still a double space after the period here.

::: browser/locales/en-US/chrome/browser/browser.properties:106
(Diff revision 5)
> +webextPerms.hostDescription.wildcard=Access your data for sites in the %S domain
> +
> +# LOCALIZATION NOTE (webextPerms.hostDescription.tooManyWildcards):
> +# Semi-colon list of plural forms.
> +# See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
> +# #1 will be replaced by an integer a number of domains

Still bad English in comment.

::: browser/locales/en-US/chrome/browser/browser.properties:118
(Diff revision 5)
> +webextPerms.hostDescription.oneSite=Access your data for %S
> +
> +# LOCALIZATION NOTE (webextPerms.hostDescription.tooManySites)
> +# Semi-colon list of plural forms.
> +# See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
> +# #1 will be replaced by an integer a number of hosts

Bad English now moved to this one too.
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/772c8d7a210c
Text for webextensions permissions r=florian
https://hg.mozilla.org/mozilla-central/rev/772c8d7a210c
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
https://hg.mozilla.org/mozilla-central/diff/772c8d7a210c/browser/locales/en-US/chrome/browser/browser.properties
> +# LOCALIZATION NOTE (webextPerms.header)
> +# This string is used as a header in the webextension permissions dialog,
> +# %S is replaced with the localized name of the extension being installed.
> +# See https://bug1308309.bmoattachments.org/attachment.cgi?id=8814612
> +# for an example of the full dialog.
> +# Note, this string will be used as raw markup. Avoid characters like <, >, &
> +webextPerms.header=Add %S?

Why webextPerms.header string is looking completely different than on https://bug1308309.bmoattachments.org/attachment.cgi?id=8814612 screenshot (tested on nightly with instructions from bug 1334404) - forced styling and line breaks are added pushing the sentence into three lines and making it incorrect and unacceptable?
(In reply to Stefan Plewako [:stef] from comment #37)

> Why webextPerms.header string is looking completely different than on
> https://bug1308309.bmoattachments.org/attachment.cgi?id=8814612 screenshot
> (tested on nightly with instructions from bug 1334404) - forced styling and
> line breaks are added pushing the sentence into three lines and making it
> incorrect and unacceptable?

I think you are seeing bug 1333168, which we are working on fixing.
(In reply to Florian Quèze [:florian] [:flo] from comment #38)
> I think you are seeing bug 1333168, which we are working on fixing.

Yes, it seems so, didn't found it dependent bugs, thx.
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: