1381401 - Add location, camera and microphone permissions along with their settings under the 'Permissions' heading in Firefox Preferences.

Status: VERIFIED FIXED

(Firefox :: Settings UI, enhancement)

Description

[:prathiksha]

https://mozilla.invisionapp.com/share/4ZB63CHD9#/screens/237173272

Comment 1

[:prathiksha]

Attachment: Bug 1381401 - Add location, camera and microphone permissions along with their settings under the 'Permissions' heading in Firefox Preferences.

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

Comment 2

[Johann Hofmann [:johannh]]

Comment on attachment 8894451 [details]
Bug 1381401 - Add location, camera and microphone permissions along with their settings under the 'Permissions' heading in Firefox Preferences.

https://reviewboard.mozilla.org/r/165634/#review170708

A couple of things to fix:

::: browser/locales/en-US/chrome/browser/preferences/content.dtd:11
(Diff revision 1)
>  
>  <!ENTITY  blockPopups.label           "Block pop-up windows">
>  <!ENTITY  blockPopups.accesskey       "B">
>  
>  <!ENTITY  notificationsPolicyLearnMore.label   "Learn more">
> -<!ENTITY  notificationsPolicyDesc4.label       "Choose which websites are allowed to send you notifications">
> +<!ENTITY  notificationsPolicyDesc4.label       "Notifications">

You need to update the string id when you update strings. Maybe you should change this to match the other labels below.

::: browser/locales/en-US/chrome/browser/preferences/content.dtd:21
(Diff revision 1)
>  <!ENTITY  notificationsDoNotDisturbDetails.value "No notification will be shown until you restart &brandShortName;">
>  
>  <!ENTITY  popupExceptions.label       "Exceptions…">
>  <!ENTITY  popupExceptions.accesskey   "E">
>  
> +<!ENTITY  settingsButton.label                    "Settings">

Maybe call this permissionSettingsButton.label?

::: browser/locales/en-US/chrome/browser/preferences/content.dtd:23
(Diff revision 1)
>  <!ENTITY  popupExceptions.label       "Exceptions…">
>  <!ENTITY  popupExceptions.accesskey   "E">
>  
> +<!ENTITY  settingsButton.label                    "Settings">
> +
> +<!ENTITY  location.label                          "Location">

I wonder if we should give these a little more specific names, such as "locationPermissionsDesc.label". What do you think?

