1152743 - Saved Files tab (about:downloads): Re-Implement "Search" field and "Clear List" button

Status: RESOLVED FIXED

(Thunderbird :: General, enhancement)

Description

[Thomas D. (:thomas8)]

+++ This bug was initially created as a clone of Bug #1152736 +++

Saved Files dialogue in TB currently has a "Search" field to search within the names of searched files, and a "Clear List" button. Both have been lost by the migration to about:downloads, and both should be restored (Dedicated toolbar at the top of the saved files tab).

Comment 1

[:aceman]

Yeah, let's try this. The UI is kinda strange without any buttons.

Comment 2

[:aceman]

Attachment: patch

Is this what is wanted?

I added the border in the themes because the current state (full white area without any separators) is totally ugly, at least on linux. Of course you can propose any other style.

Comment 3

[:aceman]

Attachment: screenshot of the patch

This is how it looks like. I am not sure why the "Search" text is grey. It should be black as on the button. That still needs fixing.

Comment 4

[Thomas D. (:thomas8)]

Comment on attachment 8597455 [details]
screenshot of the patch

Thanks a lot for taking this, and it's looking good except a few nits:

1a) Pls use "Clear List" as button text. "Clear *Downloads*" is unexpected (for Saved *Files*, we don't use the term downloads anywhere in the UI), ambiguous (will it clear my downloaded files, too?), and inconsistent with FF and TB 31, which both just use "Clear List", which is just fine to describe what it does...
1b) Same for the context menu, pls replace "Clear Downloads" with "Clear List".
(ux-consistency)

2) Also, pls add the button tooltip exactly as present in TB 31:
"Removes completed, canceled, and failed downloads from the list."
That's probably a correct and positive way of saying that the currently still ongoing downloads will not be cleared from the list (I think).

3) Pls remove the label "Search for saved files" in front of the search box; I think we don't need/want this, because:
- inconsistent with most other searchboxes in TB and FF, including the "Saved Files" searchbox in TB31, none of which have labels (ux-consistency)
- permanent text clutter in the UI without much benefit, if we ensure keyboard accessibility in other ways (ux-minimalism)
- magnifier icon indicates "search", and the placeholder indicates what to search for

4) Because of 3a), pls change placeholder text to just "Search...". I don't think we need to indicate that this will search file names, because there's really nothing else to search (it's quite unlikely that users will try to search for Date, Time, or KiloBytes of a saved file; even Windows Explorer won't do that in the default search box). Let's keep it simple (ux-minimalism). In the long run, those details tend to irritate because they are unnecessary information which is easy to understand and remember by just using the feature. Parsing the meaning of the extra words each time takes longer than just going for "Search...", using the default search keyboard shortcut of 5) below.

5) Pls implement Ctrl+F keyboard shortcut for this search box, as present in TB 31. Imo, that's still sufficient and intuitive for keyboard access.

ui-r=me with that fixed.

Comment 5

[:aceman]

Attachment: patch v2

This should address the requests from Thomas.

Comment 6

[Thomas D. (:thomas8)]

Comment on attachment 8597455 [details]
screenshot of the patch

Screenshot no longer represents the patch -> cancel ui-review of obsolete screenshot (updated screenshot welcome :)

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 8606642 [details] [diff] [review]
patch v2

Review of attachment 8606642 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, r=mkmelin with a few nits

