726344 - add tooltiptext to more (addresses) indicator in the header pane

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, enhancement)

Description

[Joachim Herb]

Attachment: Screenshot of the compactheader implementation

Add a tooltiptext to the more (addresses) indicator in the header pane for the to, cc bcc fields. See the attached screenshot of the implementation for the two line compact header view if the CompactHeader addon is installed (version 2.0.2beta2).

The addon version includes all addresses in the tooltip text. It might make more sense to only show the hidden addresses (but it's harder to implement in the addon).

Any interest to add this feature to Thunderbird core?

Comment 1

[rsx11m]

Sounds useful if you quickly want to see who else is on the list but don't want to open the list in full and clog the headers. A potential problem may be if you just happen to hover of the button by chance and the tooltip shows up. Let's assume there are 200 addresses in the list as a worst-case scenario, that would be a big tooltip. Thus, some size restriction probably would be needed.

Comment 2

[:aceman]

Maybe the addon author could implement it if the TB developers/UI people say this would be accepted.

Comment 3

[Joachim Herb]

Attachment: First patch for adding a tooltiptext to the more widget

I am not sure that I used the correct email address (fullAddress/displayName) for the tooltiptext. I copied the code from http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWidgets.xml#520

Also I am not sure whom to ask for ui-/review.

If there is a change to get this into Thunderbird, I will also supplied mozmill tests.

Comment 4

[Joachim Herb]

Attachment: Screenshot of the first patch implementation showing a few (hidden) email addresses

Comment 5

[Joachim Herb]

Attachment: Screenshot of the first patch implementation showing many (hidden) email addresses.

Comment 6

[:aceman]

:bwinton can do ui-reviews, but in this case (Message reader UI) he probably does reviews too (https://wiki.mozilla.org/Modules/Thunderbird).

Comment 7

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 598552 [details] [diff] [review]
First patch for adding a tooltiptext to the more widget

Yeah, I might as well add these to the pile…  ;)

(I'm hoping to get to them on Tuesday, since Monday is a holiday.)

Thanks,
Blake.

Comment 8

[Joachim Herb]

This is a (partly pending) try server build:
http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=cb254fcc8402

Comment 9

[Joachim Herb]

Here is another try server build for all OS:
http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=2e83c1e54744

Comment 10

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 598552 [details] [diff] [review]
First patch for adding a tooltiptext to the more widget

That feels like a great idea.  I don't know how many people will discover it, but for those who do, it seems very useful!  ui-r=me!

The implementation seems reasonable, although I wonder if you could do it without the extra array.  Enh, r=me either way.  ;)

Thanks,
Blake.

Comment 11

[Jim Porter (:squib)]

Could we clamp the number of items in the tooltip? (Possibly by number of characters instead of number of addresses). Huge tooltips can be pretty disruptive if you accidentally trigger them while trying to do something else, especially since our tooltips don't automatically time out.

Comment 12

[Joachim Herb]

Attachment: Patch without additional array and tooltiptext length limit

Thank you for the reviews for the first patch.

Would this be a better solution? Is a preference setting the right way to set the tooltiptext length limit? What would be its name?

Comment 13

[Jim Porter (:squib)]

Comment on attachment 601777 [details] [diff] [review]
Patch without additional array and tooltiptext length limit

I don't think a pref makes much sense here, since we don't need prefs to tweak every little thing. You could make the max length a read/write property on the XBL binding to make it easier to override in an extension, though. Also, is there ever a case where fullAddress isn't defined? Do we even need to fall back to displayName?

More generally, I'd structure the code something like:

  if (aAddresses.length == 0)
    return;

  let ttText = aAddresses[0].fullAddress;
  for (let i = 1; i < aAddresses.length; i++) {
    if (i >= maxLength) {
      ttText += ", and " + (maxLength - i) + " more"; // XXX: use l10n here
      break;
    }

    ttText += ", " + aAddresses[i].fullAddress;
  }
  this.more.setAttribute("tooltiptext", ttText);

Also, you've got a typo: "maxTTTextLenght".

Comment 14

[Joachim Herb]

Attachment: Redesign of code following comment #13

Comment 15

[Jim Porter (:squib)]

Comment on attachment 609155 [details] [diff] [review]
Redesign of code following comment #13

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

We should probably have tests for this. r=me with the comments below fixed, some tests added, and a successful try run for the tests.

::: mail/base/content/mailWidgets.xml
@@ +784,5 @@
>        </method>
>  
> +      <!-- This field is used to specify the maximum number of addresses in the more 
> +           button tooltip text. -->
> +      <field name="tooltipLength">50</field>

I think 50 is a little much. Maybe 20?

@@ +807,5 @@
> +                                    .getString("headerMoreAddrs");
> +                let remainingAddresses = (aAddresses.length - i);
> +                let moreForm = PluralForm.get(remainingAddresses, words).replace("#1",
> +                                                                      remainingAddresses);
> +                ttText += " and " + moreForm;

This needs to be localized, and maybe should have a comma before it (not sure it really matters). You might want to make a single string of the form ", and NN more" instead of using the "headerMoreAddrs" string.

Comment 16

[Joachim Herb]

(In reply to Jim Porter (:squib) from comment #15)
> @@ +807,5 @@
> > +                                    .getString("headerMoreAddrs");
> > +                let remainingAddresses = (aAddresses.length - i);
> > +                let moreForm = PluralForm.get(remainingAddresses, words).replace("#1",
> > +                                                                      remainingAddresses);
> > +                ttText += " and " + moreForm;
> 
> This needs to be localized, and maybe should have a comma before it (not
> sure it really matters). You might want to make a single string of the form
> ", and NN more" instead of using the "headerMoreAddrs" string.

As I have no experience with localization: Does this mean I have to add a plural form of this string to messenger.properties like http://mxr.mozilla.org/comm-central/source/mail/locales/en-US/chrome/messenger/messenger.properties#536
What else do I have to do for this?

Would it be acceptable to use the English version hard coded in a mozmill test?

Comment 17

[:aceman]

Yes, that link is a good place you can see how the plurals work. See also http://developer.mozilla.org/en/Localization_and_Plurals .

Comment 18

[Joachim Herb]

Attachment: Patch including mozmill tests

This patch uses the same code for the functionality as the previous (reviewed) one. Only the number of addresses shown in the tooltip text has been changed to 20. 

It adds a new localization string for the ", and X more" text. I have included the comma to the string, because e.g. in German there is no comma "<[e-mail]> und X weitere".

Also there are mozmill tests now. I guess somebody has to review them. As described in bug 743075 the assert_element_visible does not work correctly if a parent element of a DOM element is collapsed/hidden. Therefore I used the old function isVisible from test-screen-helpers and moved it in test-dom-helpers.

The mozmill tests check:
- The number before the more widget (actually is it called widget or button?). This test is missing until now.
- The addresses in the tooltip text.
- The ", and X more" text shows the correct number.

Comment 19

[Joachim Herb]

Forgot:
The patch did run on the try server:
http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=ec1de1b2eb0b
There were some test fails on Windows (related to the more button). But then I pushed the same patch again, just for Windows (because it always worked on my local Windows machine), and it worked:
http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=1d9ee2af03ab
So I guess the tests are ok. The sporadic failed test might be related to bug 647211.

Comment 20

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 619630 [details] [diff] [review]
Patch including mozmill tests

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

Hey Joachim,

Sorry it took a while to get to this.  This is generally pretty good looking - I just found a couple of small problems, and have a few suggestions.

-Mike

::: mail/base/content/mailWidgets.xml
@@ +807,5 @@
> +            if (aAddresses.length == 0)
> +              return;
> +
> +            let ttText = aAddresses[0].fullAddress;
> +            for (let i = 1; i < aAddresses.length; i++) {

I'm not the biggest fan of this for-loop, with the break in the conditional, etc.

How about instead, we do the following:

Copy aAddresses into an array X that we can safely add / remove elements from without affecting the caller.
If aAddresses.length > tooltipLength:
  Splice out the values after tooltipLength
  Add the "moreForm" PluralForm'd string to the end of the array X

Join the array X using String.join(", ")

@@ +819,5 @@
> +                ttText += moreForm;
> +                break;
> +              }
> +
> +              ttText += ", " + aAddresses[i].fullAddress;

Does this separator need to be l10n-able?

::: mail/test/mozmill/message-header/test-message-header.js
@@ +80,5 @@
>  
> +  // create a message that has the more to and cc addresses than visible
> +  // in the tooltip text of the more button
> +  let msgMore1 = create_message({to: msgGen.makeNamesAndAddresses(40),
> +                                  cc: msgGen.makeNamesAndAddresses(40)});

Nit: the cc: should be lined up beneath the to:

@@ +86,5 @@
> +
> +  // create a message that has the more to and cc addresses than visible
> +  // in the header
> +  let msgMore2 = create_message({to: msgGen.makeNamesAndAddresses(20),
> +    cc: msgGen.makeNamesAndAddresses(20)});

Same as above - the cc: should line up under the to:

@@ +484,5 @@
>    subtest_more_widget_display(toDescription);
>    subtest_more_widget_click(toDescription);
>    subtest_more_widget_star_click(toDescription);
> +
> +  Services.prefs.clearUserPref("mailnews.headers.show_n_lines_before_more");

If you change prefs in a test, you should change it back to its original state once the test completes.

@@ +789,5 @@
> + * not shown in the header and the number of addresses also hidden in the
> + * tooltip text.
> + */
> +function subtest_more_button_tooltip(aMsg) {
> +  let headerParser = Cc["@mozilla.org/messenger/headerparser;1"]

You should be able to get this from MailServices.headerParser.  You can import mailServices.js with:

Components.utils.import("resource:///modules/mailServices.js");

Then you can just use MailServices.headerParser below.

@@ +829,5 @@
> +function get_number_of_addresses_in_header(aHeaderBox) {
> +  let headerBoxElement = mc.a(aHeaderBox, {class: "headerValue"});
> +  let addrs = headerBoxElement.getElementsByTagName('mail-emailaddress');
> +  let addrNum = 0;
> +  for (let i = 0; i<addrs.length; i++) {

Nit: space on either side of <

@@ +832,5 @@
> +  let addrNum = 0;
> +  for (let i = 0; i<addrs.length; i++) {
> +    // check that the address is really visible and not just a cached
> +    // element
> +    if (element_visible_recursive(addrs[i])) {

Nit: You don't need the opening/closing braces for this if-block, since its contents is a one-liner.

@@ +836,5 @@
> +    if (element_visible_recursive(addrs[i])) {
> +      addrNum += 1;
> +    }
> +  }
> +  return addrNum

Nit: missing semicolon

@@ +863,5 @@
> +  // check for more indicator number of the more widget
> +  let addresses = {};
> +  let fullNames = {};
> +  let names = {};
> +  let headerParser = Cc["@mozilla.org/messenger/headerparser;1"]

Same as above - you can just use MailServices.headerParser.

@@ +887,5 @@
> +  else {
> +    assert_equals(maxTooltipAddrsNum, addrsNumInTooltip);
> +    // check if ", and X more" shows the correct number
> +    let moreTooltipSplit = tooltipText.split(", ");
> +    let expectedText = "and " +

We should try to avoid making our tests work only in English locales.

::: mail/test/mozmill/shared-modules/test-window-helpers.js
@@ +1739,4 @@
>    function subrenderCandidates(aElements) {
>      for (let i = 0; i < aElements.length; i++) {
>        let elem = aElements[i];
>        if (isVisible(elem)) {

This function will now fail because isVisible is no longer defined.

Comment 21

[Joachim Herb]

Hi Mike,

thank you for your review:
(In reply to Mike Conley (:mconley) from comment #20)
> Comment on attachment 619630 [details] [diff] [review]
> Patch including mozmill tests
> 
> Review of attachment 619630 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mail/base/content/mailWidgets.xml
> @@ +807,5 @@
> > +            if (aAddresses.length == 0)
> > +              return;
> > +
> > +            let ttText = aAddresses[0].fullAddress;
> > +            for (let i = 1; i < aAddresses.length; i++) {
> 
> I'm not the biggest fan of this for-loop, with the break in the conditional,
> etc.
> 
> How about instead, we do the following:
> 
> Copy aAddresses into an array X that we can safely add / remove elements
> from without affecting the caller.
> If aAddresses.length > tooltipLength:
>   Splice out the values after tooltipLength
>   Add the "moreForm" PluralForm'd string to the end of the array X
This is what I did in the first patch:
https://bugzilla.mozilla.org/attachment.cgi?id=598552&action=diff#a/mail/base/content/mailWidgets.xml_sec2
In comment #10 :bwinton suggested to do the patch without an additional array. So which way should I go?

> > +              ttText += ", " + aAddresses[i].fullAddress;
> 
> Does this separator need to be l10n-able?
I have no idea? Whom should I ask?

> > +  Services.prefs.clearUserPref("mailnews.headers.show_n_lines_before_more");
> 
> If you change prefs in a test, you should change it back to its original
> state once the test completes.
Ok. Actually is there a way to do this at the end of a test even if it fails?

> @@ +887,5 @@
> > +  else {
> > +    assert_equals(maxTooltipAddrsNum, addrsNumInTooltip);
> > +    // check if ", and X more" shows the correct number
> > +    let moreTooltipSplit = tooltipText.split(", ");
> > +    let expectedText = "and " +
> 
> We should try to avoid making our tests work only in English locales.
How to do this? Can (and should) I use the PluralForm.get(remainingAddresses, words).replace("#1", remainingAddresses) also in the mozmill test?

> ::: mail/test/mozmill/shared-modules/test-window-helpers.js
> @@ +1739,4 @@
> >    function subrenderCandidates(aElements) {
> >      for (let i = 0; i < aElements.length; i++) {
> >        let elem = aElements[i];
> >        if (isVisible(elem)) {
> 
> This function will now fail because isVisible is no longer defined.
This function is actually commented out in that file. For the case that it is reactivated again, then I have added the commented above mentioning the renaming and moving it to a different file (dom-helper) of isVisible. Or should I move the new (renamed) function element_visible_recursive to test-window-helpers.js?

Comment 22

[Joachim Herb]

(In reply to Joachim Herb from comment #21)
> > @@ +887,5 @@
> > > +  else {
> > > +    assert_equals(maxTooltipAddrsNum, addrsNumInTooltip);
> > > +    // check if ", and X more" shows the correct number
> > > +    let moreTooltipSplit = tooltipText.split(", ");
> > > +    let expectedText = "and " +
> > 
> > We should try to avoid making our tests work only in English locales.
> How to do this? Can (and should) I use the
> PluralForm.get(remainingAddresses, words).replace("#1", remainingAddresses)
> also in the mozmill test?
This version works in the mozmill test:
    let remainingAddresses = numAddresses - aShownAddrsNum - maxTooltipAddrsNum;
    let moreForm = mc.window.PluralForm.get(remainingAddresses, words)
                            .replace("#1", remainingAddresses);
    assert_equals(moreForm, ", " + moreTooltipSplit[moreTooltipSplit.length - 1]);
Is it ok to do it this way?

Comment 23

[Joachim Herb]

Attachment: Changes requested by reviewer

This patch should include all changes requested by the reviewer.

It ran on the try-server:
https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=2a9786c84541
https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=b6ebe719f315

The failed mozmill tests are not related to this patch (hopefully).

Comment 24

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 622138 [details] [diff] [review]
Changes requested by reviewer

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

This looks really good - I think one more iteration will do it.

::: mail/base/content/mailWidgets.xml
@@ +806,5 @@
> +          <![CDATA[
> +            if (aAddresses.length == 0)
> +              return;
> +
> +            let tttArray = new Array;

I'd prefer:

let tttArray = [];

@@ +808,5 @@
> +              return;
> +
> +            let tttArray = new Array;
> +            let i;
> +            for (i = 0; (i < aAddresses.length) && (i < this.tooltipLength); i++) {

Instead of declaring i outside of the for loop, declare it inline.  We don't need i elsewhere (see my comment on line 817 below)

@@ +813,5 @@
> +              tttArray.push(aAddresses[i].fullAddress);
> +            }
> +            let ttText = tttArray.join(", ");
> +
> +            let remainingAddresses = aAddresses.length - i;

Instead of using i, we could use tttArray.length

::: mail/test/mozmill/message-header/test-message-header.js
@@ +48,4 @@
>  
>  var elib = {};
>  Cu.import('resource://mozmill/modules/elementslib.js', elib);
> +Components.utils.import("resource:///modules/mailServices.js");

Just use Cu, like line 50.

@@ +78,5 @@
>      }});
>  
>    add_message_to_folder(folder, gInterestingMessage);
>  
> +  // create a message that has the more to and cc addresses than visible

Nit - typo, "the more"

@@ +84,5 @@
> +  let msgMore1 = create_message({to: msgGen.makeNamesAndAddresses(40),
> +                                 cc: msgGen.makeNamesAndAddresses(40)});
> +  add_message_to_folder(folderMore, msgMore1);
> +
> +  // create a message that has the more to and cc addresses than visible

Same typo as above.

@@ +790,5 @@
> +/**
> + * Test if the tooltip text of the more widget contains the correct addresses
> + * not shown in the header and the number of addresses also hidden in the
> + * tooltip text.
> + */

You should have a @param line to explain what aMsg is.

@@ +824,5 @@
> +}
> +
> +/**
> + * Return the number of addresses visible in headerBox.
> + */

You should have a @param line to explain what aHeaderBox is.

@@ +840,5 @@
> +}
> +
> +/**
> + * Return the number shown in the more widget.
> + */

Like above, needs @param

@@ +855,5 @@
> +}
> +
> +/**
> + * Check if hidden addresses are part of more tooltip text.
> + */

Needs @param lines.

Comment 25

[Joachim Herb]

Attachment: More changes requested by the reviewer

The patch build at the try server:
http://build.mozillamessaging.com/tinderboxpushlog?tree=ThunderbirdTry&rev=9455c0364f7a
http://build.mozillamessaging.com/tinderboxpushlog?tree=ThunderbirdTry&rev=9455c0364f7a
http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/[e-mail]-9455c0364f7a
(somehow the front end of the server is broken, but it looks like the mozmill tests related to this patch passed).

Comment 26

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 624168 [details] [diff] [review]
More changes requested by the reviewer

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

Small nit, but otherwise, this looks good.

Thanks for your work, Joachim!

::: mail/base/content/mailWidgets.xml
@@ +798,5 @@
> +                onset="return this.tooltipLength=val;"
> +                onget="return this.tooltipLength;"/>
> +
> +      <!-- Populate the tooltiptext of the (N more) widget with hidden email
> +           adddresses. -->

typo nit: adddresses -> addresses

Comment 27

[Joachim Herb]

Attachment: Final cleanup

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Thanks for the patch, Joachim! One request - for future patches, please follow the directions below to make sure that they contain all the needed metadata in them for checkin. It makes life a lot easier for those checking in on your behalf. I've already taken care of adding it to this patch. Thanks!
https://developer.mozilla.org/en/Creating_a_patch_that_can_be_checked_in

The tree is currently closed, but I will land it as soon as it reopens.

Comment 29

[Joachim Herb]

Attachment: Patch with metadata

Just to check that I get everything correct the next time: Does the attached patch contains all necessary metadata?

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/7638a8ceab5b

Sorry, I missed your last reply. Yes, that information looks good for next time. Thanks again!