::: browser/themes/shared/incontentprefs/privacy.css:8
(Diff revision 1)
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +.permission-icon {
> +  max-height: 20px;
> +  max-width: 20px;
> +  vertical-align: middle;

This needs to use

-moz-context-properties: fill;
fill: currentColor;

to make the icons have the text color.

Comment 3

[:prathiksha]

Comment on attachment 8894451 [details]
Bug 1381401 - Add location, camera and microphone permissions along with their settings under the 'Permissions' heading in Firefox Preferences.

https://reviewboard.mozilla.org/r/165634/#review170708

> I wonder if we should give these a little more specific names, such as "locationPermissionsDesc.label". What do you think?

That sounds good but I think we should make it "locationPermission.label" since it's more of a label than a description like notification label once was. :)

Comment 4

[:prathiksha]

Comment on attachment 8894451 [details]
Bug 1381401 - Add location, camera and microphone permissions along with their settings under the 'Permissions' heading in Firefox Preferences.

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

Comment 5

[Johann Hofmann [:johannh]]

Comment on attachment 8894451 [details]
Bug 1381401 - Add location, camera and microphone permissions along with their settings under the 'Permissions' heading in Firefox Preferences.

https://reviewboard.mozilla.org/r/165634/#review171118

r=me with the nits addressed. Thanks!

::: browser/locales/en-US/chrome/browser/preferences/content.dtd:23
(Diff revision 2)
>  <!ENTITY  popupExceptions.label       "Exceptions…">
>  <!ENTITY  popupExceptions.accesskey   "E">
>  
> +<!ENTITY  permissionSettingsButton.label          "Settings">
> +
> +<!ENTITY  locationPermission.label                          "Location">

Nit: the indentation/whitespace for these strings looks a little off to me (in the reviewboard UI). Can you make it consistent?

Comment 6

[Johann Hofmann [:johannh]]

Comment on attachment 8894451 [details]
Bug 1381401 - Add location, camera and microphone permissions along with their settings under the 'Permissions' heading in Firefox Preferences.

https://reviewboard.mozilla.org/r/165634/#review170708

> That sounds good but I think we should make it "locationPermission.label" since it's more of a label than a description like notification label once was. :)

Well, it's a label anyway, but I'm good with that name. Can we make it locationPermissions.label (plural?).

Comment 7

[Johann Hofmann [:johannh]]

Oh, one thing I forgot:

You will need to add your strings as search keywords, like in https://searchfox.org/mozilla-central/rev/bd39b6170f04afeefc751a23bb04e18bbd10352b/browser/components/preferences/in-content-new/privacy.js#270

Can you then please add tests for searching inside your sub-dialogs to https://searchfox.org/mozilla-central/source/browser/components/preferences/in-content-new/tests/browser_search_subdialogs_within_preferences_4.js?

You can either do that in another commit or re-request review. 

Thanks.

Comment 8

[:prathiksha]

Comment on attachment 8894451 [details]
Bug 1381401 - Add location, camera and microphone permissions along with their settings under the 'Permissions' heading in Firefox Preferences.

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

Comment 9

[:prathiksha]

Comment on attachment 8894451 [details]
Bug 1381401 - Add location, camera and microphone permissions along with their settings under the 'Permissions' heading in Firefox Preferences.

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/75509cda693e
Add location, camera and microphone permissions along with their settings under the 'Permissions' heading in Firefox Preferences. r=johannh

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/75509cda693e

Comment 12

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8894451 [details]
Bug 1381401 - Add location, camera and microphone permissions along with their settings under the 'Permissions' heading in Firefox Preferences.

https://reviewboard.mozilla.org/r/165634/#review173298

::: browser/locales/en-US/chrome/browser/preferences/content.dtd:17
(Diff revision 4)
>  <!ENTITY  notificationsDoNotDisturbDetails.value "No notification will be shown until you restart &brandShortName;">
>  
> -<!ENTITY  popupExceptions.label       "Exceptions…">
> -<!ENTITY  popupExceptions.accesskey   "E">
> +<!ENTITY  popupExceptions.label                                 "Exceptions…">
> +<!ENTITY  popupExceptions.accesskey                             "E">
> +
> +<!ENTITY  permissionSettingsButton.label                        "Settings">

According to Photon preferences reorg v2 spec (https://mozilla.invisionapp.com/share/P4ACQT1E3#/screens/217167559), for those buttons are able to open a sub-dialog that should be added an extra ellipsis after the text. For this case, "Settings" will be "Settings…".

prathiksha, would you do me a favor to file a bug for fixing the "Settings…" button string?

BTW, please set one of those people (Jared (:jaws), Mike (:mconley), Evan (:evanxd), Ricky (:rickychien)) as second reviewer when doing changes in about:preferences?

Thank you very much!

Comment 13

[:prathiksha]

ok :)

Comment 14

[Ton [:Tonnes]]

(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #11)
> https://hg.mozilla.org/mozilla-central/rev/75509cda693e
> https://hg.mozilla.org/mozilla-central/diff/75509cda693e/browser/locales/en-US/chrome/browser/preferences/content.dtd
> 
> +<!ENTITY  notificationPermissions.label                         "Notifications">
> +<!ENTITY  notificationSettingsButton.accesskey                  "n">
> .
> .
> +<!ENTITY  microphonePermissions.label                           "Microphone">
> +<!ENTITY  microphoneSettingsButton.accesskey                    "m">

No complaints about these keys currently applying to the Settings… buttons?

Also, please keep access keys case sensitive to avoid (future) issues - the Notifications string is a nice example.

Comment 15

[Johann Hofmann [:johannh]]

(In reply to Ton from comment #14)
> (In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on
> intermittent or backout) from comment #11)
> > https://hg.mozilla.org/mozilla-central/rev/75509cda693e
> > https://hg.mozilla.org/mozilla-central/diff/75509cda693e/browser/locales/en-US/chrome/browser/preferences/content.dtd
> > 
> > +<!ENTITY  notificationPermissions.label                         "Notifications">
> > +<!ENTITY  notificationSettingsButton.accesskey                  "n">
> > .
> > .
> > +<!ENTITY  microphonePermissions.label                           "Microphone">
> > +<!ENTITY  microphoneSettingsButton.accesskey                    "m">
> 
> No complaints about these keys currently applying to the Settings… buttons?
> 
> Also, please keep access keys case sensitive to avoid (future) issues - the
> Notifications string is a nice example.

Good catch. Please file a bug. Thanks!

Comment 16

[Johann Hofmann [:johannh]]

Thanks!

Comment 17

[Azmina]

I have seen the functions Location, Camera, Microphone has been added under 'Permissions' heading in Firefox Preferences with Latest Nightly 57.0a1 on Windows 8.1, 64 bit.

Build ID : 20170817100132
User Agent : Mozilla/5.0 (Windows NT 6.3; WOW64; rv:57.0) Gecko/20100101 Firefox/57.0

Comment 18

[Md. Majedul islam]

I have seen the functions Location, Camera, Microphone has been added under 'Permissions' heading in Firefox Preferences with Latest Nightly 57.0a1 on ubuntu 16.04, 64 bit.

Build ID :  20170817100132
User Agent : Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0

[bugday-20170816]

Comment 19

[Azmina]

As per Comment 17 and Comment 18, I am marking this bug as verified fixed.