1338757 - "Remove All Shown" and button in about:preferences should use its own accesskey

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

[Francesco Lodolo [:flod]]

https://hg.mozilla.org/mozilla-central/rev/fc4ac23cf72b#l1.15

removeSelectedCookies has accesskey removeSelectedCookies.accesskey

But there's no such string anymore in the file, you forgot to move it from the dtd file.

Comment 1

[Francesco Lodolo [:flod]]

Ehm, forget comment 0, foggy brain got lost when switching from the .properties file to code.

What got me confused is that removeAllShownCookies.label and removeAllCookies.label share the same accesskey (removeAllCookies.accesskey)
https://hg.mozilla.org/mozilla-central/rev/fc4ac23cf72b#l1.13

It would be safer to have removeAllShownCookies.accesskey, and explain that the same accesskey could be used for both, since the two labels are mutually exclusive, and never displayed together.

Comment 2

[Francesco Lodolo [:flod]]

Some more notes.

https://hg.mozilla.org/mozilla-central/rev/fc4ac23cf72b#l6.49

removeAll.* and removeAllShown.* for Site Data strings are a very bad ID choice, how are localizers supposed to know that they reference websites? In most languages "shown" and "all" need to be declined according to the noun they refer to.

That was evident before, since they were in a siteDataSettings.dtd, now they're in preferences.properties. Localization comment should explain that (currently it's not), but the string ID should also be self-explanatory.

When so many strings are involved, I'm happy to give feedback beforehand.

Comment 3

[Fischer [:Fischer]]

