Current permission state should use the past tense

VERIFIED FIXED in Firefox 53

Status

()

P2
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: dao, Assigned: johannh)

Tracking

Trunk
Firefox 53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 verified, firefox54 verified)

Details

(Whiteboard: [fxprivacy][strings])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
From https://news.ycombinator.com/item?id=13394153:

> One suggestion: In the Control Center™, I would recommend using the past-tense
> for the current state. E.g.,
> 
>     Receive Notifications           Allowed X
>     Access Your Location            Allowed X
>     Maintain Offline Storage        Allowed X
> 
> As it exists in the screenshots, the present tense is used, and the X button
> seems to be associated with the word "Allow." Further clarification could be
> achieved by making the X button actually say "Disallow" and giving it a border
> separate from the word "Allowed." E.g.,
> 
>     Receive Notifications      Allowed   [Disallow]

And https://news.ycombinator.com/item?id=13392298:

> The status "Use the Camera - Allow - X" can be confusing. Is the site currently
> allowed to use the camera, or not? The word Allow could either mean "currently
> allowed" or "click to allow." The X could mean either "currently blocked" or
> "click to block."
Comment hidden (mozreview-request)
(Reporter)

Updated

2 years ago
Assignee: nobody → dao+bmo
(Reporter)

Comment 2

2 years ago
I also suspect "Allowed for Session" and "Allowed Temporarily" shouldn't use title capitalization in this context...

Comment 3

2 years ago
Michelle, in general saying "Allowed" instead of "Allow" in the Control Center seems reasonable.

Would you have any objection?
Flags: needinfo?(mheubusch)
Whiteboard: [fxprivacy][triage][strings]

Comment 4

2 years ago
Comment on attachment 8826836 [details]
Bug 1331172 - Current permission state should use the past tense.

We should land bug 1206232 first, and assuming we want to go ahead with this change, you should re-request review once that bug is fixed.
Attachment #8826836 - Flags: review?(jhofmann)
(Reporter)

Comment 5

2 years ago
Comment on attachment 8826836 [details]
Bug 1331172 - Current permission state should use the past tense.

Bug 1206232 is assigned to Johann. He's capable of postponing this request (or processing it and updating his own patch!) as needed. No need to cancel the request and risk forgetting about this bug.
Attachment #8826836 - Flags: review?(jhofmann)

Comment 6

2 years ago
As far as priority goes, the "Allow" string is already part of the Control Center redesign that landed earlier, and isn't new in the Permissions Notifications redesign we're doing for Firefox 53.

We're following the current convention for two new strings we're adding in bug 1206232. We'll need to change those strings in this bug. Ideally we would do this right after the other one lands, and this will save localizers a couple of strings to translate, but it's not that bad even if this bug misses Firefox 53.
status-firefox53: affected → fix-optional
Depends on: 1319112
Priority: -- → P2

Comment 7

