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

RESOLVED FIXED in Firefox 54

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: flod, Assigned: Fischer)

Tracking

unspecified
Firefox 54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: [storage-v1])

MozReview Requests

()

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

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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.
(Reporter)

Comment 1

2 years ago
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.
Summary: "Remove All Shown" button in about:preferences references non existing accesskey → "Remove All Shown" button in about:preferences should use its own accesskey
(Reporter)

Comment 2

2 years ago
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.
Summary: "Remove All Shown" button in about:preferences should use its own accesskey → "Remove All Shown" and button in about:preferences should use its own accesskey
(Assignee)

Comment 3

2 years ago
(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
Flags: needinfo?(francesco.lodolo)
(Reporter)

Comment 4

2 years ago
(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.
Flags: needinfo?(francesco.lodolo)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

2 years ago
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.
Attachment #8836568 - Flags: feedback?(francesco.lodolo)
(Assignee)

Updated

2 years ago
Whiteboard: [storage-v1]
(Reporter)

Comment 7

2 years ago
mozreview-review
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".
(Reporter)

Comment 8

2 years ago
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.
Attachment #8836568 - Flags: feedback?(francesco.lodolo) → feedback+
Comment hidden (mozreview-request)
(Assignee)

Comment 10

2 years ago
(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.
(Assignee)

Comment 11

2 years ago
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.
Attachment #8836568 - Flags: review?(jaws)
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.
Attachment #8836568 - Flags: review?(jaws) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 14

2 years ago
(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
Keywords: checkin-needed
sees there is one open issue that need to be fixed first
Flags: needinfo?(fliu)
Keywords: checkin-needed
(Assignee)

Updated

2 years ago
Flags: needinfo?(fliu)
Keywords: checkin-needed

Comment 16

2 years ago
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0d27d00f4570
"Remove All Shown" and button in about:preferences should use its own accesskey, r=jaws
Keywords: checkin-needed

Comment 17

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0d27d00f4570
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
You need to log in before you can comment on or make changes to this bug.