Closed Bug 1338757 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Settings UI, defect)

Product:
Firefox
Component:
Settings UI
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: flod, Assigned: Fischer)

References

Details

(Whiteboard: [storage-v1])

Attachments

(1 file)

Bug 1338757 - "Remove All Shown" and button in about:preferences should use its own accesskey,
59 bytes, text/x-review-board-request
jaws
: review+
Details 
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.
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
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
(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)
(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 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)
Whiteboard: [storage-v1]
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 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+
(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 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+
(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
Flags: needinfo?(fliu)
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/0d27d00f4570
Status: NEW → RESOLVED
Closed: 7 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.

Attachment

General

Creator:
Created:
Updated:
Size: