Closed Bug 1381401 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Settings UI, enhancement)

Product:
Firefox
Component:
Settings UI
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: prathiksha, Assigned: prathiksha, Mentored)

References

Details

Attachments

(1 file)

Bug 1381401 - Add location, camera and microphone permissions along with their settings under the 'Permissions' heading in Firefox Preferences.
59 bytes, text/x-review-board-request
johannh
: review+
Details
Assignee: nobody → prathikshaprasadsuman
Mentor: jhofmann
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.
Attachment #8894451 - Flags: review?(jhofmann) → review-
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 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?
Attachment #8894451 - Flags: review?(jhofmann) → review+
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?).
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.
Flags: needinfo?(prathikshaprasadsuman)
Status: NEW → ASSIGNED
Priority: -- → P1
Status: ASSIGNED → NEW
Flags: needinfo?(prathikshaprasadsuman)
Priority: P1 → --
Keywords: checkin-needed
Keywords: checkin-needed
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/75509cda693e
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
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!
Flags: needinfo?(prathikshaprasadsuman)
ok :)
Flags: needinfo?(prathikshaprasadsuman)
(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.
(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!
See Also: → 1390509
Thanks!
Depends on: 1390509, 1390049
See Also: 1390509 → 
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
QA Whiteboard: [bugday-20170816]
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]
As per Comment 17 and Comment 18, I am marking this bug as verified fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: