Closed Bug 1331172 Opened 7 years ago Closed 7 years ago

Current permission state should use the past tense

Categories

(Firefox :: Site Identity, defect, P2)

Product:
Firefox
Component:
Site Identity
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 53
Tracking Flags:
Tracking Status
firefox53 --- verified
firefox54 --- verified

People

(Reporter: dao, Assigned: johannh)

References

Details

(Whiteboard: [fxprivacy][strings])

Attachments

(1 file, 1 obsolete file)

Bug 1331172 - Current permission state should use the past tense.
59 bytes, text/x-review-board-request
johannh
: review+
Paolo
: review+
Details 
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."
Assignee: nobody → dao+bmo
I also suspect "Allowed for Session" and "Allowed Temporarily" shouldn't use title capitalization in this context...
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 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)
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)
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
(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.
(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.
(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.
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 :)
(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.
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]
(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)
I'll take it, thank you!
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Flags: needinfo?(paolo.mozmail)
Attachment #8826836 - Attachment is obsolete: true
Attachment #8827374 - Flags: review?(jhofmann)
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 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)
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.
Ugh
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+
(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 :)
(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 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+
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.
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.
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/de03a86224cd
Current permission state should use the past tense. r=Paolo
https://hg.mozilla.org/mozilla-central/rev/de03a86224cd
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox53: fix-optional → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
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.

Attachment

General

Creator:
Created:
Updated:
Size: