Fix access keys for location, camera and microphone permissions

RESOLVED FIXED in Firefox 57

Status

()

Firefox
Preferences
RESOLVED FIXED
4 months ago
4 months ago

People

(Reporter: Ton, Assigned: prathiksha)

Tracking

unspecified
Firefox 57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

4 months ago
See https://bugzilla.mozilla.org/show_bug.cgi?id=1381401#c15.

Bug 1381401 added permissions strings and keys, though the keys apply to the Settings… button, not the strings themselves.

They should probably be changed to S, e, t and n - their entities are probably fine. In this order, the n already seems to be fine too.
Blocks: 1381401
See Also: bug 1381401
We had quite a long discussion about this in #fx-team, but we didn't come to a better solution than what's proposed in comment 0. We should probably do that.
(Assignee)

Updated

4 months ago
Assignee: nobody → prathikshaprasadsuman
Status: NEW → ASSIGNED
Duplicate of this bug: 1391536
Comment hidden (mozreview-request)

Comment 4

4 months ago
mozreview-review
Comment on attachment 8898920 [details]
Bug 1390509 - Fix access keys for location, camera and microphone permissions.

https://reviewboard.mozilla.org/r/170280/#review175482

::: browser/locales/en-US/chrome/browser/preferences/content.dtd:27
(Diff revision 1)
> -<!ENTITY  cameraSettingsButton.accesskey                        "c">
> +<!ENTITY  cameraSettingsButton.accesskey                        "e">
>  
>  <!ENTITY  microphonePermissions.label                           "Microphone">
> -<!ENTITY  microphoneSettingsButton.accesskey                    "m">
> +<!ENTITY  microphoneSettingsButton.accesskey                    "t">

We should just make all of these "S". Trying to get a unique access key here isn't really possible given all the places that these buttons can appear (preferences search) and there are quite a lot of buttons on the page already.
Attachment #8898920 - Flags: review?(jaws) → review-
flod, do we need to rev the access keys here? Would we need to rev the entity name for the Settings label? It seems they all use the same entity ("permissionSettingsButton2.label") for the label (maybe that's another problem).
Flags: needinfo?(francesco.lodolo)
1. If you need to change access keys in English, there's no need to REV the string ID, just change them. Each locale has its own access keys.
2. Label and access key should follow the structure ID.label and ID.accesskey, if you change one ID, you should change the other too.

Having said that, using a string called microphoneSettingsButton.accesskey as an access key for a label permissionSettingsButton2.label doesn't make any sense. There should be one access key associated to that label, and no other accesskeys.
Flags: needinfo?(francesco.lodolo)
Thanks, that is my understanding as well. I just wanted to make sure.

Comment 8

4 months ago
mozreview-review
Comment on attachment 8898920 [details]
Bug 1390509 - Fix access keys for location, camera and microphone permissions.

https://reviewboard.mozilla.org/r/170280/#review175486

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

Please remove this string, and add a new one for locationSettingsButton.label, cameraSettingsButton.label, and microphoneSettingsButton.label.

They should all be equal to "Settings…", and should appear next to their associated accesskeys in this file.
Comment hidden (mozreview-request)
(Assignee)

Comment 10

4 months ago
I made the changes but 'S' isn't working as an accesskey. 't' works. How about we use 't' instead ?
Flags: needinfo?(jaws)

Comment 11

4 months ago
mozreview-review
Comment on attachment 8898920 [details]
Bug 1390509 - Fix access keys for location, camera and microphone permissions.

https://reviewboard.mozilla.org/r/170280/#review175898

Looks good, thanks! Please change this to 't' since 'S' isn't working.
Attachment #8898920 - Flags: review?(jaws) → review+
Flags: needinfo?(jaws)
Comment hidden (mozreview-request)
(Assignee)

Updated

4 months ago
Keywords: checkin-needed

Comment 13

4 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a486d2393755
Fix access keys for location, camera and microphone permissions. r=jaws
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a486d2393755
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.