2 years ago
(In reply to Dão Gottwald [:dao] from comment #5)
> Bug 1206232 is assigned to Johann. He's capable of postponing this request
> (or processing it and updating his own patch!) as needed. No need to cancel
> the request and risk forgetting about this bug.

Fine. While you was writing your comment, I was setting a bunch of other flags to track this bug, but I don't mind what the review status of this patch is. And surely by clearing the flag I wasn't implying anything about Johann's capability to handle this review.
(Reporter)

Comment 8

2 years ago
(In reply to :Paolo Amadini from comment #6)
> We're following the current convention for two new strings we're adding in
> bug 1206232. We'll need to change those strings in this bug. Ideally we
> would do this right after the other one lands, and this will save localizers
> a couple of strings to translate, but it's not that bad even if this bug
> misses Firefox 53.

Sounds like it would actually make sense to fix this before bug 1206232 to avoid extra work for localizers...

Also note that I wrote this patch in my spare time and I'm not sure how much time I can dedicate to this next week, so this is quite likely to miss 53 if I'll have to rewrite the patch.

Comment 9

2 years ago
(In reply to Dão Gottwald [:dao] from comment #8)
> Sounds like it would actually make sense to fix this before bug 1206232 to
> avoid extra work for localizers...

The reason for this prioritization is that we're in a time crunch for the other bug, and surely it will take time to get feedback for string changes. If this was filed a couple of weeks ago it would surely have made sense to do this in the order you suggested.

> Also note that I wrote this patch in my spare time and I'm not sure how much
> time I can dedicate to this next week, so this is quite likely to miss 53 if
> I'll have to rewrite the patch.

Don't worry, this is probably small enough that one of us can take it if we want it for Firefox 53. We'll discuss with the rest of the team at our next meeting.
(Assignee)

Comment 10

2 years ago
I don't think we necessarily need UX feedback on this. The original spec contained the strings in past tense (https://mozilla.invisionapp.com/share/PC3Y0QSRK#/screens/96290112) and that was only not implemented by us because at the time we thought it's not worth the effort. So I'll go ahead and review this as soon as I can.

As for the merge conflicts, I can see that it might be optimal to implement this after bug 1206232 and bug 1319112, but honestly I think we should just work on these three bugs normally and deal with conflicts wherever they arise.

> I wasn't implying anything about Johann's capability to handle this review.

No worries, I didn't read it that way :)

Comment 11

2 years ago
(In reply to Johann Hofmann [:johannh] from comment #10)
> I don't think we necessarily need UX feedback on this. The original spec
> contained the strings in past tense
> (https://mozilla.invisionapp.com/share/PC3Y0QSRK#/screens/96290112) and that
> was only not implemented by us because at the time we thought it's not worth
> the effort. So I'll go ahead and review this as soon as I can.

Good points about the original motivation for having the strings at the present tense.
(Assignee)

Comment 12

2 years ago
mozreview-review
Comment on attachment 8826836 [details]
Bug 1331172 - Current permission state should use the past tense.

https://reviewboard.mozilla.org/r/104706/#review105660

Nice, thank you! I don't think this will be hard to merge into bug 1206232, so please go ahead.

::: browser/modules/SitePermissions.jsm
(Diff revision 1)
> -   *   - label: the translated label of that state
>     */
>    getPermissionItem(aId, aState) {
> -    let availableStates = this.getAvailableStates(aId).map(state => {
> -      return { id: state, label: this.getStateLabel(aId, state) };
> -    });

Hah, good catch.

::: browser/modules/test/xpcshell/test_SitePermissions.js:78
(Diff revision 1)
> -    ]
>    });
> +  Assert.deepEqual(SitePermissions.getAvailableStates("camera"),
> +                   [ SitePermissions.UNKNOWN,
> +                     SitePermissions.ALLOW,
> +                     SitePermissions.BLOCK ]);

Technically this isn't testing getPermissionDetailsByURI anymore, so you might want to move this and the following three assertions into their own test.
Attachment #8826836 - Flags: review?(jhofmann) → review+
Whiteboard: [fxprivacy][triage][strings] → [fxprivacy][strings]
Comment hidden (mozreview-request)
(Reporter)

Comment 14

2 years ago
(In reply to :Paolo Amadini from comment #9)
> Don't worry, this is probably small enough that one of us can take it if we
> want it for Firefox 53. We'll discuss with the rest of the team at our next
> meeting.

-> NEEDINFO for this
Assignee: dao+bmo → nobody
Flags: needinfo?(mheubusch) → needinfo?(paolo.mozmail)
(Assignee)

Comment 15

2 years ago
I'll take it, thank you!
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Flags: needinfo?(paolo.mozmail)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8826836 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8827374 - Flags: review?(jhofmann)
(Assignee)

Comment 17

2 years ago
Sorry I'm not getting the hang of re-uploading to reviewboard. Anyway, requesting another quick review from Paolo since the changes were a bit more than I expected, after all.

Comment 19

2 years ago
mozreview-review
Comment on attachment 8827374 [details]
Bug 1331172 - Current permission state should use the past tense.

https://reviewboard.mozilla.org/r/105066/#review105906

::: browser/locales/en-US/chrome/browser/sitePermissions.properties:7
(Diff revision 1)
> -allowTemporarily = Allow Temporarily
> -block = Block
> -blockTemporarily = Block Temporarily
> -alwaysAsk = Always Ask
> +state.current.allowed = Allowed
> +state.current.allowedForSession = Allowed for Session
> +state.current.allowedTemporarily = Allowed Temporarily
> +state.current.blockedTemporarily = Blocked Temporarily
> +state.current.blocked = Blocked
> +
> +state.multichoice.allow = Allow
> +state.multichoice.allowForSession = Allow for Session
> +state.multichoice.block = Block

Having new strings for each case sounds good to me, so localizers can think if the old ones need updating given the different context. Please add a localization note for each group of strings, in which you clarify the context where the labels appear so they can be tested.

::: browser/modules/SitePermissions.jsm:227
(Diff revision 1)
> +   *           - state: the passed in state argument
> +   *           - scope: the passed in scope argument

This comment should be updated.

::: browser/modules/SitePermissions.jsm:474
(Diff revision 1)
> +   * @return {String} the localized label.
> +   */
> +  getMultichoiceStateLabel(state) {
> +    switch (state) {
> +      case this.UNKNOWN:
> +        return gStringBundle.GetStringFromName("state.alwaysAsk");

Worth having a separate "state.multichoice.alwaysAsk" string, the string might be different in locales other than English.

::: browser/modules/SitePermissions.jsm:499
(Diff revision 1)
>     * @return {String} the localized label.
>     */
> -  getStateLabel(state, scope = null) {
> +  getCurrentStateLabel(state, scope = null) {
>      switch (state) {
>        case this.UNKNOWN:
> -        return gStringBundle.GetStringFromName("alwaysAsk");
> +        return gStringBundle.GetStringFromName("state.alwaysAsk");

...and I think this case never happens in the current code, so we might as well remove it (with a comment in the JSDoc) so that the code falls back to the "return null" default. This way we have one less string to localize.

::: browser/modules/test/browser_SitePermissions.js:50
(Diff revision 1)
>    permissions = SitePermissions.getAllPermissionDetailsForBrowser(tab.linkedBrowser);
>  
>    camera = permissions.find(({id}) => id === "camera");
>    Assert.equal(camera, undefined);
>  
>    // check that different available state values are represented

nit: might update the comment, for what it's worth.
Attachment #8827374 - Flags: review?(paolo.mozmail)

Comment 20

2 years ago
Ah, almost forgot, I'm really not sure about what capitalization style is better for these strings, both for the Control Center and for the dialogs that allow permissions to be controlled. Worth exploring and trying to be consistent with other comparable parts of the user interface, if there are any.
Comment hidden (mozreview-request)
(Assignee)

Comment 22

2 years ago
Ugh
(Assignee)

Comment 23

2 years ago
mozreview-review
Comment on attachment 8827374 [details]
Bug 1331172 - Current permission state should use the past tense.

https://reviewboard.mozilla.org/r/105066/#review106636
Attachment #8827374 - Flags: review?(jhofmann) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 25

2 years ago
(In reply to :Paolo Amadini from comment #20)
> Ah, almost forgot, I'm really not sure about what capitalization style is
> better for these strings, both for the Control Center and for the dialogs
> that allow permissions to be controlled. Worth exploring and trying to be
> consistent with other comparable parts of the user interface, if there are
> any.

The strings seem pretty consistent to me :)
(Reporter)

Comment 26

2 years ago
(In reply to Johann Hofmann [:johannh] from comment #25)
> (In reply to :Paolo Amadini from comment #20)
> > Ah, almost forgot, I'm really not sure about what capitalization style is
> > better for these strings, both for the Control Center and for the dialogs
> > that allow permissions to be controlled. Worth exploring and trying to be
> > consistent with other comparable parts of the user interface, if there are
> > any.
> 
> The strings seem pretty consistent to me :)

Strings in different UIs aren't supposed to be consistent in the sense that they should all use title capitalization. I think title capitalization /might/ be the right choice for the radio buttons in Page Info > Permissions, but it feels more wrong for the permission states in the Control Center.

Comment 27

2 years ago
mozreview-review
Comment on attachment 8827374 [details]
Bug 1331172 - Current permission state should use the past tense.

https://reviewboard.mozilla.org/r/105066/#review106646

::: browser/locales/en-US/chrome/browser/sitePermissions.properties:5
(Diff revision 3)
> -allow = Allow
> -allowForSession = Allow for Session
> -allowTemporarily = Allow Temporarily
> -block = Block
> -blockTemporarily = Block Temporarily
> -alwaysAsk = Always Ask
> +# LOCALIZATION NOTE: The next labels (state.current.*) will be
> +# used to display active permission states in the site identity popup
> +# (which does not have a lot of screen space).
> +state.current.allowed = Allowed
> +state.current.allowedForSession = Allowed for Session
> +state.current.allowedTemporarily = Allowed Temporarily
> +state.current.blockedTemporarily = Blocked Temporarily
> +state.current.blocked = Blocked

You need to specify each label so the localization tools display the notes correctly. See this example I've worked on recently:

https://dxr.mozilla.org/mozilla-central/rev/96cb95af530477edb66ae48d98c18533476e57bb/browser/locales/en-US/chrome/browser/browser.properties#479-495
Attachment #8827374 - Flags: review?(paolo.mozmail) → review+

Comment 28

2 years ago
For capitalization, I've talked to Michelle who recommended to follow the current specification documents. She is already working on making capitalization consistent across the browser going forward.
Comment hidden (mozreview-request)
(Assignee)

Comment 30

2 years ago
I've kept the states capitalized for now. "Access Your Location: Blocked temporarily" looks wrong and the design spec is unfortunately not yet updated for the new permission labels, so I don't really know how to capitalize those. Anyway, since there's an ongoing UX project to review capitalization across the whole product, we may revisit this soon.

Comment 31

2 years ago
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/de03a86224cd
Current permission state should use the past tense. r=Paolo

Comment 32

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/de03a86224cd
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox53: fix-optional → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53

Comment 33

2 years ago
Build ID: 20170202030211
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.
Status: RESOLVED → VERIFIED
status-firefox53: fixed → verified
status-firefox54: --- → verified
You need to log in before you can comment on or make changes to this bug.