::: mail/components/downloads/content/aboutDownloads.js
@@ +112,5 @@
> +    let searchString = document.getElementById("searchBox").value;
> +    for (let i = 0; i < this.listElement.itemCount; i++) {
> +      let downloadElem = this.listElement.getItemAtIndex(i);
> +      downloadElem.collapsed =
> +        !downloadElem.downloadItem.fileName.contains(searchString);

.includes, not .contains

::: mail/components/downloads/content/aboutDownloads.xul
@@ +43,5 @@
> +         oncommand="document.getElementById('searchBox').focus();"/>
> +  </keyset>
> +
> +  <hbox align="center">
> +    <button command="msgDownloadsCmd_clearDownloads"

add an id for this

::: mail/locales/en-US/chrome/messenger/aboutDownloads.dtd
@@ +18,5 @@
>  <!ENTITY cmd.removeFromHistory.label               "Remove From History">
>  <!ENTITY cmd.removeFromHistory.accesskey           "e">
> +<!ENTITY cmd.clearList.label                       "Clear List">
> +<!ENTITY cmd.clearList.accesskey                   "C">
> +<!ENTITY cmd.clearList.tooltip                     "Removes completed, canceled, and failed downloads from the list.">

"Removes all saved files from the list."

Comment 8

[Magnus Melin [:mkmelin]]

Oh, and the patch had bitrotted slightly, so you should refresh it

Comment 9

[:aceman]

(In reply to Magnus Melin from comment #7)
> > +<!ENTITY cmd.clearList.tooltip                     "Removes completed, canceled, and failed downloads from the list.">
> 
> "Removes all saved files from the list."

This intentionally copies the same string that was there in the original Saved files dialog. Do you really want to change it?

Comment 10

[Thomas D. (:thomas8)]

(In reply to Magnus Melin from comment #7)
> Comment on attachment 8606642 [details] [diff] [review]
> patch v2
> 
> Review of attachment 8606642 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM, r=mkmelin with a few nits
> 
> ::: mail/components/downloads/content/aboutDownloads.js
> @@ +112,5 @@
> > +    let searchString = document.getElementById("searchBox").value;
> > +    for (let i = 0; i < this.listElement.itemCount; i++) {
> > +      let downloadElem = this.listElement.getItemAtIndex(i);
> > +      downloadElem.collapsed =
> > +        !downloadElem.downloadItem.fileName.contains(searchString);
> 
> .includes, not .contains

Is fileName an array? I'd think not. So isn't this just a string comparison for each element, which requires 'contains'?

> 
> ::: mail/components/downloads/content/aboutDownloads.xul
> @@ +43,5 @@
> > +         oncommand="document.getElementById('searchBox').focus();"/>
> > +  </keyset>
> > +
> > +  <hbox align="center">
> > +    <button command="msgDownloadsCmd_clearDownloads"
> 
> add an id for this
> 
> ::: mail/locales/en-US/chrome/messenger/aboutDownloads.dtd
> @@ +18,5 @@
> >  <!ENTITY cmd.removeFromHistory.label               "Remove From History">
> >  <!ENTITY cmd.removeFromHistory.accesskey           "e">
> > +<!ENTITY cmd.clearList.label                       "Clear List">
> > +<!ENTITY cmd.clearList.accesskey                   "C">
> > +<!ENTITY cmd.clearList.tooltip                     "Removes completed, canceled, and failed downloads from the list.">
> 
> "Removes all saved files from the list."

It will remove not only (successfully) saved files, but also canceled and failed downloads. And it will NOT remove ongoing downloads. That's important information to know before proceeding with that action. Tooltips are meant to provide more detail - the short & easy version is already on the button label (Clear List).
I don't see any reason to change this string from what it was before, more so because FF has the same, so we should be consistent in our wording.

Comment 11

[:aceman]

(In reply to Thomas D. from comment #10)
> > .includes, not .contains
> 
> Is fileName an array? I'd think not. So isn't this just a string comparison
> for each element, which requires 'contains'?

String.contains() is also migrated to String.includes(), see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String .

Comment 12

[:aceman]

Attachment: patch v2.1

Comment 13

[Magnus Melin [:mkmelin]]

(In reply to Thomas D. from comment #10)
> It will remove not only (successfully) saved files, but also canceled and
> failed downloads. And it will NOT remove ongoing downloads. That's important
> information to know before proceeding with that action. Tooltips are meant
> to provide more detail - the short & easy version is already on the button
> label (Clear List).
> I don't see any reason to change this string from what it was before, more
> so because FF has the same, so we should be consistent in our wording.

But we don't really have any of those. What you have is a list of attachments you saved. 
I don't feel strongly about it though, so lets hear what others prefer.

Comment 14

[Thomas D. (:thomas8)]

(In reply to Magnus Melin from comment #13)
> (In reply to Thomas D. from comment #10)
> > It will remove not only (successfully) saved files, but also canceled and
> > failed downloads. And it will NOT remove ongoing downloads. That's important
> > information to know before proceeding with that action. Tooltips are meant
> > to provide more detail - the short & easy version is already on the button
> > label (Clear List).
> > I don't see any reason to change this string from what it was before, more
> > so because FF has the same, so we should be consistent in our wording.
> 
> But we don't really have any of those. What you have is a list of
> attachments you saved. 
> I don't feel strongly about it though, so lets hear what others prefer.

Hmm. I think all of these cases CAN happen with downloaded "saved files", too.
For big files, you can certainly cancel the ongoing download half-way down the road.
For remote files, or perhaps deleting source mail while downloading big file etc., I'd guess we can also have failed downloads.

Moreover, "Removes all saved files from the list." doesn't sound adequate because at least it will NOT remove those saved files which are still in the process of downloading (if the tooltip is actually correctly describing the behaviour).

Comment 15

[Magnus Melin [:mkmelin]]

(In reply to Thomas D. from comment #14)
> Moreover, "Removes all saved files from the list." doesn't sound adequate
> because at least it will NOT remove those saved files which are still in the
> process of downloading 

If we're being nitpicky, I wouldn't call those saved yet.

Comment 16

[Thomas D. (:thomas8)]

(In reply to Magnus Melin from comment #15)
> (In reply to Thomas D. from comment #14)
> > Moreover, "Removes all saved files from the list." doesn't sound adequate
> > because at least it will NOT remove those saved files which are still in the
> > process of downloading 
> 
> If we're being nitpicky, I wouldn't call those saved yet.

You are right. So the mismatch is more about those other types I mentioned, because we also remove canceled or failed downloads which I wouldn't think of when I hear "saved files" (which sounds like "successfully saved files"). But I don't have strong feelings about this.

Perhaps this?

"Remove all entries from the list of saved files, except ongoing downloads"

- "all entries" is neutral so it also includes canceled and failed downloads which have not yet become "saved files", but will also be removed
- in general, this is the "list of saved files", so let's add that for clarity
- make clear which entries will *not* be removed (which I think was the original purpose of that somewhat clumsy tooltip on Firefox; and somebody should test with big download if it actually works that way...)

Comment 17

[Kent James (:rkent)]

This is a sorta regression, and as such it would be a candidate to fix in TB 38. Would it be possible to do a variant of the UI that reused existing strings, that we could land in TB 38 (maybe 38.1.0)?

Comment 18

[Thomas D. (:thomas8)]

(In reply to Kent James (:rkent) from comment #17)
> This is a sorta regression, and as such it would be a candidate to fix in TB
> 38. Would it be possible to do a variant of the UI that reused existing
> strings, that we could land in TB 38 (maybe 38.1.0)?

Yes, definitely possible and desirable to not regress this for TB 38.

1) We have all the required strings in TB 31.7.0:
- Clear List
- Removes completed, canceled, and failed downloads from the list.
- Search...
I don't know how much work it is technically to get those back.

2) The current patch takes strings from
http://mxr.mozilla.org/comm-central/source/mail/locales/en-US/chrome/messenger/aboutDownloads.dtd
...which currently has some unwanted strings:
<!ENTITY cmd.clearDownloads.label                  "Clear Downloads">
But we could also take that temporarily.

3) Finally, for a temporary transition period, we could snatch the desired (existing/current) strings from all sorts of other locations where they still exist in the current codebase (and which might actually still be the same as the originals in 1):

http://mxr.mozilla.org/comm-central/search?string=Clear%2BList
http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/locales/en-US/chrome/mozapps/downloads/downloads.dtd#42
http://mxr.mozilla.org/comm-central/source/mozilla/webapprt/locales/en-US/webapprt/downloads/downloads.dtd#42
http://mxr.mozilla.org/comm-central/source/mail/locales/en-US/chrome/messenger/activity.dtd#31

Comment 19

[:aceman]

(In reply to Kent James (:rkent) from comment #17)
> This is a sorta regression, and as such it would be a candidate to fix in TB
> 38. Would it be possible to do a variant of the UI that reused existing
> strings, that we could land in TB 38 (maybe 38.1.0)?

Yes, I can try that once the final shape of this is approved for trunk.

Comment 20

[Josiah Bruner [:jsbruner]]

Comment on attachment 8609866 [details] [diff] [review]
patch v2.1

Review of attachment 8609866 [details] [diff] [review]:
-----------------------------------------------------------------

I'm going to let Blake and Thomas handle the reviews for this one. Personally I would like there to be an actual toolbar, with the button and field inside it, instead of what the screenshot shows, but that's not a big deal.

Comment 21

[:aceman]

(In reply to Josiah Bruner [:JosiahOne] (needinfo for responses) from comment #20)
> Personally I would like there to be an actual toolbar, with the button and
> field inside it, instead of what the screenshot shows, but that's not a big
> deal.

How would it look differently with a toolbar? Or do you mean a toolbar above the tab bar?

Comment 22

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #21)
> (In reply to Josiah Bruner [:JosiahOne] (needinfo for responses) from
> comment #20)
> > Personally I would like there to be an actual toolbar, with the button and
> > field inside it, instead of what the screenshot shows, but that's not a big
> > deal.
> 
> How would it look differently with a toolbar? Or do you mean a toolbar above
> the tab bar?

It would just look "more heavy" with a toolbar (extra lines, larger, less smooth), so I don't see any advantage.
These are just two lightweight controls which are added to the background as an integral part of the list. Which is different in quantity from an entire set of tools which needs visual distinction from the target area. (ux-minimalism)

Also, Addons tab is very similar in structure and behaviour and we have the same lightweight layout there (ux-consistency).
Also in general (for better or worse), we seem to go away from extra lines where they can be avoided in favor of a more flat design...

Comment 23

[Kent James (:rkent)]

This seems to have stalled, not sure why. Aceman, are you waiting for comments from Thomas before moving forward?

Comment 24

[Thomas D. (:thomas8)]

Comment on attachment 8609866 [details] [diff] [review]
patch v2.1

Review of attachment 8609866 [details] [diff] [review]:
-----------------------------------------------------------------

Kent, thanks for the friendly reminder :)

From code inspection, looks good to me. Let's not burden Blake with small things which we can handle ourselves; he'll chip in anyway if we go wild... ;) After all, this is just restoring the original layout and functionality of the "Clear List" button and "Search..." field.

::: mail/components/downloads/content/aboutDownloads.js
@@ +97,5 @@
>      this.items.delete(aDownload);
>      this.listElement.removeChild(item.element);
>    },
>  
> +  onDownloadContextMenu() {

Is it no longer necessary to check for empty selection? (Not sure what the removed element check was doing before?)

On current release, when deselecting all downloads (use Ctrl+click on selected item), if I right-click on empty white space below downloads, I still get the *download-item-related* full context menu, which is odd because most commands won't do anything except "clear list". Can we fix that (in a followup bug if you prefer)? I'd expect the *list-related* context menu for which only "clear list" should be enabled/shown. In fact, right/left-clicking on the blank space below items should deselect everything under Windows (here or followup bug).

@@ +121,3 @@
>    supportsCommand(aCommand) {
> +    return (this.commands.includes(aCommand) ||
> +            (DownloadItem.prototype.supportsCommand(aCommand)));

Nice code cleanup, here and elsewhere! :)

@@ +127,5 @@
> +    switch (aCommand) {
> +      case "msgDownloadsCmd_clearDownloads":
> +      case "msgDownloadsCmd_searchDownloads":
> +        // We could disable these if there are no downloads in the list, but
> +        // updating the commands when new items become available is tricky.

Clear Downloads should be disabled (followup bug ok), Search Downloads should stay enabled (e.g. Windows Explorer also allows searching on empty folders).

Comment 25

[:aceman]

(In reply to Thomas D. from comment #24)
> > +  onDownloadContextMenu() {
> 
> Is it no longer necessary to check for empty selection? (Not sure what the
> removed element check was doing before?)

I do handle empty selection in which case only the "Clear list" item is enabled in the context menu.

> On current release, when deselecting all downloads (use Ctrl+click on
> selected item), if I right-click on empty white space below downloads, I
> still get the *download-item-related* full context menu, which is odd
> because most commands won't do anything except "clear list". Can we fix that
> (in a followup bug if you prefer)? I'd expect the *list-related* context
> menu for which only "clear list" should be enabled/shown. In fact,
> right/left-clicking on the blank space below items should deselect
> everything under Windows (here or followup bug).

I think this is the same as I write above.

Comment 26

[:aceman]

Attachment: patch v2.2

Used the "remove all entries" string from comment 16. I hope the idea of a toolbar can be scratched for now.

Comment 27

[Magnus Melin [:mkmelin]]

Comment on attachment 8648165 [details] [diff] [review]
patch v2.2

Review of attachment 8648165 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thx! r=mkmelin

Comment 28

[:aceman]

Comment on attachment 8648165 [details] [diff] [review]
patch v2.2

Eh, looks like we have no ui-review here yet :)

Comment 29

[Thomas D. (:thomas8)]

Comment on attachment 8648165 [details] [diff] [review]
patch v2.2

Review of attachment 8648165 [details] [diff] [review]:
-----------------------------------------------------------------

Well, I'm not yet perfectly happy with the general layout of "Saved Files", but as we're not making it worse than before, I think this is good enough for fast landing to return the functionality to users, with one or two nits fixed.

> border-bottom: 1px dotted black;

I like the visual separation of items because without separation it's very hard to tell them apart (more so because of bug 1152741); it also helps discoverability of clickable area per item.

Nit1: However, dotted black is too strong and too reminiscent of focus marker.
Please use the same color which is used for the lightgrey-ish frame around the items (something near 1px dotted lightgrey but we want exactly the same color).

Nit2: On screenshot attachment 8597455 [details], Clear List button and search field are both horizontally offset to the inside compared to the item border (left/right). If it's not too much work, pls make them left/right-align correctly (otherwise we can polish that in a followup bug).

Nit3: I'm wondering if we need a bit more vertical distance between the Clear List button / Search Field and the beginning of the items list, imo it looks slightly squeezed/ too tacked on in screenshot attachment 8597455 [details]. It should not be as much as the frame on top, maximally half of that, probably less (try 1/4 of the outer frame width; we want to avoid heavy toolbar look). New screenshot on Windows? (or polish in followup bug).

In the long run, we'll probably want to get rid of the light-grey frame altogether (which is a visual alien) and make it look more like the revamped Addons list in FF with a flat background (let's try that in a followup bug).

Richard, welcome to add more, but I think Aceman just wanted to speed this up so cancelling your additional ui-r for the time being.

Comment 30

[Richard Marti (:Paenglab)]

(In reply to Thomas D. from comment #29)
> Comment on attachment 8648165 [details] [diff] [review]
> patch v2.2
> 
> Review of attachment 8648165 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nit1: However, dotted black is too strong and too reminiscent of focus
> marker.
> Please use the same color which is used for the lightgrey-ish frame around
> the items (something near 1px dotted lightgrey but we want exactly the same
> color).

border-bottom: 1px solid #f1f1f1;

> Nit2: On screenshot attachment 8597455 [details], Clear List button and
> search field are both horizontally offset to the inside compared to the item
> border (left/right). If it's not too much work, pls make them
> left/right-align correctly (otherwise we can polish that in a followup bug).

#clearDownloads {
 margin-inline-start: 0;
}

#searchBox {
 margin-inline-end: 0;
}
should work.

> Nit3: I'm wondering if we need a bit more vertical distance between the
> Clear List button / Search Field and the beginning of the items list, imo it
> looks slightly squeezed/ too tacked on in screenshot attachment 8597455 [details]
> [details]. It should not be as much as the frame on top, maximally half of
> that, probably less (try 1/4 of the outer frame width; we want to avoid
> heavy toolbar look). New screenshot on Windows? (or polish in followup bug).

You could give the surrounding hbox an ID and then a padding-bottom: 5px

> In the long run, we'll probably want to get rid of the light-grey frame
> altogether (which is a visual alien) and make it look more like the revamped
> Addons list in FF with a flat background (let's try that in a followup bug).

It's a in-content page.

Comment 31

[Thomas D. (:thomas8)]

(In reply to Richard Marti (:Paenglab) from comment #30)

Thanks! :)

> (In reply to Thomas D. from comment #29)
> > Comment on attachment 8648165 [details] [diff] [review]

> border-bottom: 1px solid #f1f1f1;

Nice! Cleaner than dotted.

> > In the long run, we'll probably want to get rid of the light-grey frame
> > altogether (which is a visual alien) and make it look more like the revamped
> > Addons list in FF with a flat background (let's try that in a followup bug).
> 
> It's a in-content page.

Can you elaborate a bit? Which page is in-content, and is that in favor of my idea or sceptical?

Comment 32

[Richard Marti (:Paenglab)]

(In reply to Thomas D. from comment #31)
> (In reply to Richard Marti (:Paenglab) from comment #30)
> > > In the long run, we'll probably want to get rid of the light-grey frame
> > > altogether (which is a visual alien) and make it look more like the revamped
> > > Addons list in FF with a flat background (let's try that in a followup bug).
> > 
> > It's a in-content page.
> 
> Can you elaborate a bit? Which page is in-content, and is that in favor of
> my idea or sceptical?

The download list is in a tab and also a about: page ergo it's a inline page and looks not as an alien on modern OSes.

Comment 33

[:aceman]

Attachment: patch v2.3

Thanks, the style suggestions worked for me, on linux.

Comment 34

[Richard Marti (:Paenglab)]

Comment on attachment 8652520 [details] [diff] [review]
patch v2.3

Review of attachment 8652520 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

::: mail/themes/linux/mail/downloads/aboutDownloads.css
@@ +28,5 @@
> +  border-bottom: 1px solid #f1f1f1;
> +}
> +
> +#clearDownloads {
> +  margin-inline-start: 0; */

Unneeded */ at the end of the line which throws a warning. The same is in osx and windows files.

Comment 35

[:aceman]

Attachment: patch v2.3.1

Thanks.

Comment 36

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/0977b5ddcf5ed7091e1a4ccd516020d775d584ad
Bug 1152743 - Re-implement "Search" field and "Clear List" button in Saved Files tab. ui-r=ThomasD, r=mkmelin, ui-r=Paenglab

Comment 37

[Thomas D. (:thomas8)]

Thank you aleth! :)

Comment 38

[:aceman]

Attachment: patch for TB38

So this is the version for TB38. It is mostly the same, just does not add new strings into aboutDownloads.dtd, instead uses the cmd.clearList* ones from activity.dtd. It seems to me there are no duplicate entities in these 2 files. I tested it on trunk as I do not have ESR tree. It worked on trunk.
Do we have an comm-ESR38-try server where we could test it? Or are there some builds from comm-ESR38 branch even before its release to users?

Comment 39

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Comment on attachment 8657886 [details] [diff] [review]
patch for TB38

Review of attachment 8657886 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/downloads/content/aboutDownloads.js
@@ +121,2 @@
>    supportsCommand(aCommand) {
> +    return (this.commands.includes(aCommand) ||

The first command / condition is already present in comm-esr38 and it should always fail because this.commands is an array and Array.includes only got turned on for branches different from trunk in 43.0a1.

Comment 40

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch for TB38, v2

Comment 41

[:aceman]

Oh, thanks for the updated patch.

Comment 42

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch for TB38, v3

Comment 43

[:aceman]

Comment on attachment 8657952 [details] [diff] [review]
patch for TB38, v3

Review of attachment 8657952 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, seems to be complete now.

Comment 44

[Kent James (:rkent)]

Comment on attachment 8657952 [details] [diff] [review]
patch for TB38, v3

Review of attachment 8657952 [details] [diff] [review]:
-----------------------------------------------------------------

I'm stealing this review since I tested it, and it does not work. This is not a thorough review.

::: mail/components/downloads/content/aboutDownloads.js
@@ +121,2 @@
>    supportsCommand(aCommand) {
> +    return (this.commands.contains(aCommand) ||

This does not actually work, because an array does not contain a .contains function. The old way to do this is:

(this.commands.indexOf(aCommand) >= 0)

This occurs multiple places in the patch. This really needs testing on an esr38 build.

Comment 45

[Kent James (:rkent)]

Comment on attachment 8657952 [details] [diff] [review]
patch for TB38, v3

Change of plans. The required changes are only two lines, I'll accept this and just fix the issues myself. I'm going to land on the beta (needs one small change) so that we can test there.

Comment 46

[Kent James (:rkent)]

Comment on attachment 8657952 [details] [diff] [review]
patch for TB38, v3

https://hg.mozilla.org/releases/comm-beta/rev/3db4218c150a
https://hg.mozilla.org/releases/comm-esr38/rev/a53289307b87

I also landed this patch on beta since we want to test this in our next beta. There was one small adaptation required.

Comment 47

[:aceman]

Thank you rkent.

Comment 48

[Kent James (:rkent)]

Comment on attachment 8657952 [details] [diff] [review]
patch for TB38, v3

This would have been comm-aurora+ if I had pushed this originally. I want it in B 42 though, so pushed to day.

http://hg.mozilla.org/releases/comm-beta/rev/d57ca248d65f

Comment 49

[Kent James (:rkent)]

Backed out version with string changes:

http://hg.mozilla.org/releases/comm-beta/rev/aa4e7f50b9c8

Pushed version without string changes:

http://hg.mozilla.org/releases/comm-beta/rev/362678f780fb

Sorry about any confusion.

Comment 50

[Kent James (:rkent)]

I had mozmill failures for the version with no string changes, so I'm just going to give up on this patch in the current beta.

Backed out: https://hg.mozilla.org/releases/comm-beta/rev/c59dd8bcfad6

Comment 51

[Ton [:Tonnes]]

> +<!ENTITY cmd.clearList.tooltip   "Remove all entries from the list of saved files, except ongoing downloads.">

For reasons described in bug 1163763 comment 39, please avoid using trailing periods in tooltips in general, and use "Removes" and leave out the period here in order to make the tooltip a descriptive answer to "What does it do?" and keep consistentcy with other Mail tooltips (see http://mxr.mozilla.org/comm-aurora/search?string=cmd.clearList.tooltip.)

The current form is an (imperative) instruction to the user and should be avoided.

Comment 52

[:aceman]

Thanks Ton. Can you file a new bug for that and maybe look over all the other tooltips if there are not more problems of this type?

Comment 53

[:aceman]

It seems to me all tooltips in TB/SM use the imperative form. Also not even m-c follows the rule, as seen on the http://mxr.mozilla.org/comm-aurora/search?string=cmd.clearList.tooltip example.
Fixing this all at once doesn't seem to be an option.

Comment 54

[Ton [:Tonnes]]

(In reply to :aceman from comment #53)

Sorry for the delay, I was in hospital for a number of day.

> It seems to me all tooltips in TB/SM use the imperative form.

Are you sure you don’t mean infinitive form instead?

For the record, imperative is not e.g. the "Removes" form, imperative/infinitive is the unnoticable difference in verbs (t least for English) between e.g. "Create a folder" if a program instructs the user to do so, and the same full verb used for actions/buttons/menus as an action being an answer to the question "what would you like (the computer) to do?"

> Also not even
> m-c follows the rule, as seen on the
> http://mxr.mozilla.org/comm-aurora/search?string=cmd.clearList.tooltip
> example.

Right, my point here was just about the trailing period in this new string only, while the verb was in fact OK, but would be exceptional compared to similar strings that use RemoveS instead of "Remove" (even without periods) that should get rid of their s as well.

> Fixing this all at once doesn't seem to be an option.

Not for this string/bug perhaps, but in another one? Or, why not? Any action from me required?

Comment 55

[Thomas D. (:thomas8)]

(In reply to Ton from comment #51)
> > +<!ENTITY cmd.clearList.tooltip   "Remove all entries from the list of saved files, except ongoing downloads.">

Ton, thanks for taking interest and commenting.
 
> For reasons described in bug 1163763 comment 39, please avoid using trailing
> periods in tooltips in general, and use "Removes" and leave out the period
> here in order to make the tooltip a descriptive answer to "What does it do?"
> and keep consistentcy with other Mail tooltips (see
> http://mxr.mozilla.org/comm-aurora/search?string=cmd.clearList.tooltip.)

Your bug 1163763 comment 39 is exactly right, so it's surprising that you go against your own guidelines by recommending "Removes" here, which is clearly the exceptional (wrong) variant which should be avoided.

> The current form is an (imperative) instruction to the user and should be
> avoided.

Indeed, trailing period as in "Remove it." makes this a full sentence, and as such in standard English it can only be an imperative, which is unwanted because it's the wrong kind of interaction because it's the application talking to us, as you say... So it should answer the user's question "What can I do with this button here?" or "What does this button do?" by presenting tooltip answers in infinitive form, e.g. "Remove the selected item", "Save", "Reply to this message".

So we should only remove the trailing dot from this tooltip:
 Ton 2015-10-10 11:55:49 PDT

> +<!ENTITY cmd.clearList.tooltip   "Remove all entries from the list of saved files, except ongoing downloads.">
 Ton 2015-10-10 11:55:49 PDT

Change this to:

+<!ENTITY cmd.clearList.tooltip   "Remove all entries from the list of saved files, except ongoing downloads">

Comment 56

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #53)
> It seems to me all tooltips in TB/SM use the imperative form.

Well, in English imperative and infinitive are identical, but it's really the infinitive, and rightly so (see my comment 55). Imperative would require a trailing exclamation mark or at least a full stop.

Comment 57

[:aceman]

(In reply to Ton from comment #54)
> Right, my point here was just about the trailing period in this new string
> only, while the verb was in fact OK, but would be exceptional compared to
> similar strings that use RemoveS instead of "Remove" (even without periods)
> that should get rid of their s as well.

Sorry I do not understand what you say. So which form it the correct one in the global picture? You propose to change it to "removes" and giving example that other strings are like that, but then say those strings are bad. So why should I make the string worse temporarily?

> > Fixing this all at once doesn't seem to be an option.
> Not for this string/bug perhaps, but in another one? Or, why not? Any action
> from me required?

Because when all of the strings are bad, it would produce changes at all callers and all over the code.

Comment 58

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #57)
> Sorry I do not understand what you say. So which form it the correct one in
> the global picture? You propose to change it to "removes" and giving example
> that other strings are like that, but then say those strings are bad. So why
> should I make the string worse temporarily?

Aceman, don't let Ton's comments confuse you.
Just scrap Ton's comment 51, he got himself confused in that comment and recommended the wrong thing.

This is the global picture:

Correct tooltip (shortened example; errors marked as *-error-*):
a) "Remove all entries" (Infinitive; no trailing period)

Wrong tooltips:
b) "Removes all entries" (*-3rd Person Singular with "s"-*; no trailing period)
c) "Remove all entries." (Infinitive/*-Imperative-*; *-trailing period-*)
Note that in c), the trailing dot "." actually changes the verb to become an imperative, which is semantically wrong in the communicative context of tooltips.

Only a) is correct tooltip.
- So most of the tooltips in this list are wrong because they are using variant b) "Removes" (with "s"), which should be fixed:
http://mxr.mozilla.org/comm-aurora/search?string=cmd.clearList.tooltip
- The tooltip we added in this bug is wrong because it has a trailing period (variant c), which changes the meaning of the verb to be in imperative form (application telling user "Do this!"), which is obviously incorrect.

Comment 59

[Ton [:Tonnes]]

Thomas, thanks for understanding.

(In reply to :aceman from comment #57)
> 
> Sorry I do not understand what you say. So which form it the correct one in
> the global picture? You propose to change it to "removes" and giving example
> that other strings are like that, but then say those strings are bad. So why
> should I make the string worse temporarily?

Sorry, I didn’t confuse myself but maybe others by combining the general rule and quick "fix" for this string only in one sentence, so probably should have written "… to at least keep consistency with *some* or *similar* Mail tooltips before changing all of them…" (the ones indicated by the mxr link) in comment 51. I should have mentioned the others are "bad" and be clearer on why I went "against my own guidelines" (which aren’t really mine of course.) ;)

Like Thomas confirmed above, the others are all bad, but since this bug is only about the recent one that properly uses "Remove" - but has a period - I thought there was no point in suggesting to change all of them to the proper fotm here. Also, removing the period seems more important than removing the s in others, hence the quick fix proposal.

So in short (already pointed out by Thomas above - I wrote this last night): in the global picture, any tooltip should use full verbs and no trailing periods, unless there are 2 or more sentences involved, which is very rare. (Note that even in such cases, the first sentence often has a trailing period and the second one doesn’t - not implying this is right.) Please keep in mind that localizers may depend on the presence of trailing periods in en-US as a rule of thumb to decide if imperative or infinitive mood is meant in English and choose their verbs, causing errors here and there until today. Considering that, a period can do more harm in English because of its similar verbs.

So if we want to fix all occurrences mentioned above, the 3 for Mail (TB/SM) and 2 others (toolkit/webapprt) in the mxr link would need to be changed just to use "Remove" and no periods, that should be clear now. :) I haven’t checked for any other tooltips containing periods in Mail or other components so far but don’t think there are many more in Mail, or even none at all. Devtools was/is the component where the issue occurs most, hence the reference to bug 1163763 that caused a few of them.

> > > Fixing this all at once doesn't seem to be an option.
> > Not for this string/bug perhaps, but in another one? Or, why not? Any action
> > from me required?
> 
> Because when all of the strings are bad, it would produce changes at all
> callers and all over the code.

Do you mean entity changes? I agree if so, and it was also a reason to only mention the new string now it’s in aurora. Maybe it wouldn’t even need an entity change because of that and sending out an l10n note would suffice. Fixing the others can be done in a separate bug and another cycle, provided there are only 4 others to fix.
I do think some action is needed some day (though no rush is involved), knowing voices on documenting all this as a guideline on MDN came up in bug 1163763 comment 41. A global search and fixes were needed anyway to meet the guideline and catch remaining occurrences in the tree, so these 4 could become part of it if that’s easier, but rather not I suppose.

Comment 60

[:aceman]

(In reply to Ton from comment #59)
> Do you mean entity changes? I agree if so, and it was also a reason to only
> mention the new string now it’s in aurora. Maybe it wouldn’t even need an
> entity change because of that and sending out an l10n note would suffice.
> Fixing the others can be done in a separate bug and another cycle, provided
> there are only 4 others to fix.

Yes, because I thought you say we need to change the "remove" form to "removes" and then all tooltips in c-c would need such a change. If only 4 are needed then please file the bug and I am sure Thomas can fix it quickly.
http://mxr.mozilla.org/comm-central/search?string=.tooltip+&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central . But I only see one problem there.