(In reply to Francesco Lodolo [:flod] from comment #1)
> Ehm, forget comment 0, foggy brain got lost when switching from the
> .properties file to code.
> 
> What got me confused is that removeAllShownCookies.label and
> removeAllCookies.label share the same accesskey (removeAllCookies.accesskey)
> https://hg.mozilla.org/mozilla-central/rev/fc4ac23cf72b#l1.13
> 
> It would be safer to have removeAllShownCookies.accesskey, and explain that
> the same accesskey could be used for both, since the two labels are mutually
> exclusive, and never displayed together.
Hi Francesco,
In fact, removeAllShownCookies.label and removeAllCookies.label both are used on the exactly same button. That is why here is only one accesskey. In this case, would it be better that add some comments about this?

(In reply to Francesco Lodolo [:flod] from comment #2)
> https://hg.mozilla.org/mozilla-central/rev/fc4ac23cf72b#l6.49
> 
> removeAll.* and removeAllShown.* for Site Data strings are a very bad ID
> choice, how are localizers supposed to know that they reference websites? In
> most languages "shown" and "all" need to be declined according to the noun
> they refer to.
> 
> That was evident before, since they were in a siteDataSettings.dtd, now
> they're in preferences.properties. Localization comment should explain that
> (currently it's not), but the string ID should also be self-explanatory.
> 
> When so many strings are involved, I'm happy to give feedback beforehand.
Do you mean something like removeAllSites.* and removeAllSitesShown.* are better for localizers to understand?
If this is the case, then, a update in passwordmgr.properties is desired as well because it is about websites too.

Thanks

Comment 4

[Francesco Lodolo [:flod]]

(In reply to Fischer [:Fischer] from comment #3)
> In fact, removeAllShownCookies.label and removeAllCookies.label both are
> used on the exactly same button. That is why here is only one accesskey. In
> this case, would it be better that add some comments about this?

The fact that the two label are almost identical in English and can share an accesskey doesn't mean it's necessarily true for all languages. It would be safer to have one accesskey for each label, and then explain that the same letter can be used (if possible).

> Do you mean something like removeAllSites.* and removeAllSitesShown.* are
> better for localizers to understand?

Or removeSitesDataAll and removeSitesDataShown.

> If this is the case, then, a update in passwordmgr.properties is desired as
> well because it is about websites too.

Given that they're in passwordmrg.properties, that's not as important. Adding a comment definitely wouldn't hurt.

Comment 5

[Fischer [:Fischer]]

Attachment: Bug 1338757 - "Remove All Shown" and button in about:preferences should use its own accesskey,

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

Comment 6

[Fischer [:Fischer]]

Comment on attachment 8836568 [details]
Bug 1338757 - "Remove All Shown" and button in about:preferences should use its own accesskey,

Hi Francesco,

Would you please give some feedbacks on the string updates?
Now "Remove All Shown" has its own label and accesskey. The comments denoting "Remove All" and "Remove All Shown" are both for the same oen button are appended.
Please let us know if any still unclear for localization.

Thanks.

Comment 7

[Francesco Lodolo [:flod]]

Comment on attachment 8836568 [details]
Bug 1338757 - "Remove All Shown" and button in about:preferences should use its own accesskey,

https://reviewboard.mozilla.org/r/111970/#review113214

::: browser/locales/en-US/chrome/browser/preferences/preferences.properties:128
(Diff revision 1)
>  noCookieSelected=<no cookie selected>
>  cookiesAll=The following cookies are stored on your computer:
>  cookiesFiltered=The following cookies match your search:
>  
>  # LOCALIZATION NOTE (removeAllCookies, removeAllShownCookies):
> +# removeAllCookies and removeAllShownCookies are both used on the same one button.

I would use: "removeAllCookies and removeAllShownCookies are never displayed at the same time and can share the same accesskey".

::: browser/locales/en-US/chrome/browser/preferences/preferences.properties:128
(Diff revision 1)
>  noCookieSelected=<no cookie selected>
>  cookiesAll=The following cookies are stored on your computer:
>  cookiesFiltered=The following cookies match your search:
>  
>  # LOCALIZATION NOTE (removeAllCookies, removeAllShownCookies):
> +# removeAllCookies and removeAllShownCookies are both used on the same one button.

I would say something more explicit: "removeAllCookies and removeAllShownCookies are never displayed together and can share the same accesskey".

Comment 8

[Francesco Lodolo [:flod]]

Comment on attachment 8836568 [details]
Bug 1338757 - "Remove All Shown" and button in about:preferences should use its own accesskey,

Uhm, mozreview did something weird with my comments. The same comment style should be applied to all 3 cases.

It looks good though, thanks.

Comment 9

[Fischer [:Fischer]]

Comment on attachment 8836568 [details]
Bug 1338757 - "Remove All Shown" and button in about:preferences should use its own accesskey,

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

Comment 10

[Fischer [:Fischer]]

(In reply to Francesco Lodolo [:flod] from comment #8)
> Comment on attachment 8836568 [details]
> Bug 1338757 - "Remove All Shown" and button in about:preferences should use
> its own accesskey
> 
> Uhm, mozreview did something weird with my comments. The same comment style
> should be applied to all 3 cases.
> 
> It looks good though, thanks.
Thanks, updated.
p.s the f+ flag was removed automatically after updating the mozreview.

Comment 11

[Fischer [:Fischer]]

Comment on attachment 8836568 [details]
Bug 1338757 - "Remove All Shown" and button in about:preferences should use its own accesskey,

Hi Jaws,
This patch updates the strings based on the feedbacks from Francesco (Comment 8) so they are better for localization, thanks.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8836568 [details]
Bug 1338757 - "Remove All Shown" and button in about:preferences should use its own accesskey,

https://reviewboard.mozilla.org/r/111970/#review113320

::: browser/components/preferences/cookies.js:881
(Diff revision 2)
> -      this._bundle.getString("removeAllShownCookies.label") : this._bundle.getString("removeAllCookies.label");
> -    removeAllCookies.setAttribute("label", label);
> +    let removeAllCookiesLabelStringID = "removeAllCookies.label";
> +    let removeAllCookiesAccesskeyStringID = "removeAllCookies.accesskey";
> +    if (this._view._filtered) {
> +      removeAllCookiesLabelStringID = "removeAllShownCookies.label";
> +      removeAllCookiesAccesskeyStringID = "removeAllShownCookies.accesskey";

Because this function is called "updateRemoveAllButton", it should be explanatory that the strings will be related to the Remove All button.

Therefore, you can use `labelStringID` and `accessKeyStringID` as the variable names instead, which are shorter and thus easier to read.

Comment 13

[Fischer [:Fischer]]

Comment on attachment 8836568 [details]
Bug 1338757 - "Remove All Shown" and button in about:preferences should use its own accesskey,

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

Comment 14

[Fischer [:Fischer]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #12)
> ::: browser/components/preferences/cookies.js:881
> (Diff revision 2)
> > -      this._bundle.getString("removeAllShownCookies.label") : this._bundle.getString("removeAllCookies.label");
> > -    removeAllCookies.setAttribute("label", label);
> > +    let removeAllCookiesLabelStringID = "removeAllCookies.label";
> > +    let removeAllCookiesAccesskeyStringID = "removeAllCookies.accesskey";
> > +    if (this._view._filtered) {
> > +      removeAllCookiesLabelStringID = "removeAllShownCookies.label";
> > +      removeAllCookiesAccesskeyStringID = "removeAllShownCookies.accesskey";
> 
> Because this function is called "updateRemoveAllButton", it should be
> explanatory that the strings will be related to the Remove All button.
> 
> Therefore, you can use `labelStringID` and `accessKeyStringID` as the
> variable names instead, which are shorter and thus easier to read.
Thanks, updated.
TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e95db79fc3b7df9b3bcb66920beeaf957584505

Comment 15

[Carsten Book [:Tomcat]]

sees there is one open issue that need to be fixed first

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0d27d00f4570
"Remove All Shown" and button in about:preferences should use its own accesskey, r=jaws

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0d27d00f4570