1331172 - Current permission state should use the past tense

Status: VERIFIED FIXED

(Firefox :: Site Identity, defect, P2)

Description

[Dão Gottwald [:dao]]

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 1

[Dão Gottwald [:dao]]

Attachment: Bug 1331172 - Current permission state should use the past tense.

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

Comment 2

[Dão Gottwald [:dao]]

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

Comment 3

[:Paolo Amadini]

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

Would you have any objection?

Comment 4

[:Paolo Amadini]

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.

Comment 5

[Dão Gottwald [:dao]]

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.

Comment 6

[:Paolo Amadini]

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.

Comment 7

[:Paolo Amadini]

(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.

Comment 8

[Dão Gottwald [:dao]]

(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

[:Paolo Amadini]

(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.

Comment 10

[Johann Hofmann [:johannh]]

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

[:Paolo Amadini]

(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 12

[Johann Hofmann [:johannh]]

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.

Comment 13

[Dão Gottwald [:dao]]

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

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

Comment 14

[Dão Gottwald [:dao]]

(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

Comment 15

[Johann Hofmann [:johannh]]

I'll take it, thank you!

Comment 16

[Johann Hofmann [:johannh]]

Attachment: Bug 1331172 - Current permission state should use the past tense.

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

Comment 17

[Johann Hofmann [:johannh]]

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 18

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0779d5a9417f

Comment 19

[:Paolo Amadini]

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.

Comment 20

[:Paolo Amadini]

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 21

[Johann Hofmann [:johannh]]

Comment on attachment 8827374 [details]
Bug 1331172 - Current permission state should use the past tense.

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

Comment 22

[Johann Hofmann [:johannh]]

Ugh

Comment 23

[Johann Hofmann [:johannh]]

Comment on attachment 8827374 [details]
Bug 1331172 - Current permission state should use the past tense.

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

Comment 24

[Johann Hofmann [:johannh]]

Comment on attachment 8827374 [details]
Bug 1331172 - Current permission state should use the past tense.

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

Comment 25

[Johann Hofmann [:johannh]]

(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 :)

Comment 26

[Dão Gottwald [:dao]]

(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

[:Paolo Amadini]

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

Comment 28

[:Paolo Amadini]

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 29

[Johann Hofmann [:johannh]]

Comment on attachment 8827374 [details]
Bug 1331172 - Current permission state should use the past tense.

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

Comment 30

[Johann Hofmann [:johannh]]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/de03a86224cd
Current permission state should use the past tense. r=Paolo

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/de03a86224cd

Comment 33

[Hani Yacoub, Desktop QA]

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.