Closed Bug 954653 Opened 7 years ago Closed 7 years ago

Redesign buddy tooltips

Categories

(Instantbird :: Contacts window, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: florian, Assigned: wnayes)

References

Details

Attachments

(1 file, 8 obsolete files)

*** Original post on bio 1221 at 2012-01-02 22:59:00 UTC ***

The current tooltips look awful. I think a relatively easy redesign could dramatically improve them.
My idea is to make them look like the status selector at the top of the contact list and like the conv-top-info toolbar at the top of conversation tabs.
This means:
- Contact icon on the left with the status icon overlaying it in the bottom right corner.
- The display name with a relatively large font
- An horizontal line
- The status text (or topic for MUCs) on a second line, with a smaller font.
- (maybe) The protocol icon of the most available buddy at the right end of the first line.

Benefits:
- better information density
- better readability
- more polished appearance
- consistency with other parts of the application. The layout I'm proposing is already used in 2 places of our UI, it could become distinctive for Instantbird.
Duplicate of this bug: 954443
Whiteboard: [1.3-wanted]
*** Original post on bio 1221 at 2012-11-02 00:12:21 UTC ***

Too late to work on this for 1.3, replacing 1.3-wanted by 1.4-wanted in the whiteboard.
Whiteboard: [1.3-wanted] → [1.4-wanted]
Whiteboard: [1.4-wanted]
*** Original post on bio 1221 as attmnt 2541 at 2013-06-29 23:24:00 UTC ***

Here's a patch that makes the buddy tooltips quite similar to the existing UI, as described in the first comment.
Attachment #8354309 - Flags: review?(florian)
Assignee: nobody → wnayes
Status: NEW → ASSIGNED
*** Original post on bio 1221 at 2013-06-30 14:21:05 UTC ***

Thanks a lot! I've tried the patch and it's looking really good already!

I've noticed the following things though:
- the protocol icons for buddies of a contact are distorted,
- the row with the status text can be missing for some contacts and their display name and separator are moved down into its place instead,
- the paddings or margins on the left hand side will need tweaking (at least on Windows). I'd place the text flush with the left end of the icon itself while the separators would extend a few pixels further and end at the same distance as the icon's outer end of its border.

I'd also add a separator (or an unobtrusive border) between the top section of the tooltip which contains the icon, name and status message and the bottom with the detailed information.
Comment on attachment 8354309 [details] [diff] [review]
Buddy tooltip design closely based on existing UI

*** Original change on bio 1221 attmnt 2541 at 2013-07-02 22:19:14 UTC ***

If you moved the .statusTypeIcon[status=*] rules from http://lxr.instantbird.org/instantbird/source/instantbird/themes/conversation.css#76 to status.css, and imported status.css from conversation.css, would you be able to continue using status.css for the tooltip instead of adding more rules for the status icons?


>@@ -268,17 +281,20 @@
>        <body>
>        <![CDATA[
>          while (aTooltipInfo.hasMoreElements()) {
>            var elt =
>              aTooltipInfo.getNext().QueryInterface(Ci.prplITooltipInfo);
>            switch (elt.type) {
>              case Ci.prplITooltipInfo.pair:
>              case Ci.prplITooltipInfo.sectionHeader:
>-               this.addRow(elt.label, elt.value);
>+               if (elt.label == this.bundle.GetStringFromName("buddy.status"))
>+                 this.setMessage(elt.value);

What is this test doing (or attempting to do)?


Removing the review request because it seems Mic already gave several comments that need to be addressed in comment 4, but I really look forward to having this in nightlies! :-)
Attachment #8354309 - Flags: review?(florian)
*** Original post on bio 1221 at 2013-07-03 21:41:50 UTC ***

(In reply to comment #5)
> Comment on attachment 8354309 [details] [diff] [review] (bio-attmnt 2541) [details]
> Buddy tooltip design closely based on existing UI
> 
> If you moved the .statusTypeIcon[status=*] rules from
> http://lxr.instantbird.org/instantbird/source/instantbird/themes/conversation.css#76
> to status.css, and imported status.css from conversation.css, would you be able
> to continue using status.css for the tooltip instead of adding more rules for
> the status icons?

Most of the work to address this comment has already been handled in bug 955468 (bio 2031).
Depends on: 955468
Attached patch Layout changes for new tooltips (obsolete) — Splinter Review
*** Original post on bio 1221 as attmnt 2554 at 2013-07-04 02:42:00 UTC ***

This patch addresses most of the feedback from the first.

- Protocol icons are no longer distorted.
- Mic's padding advice should be in place.
- The status.css additions are no longer duplicated in this patch.

> > + if (elt.label == this.bundle.GetStringFromName("buddy.status"))
> > +   this.setMessage(elt.value);
> What is this test doing (or attempting to do)?

The only way I have found so far to retrieve the status text of the user was to catch it during the iteration over the getTooltipInfo. Instead of making a new grid row with the status, it then gets placed in the layout header underneath the name.

To determine what the status is, I have been testing whether the label of each element is "Status" or not. This wasn't 100% effective in the last patch (sometimes the label can be "Status (SOME_ID_VALUE?)" so for this patch I have hopefully fixed at least that case by checking using startsWith rather than ==.

> I'd also add a separator (or an unobtrusive border) between the top
> section of the tooltip which contains the icon, name and status message
> and the bottom with the detailed information.

I tried this out and the tooltips without the icon in the header did not look good having the existing display name line with the separator right underneath. There could be ways to script hiding and showing a separator but I didn't bother for now.
Attachment #8354322 - Flags: review?(florian)
Comment on attachment 8354309 [details] [diff] [review]
Buddy tooltip design closely based on existing UI

*** Original change on bio 1221 attmnt 2541 at 2013-07-04 02:42:50 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354309 - Attachment is obsolete: true
*** Original post on bio 1221 at 2013-07-04 06:52:13 UTC ***

(In reply to comment #7)

> > > + if (elt.label == this.bundle.GetStringFromName("buddy.status"))
> > > +   this.setMessage(elt.value);
> > What is this test doing (or attempting to do)?
> 
> The only way I have found so far to retrieve the status text of the user was to
> catch it during the iteration over the getTooltipInfo. Instead of making a new
> grid row with the status, it then gets placed in the layout header underneath
> the name.
> 
> To determine what the status is, I have been testing whether the label of each
> element is "Status" or not. This wasn't 100% effective in the last patch
> (sometimes the label can be "Status (SOME_ID_VALUE?)" so for this patch I have
> hopefully fixed at least that case by checking using startsWith rather than ==.

You really don't want to do that kind of tests on strings that are localized, and may not be looking like you expect on other locales.

From the method updateTooltipFromBuddy, you should be able to get the status text with: aBuddy.statusText
*** Original post on bio 1221 as attmnt 2593 at 2013-07-13 18:23:00 UTC ***

This patch removes the (attempted) placement of the status within the header description. Instead, the status remains in the tooltip table rows where it has been. The conversation topic does appear in the header, however.

I tried to add the status to the header using aBuddy.statusText, and it did work. The issue was that it would look odd when a long status wrapped. The other headers in the buddy list and conversation window handle this by cutting off the text at one line, but this didn't make sense for a tooltip that needs to show the entire status. So for now, the header description just has the "Available" or "Offline" labels and the status remains where it is.
Attachment #8354362 - Flags: review?(florian)
Comment on attachment 8354322 [details] [diff] [review]
Layout changes for new tooltips

*** Original change on bio 1221 attmnt 2554 at 2013-07-13 18:23:49 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354322 - Attachment is obsolete: true
Attachment #8354322 - Flags: review?(florian)
Comment on attachment 8354362 [details] [diff] [review]
Tooltips with the status in the existing placement

*** Original change on bio 1221 attmnt 2593 at 2013-07-14 15:21:05 UTC ***

>   <binding id="tooltip" extends="chrome://global/content/bindings/popup.xml#tooltip">

>+    <content noautohide="true" orient="vertical">
>+      <xul:hbox align="start" crop="end" flex="1">
>+        <xul:stack>
>+          <xul:image id="tooltipUserIcon" anonid="userIcon"/>
>+          <xul:image id="tooltipStatusIcon" class="statusTypeIcon"

XUL code in XBL binding should never us id attributes. ids are supposed to be unique across the whole document, and an XBL binding can be instantiated several times.

You seem to only be using these ids in the CSS, so you can likely use the class attribute instead.

For this second image, you already have a statusTypeIcon class, is there any reason why you can't just use it in the CSS? (the buddytooltip.css file used only in this binding, so all rules defined there are scoped to this binding; you don't have to be afraid of conflicts with other code.)


>      <method name="setBuddyIcon">

>          if (aSrc) {

>-           img.parentNode.collapsed = false;
>+           img.parentNode.style.display = "-moz-stack";
>          }
>          else
>-           img.parentNode.collapsed = true;
>+           img.parentNode.style.display = "none";

Why are you touching the image's parent node? With your new XUL code, the image's parent is the stack that also contains the status icon.

Also, what's wrong with the collapsed XUL property? (I suspect it's faster than setting a value in .style, but I may be wrong on that.)


>      <handler event="popuphiding">
>        <![CDATA[
>        this.buddy = null;
>        this.contact = null;
>        this.elt = null;
>+       this.setMessage("");
>+       this.setBuddyIcon(null);

Why do you need this?

The goal of the existing code running in this popuphiding handler is to drop all references to XPCOM objects to avoid leaking them until the window is closed.

>diff --git a/instantbird/themes/buddytooltip.css b/instantbird/themes/buddytooltip.css

>+#tooltipDisplayName {
>+  border-bottom: 1px solid rgba(0,0,0,0.25);

Nit: add a space after each comma. (This applies in all CSS files, unless there are already plenty of CSS rules with a different coding style, in which case we just match the existing style when adding more rules to the file.)
Attachment #8354362 - Flags: review?(florian) → review-
*** Original post on bio 1221 as attmnt 2609 at 2013-07-19 01:45:00 UTC ***

This patch should fix all of the comments from the previous one. The main visual change in this patch is that the status icon now remains in view regardless of the buddy icon presence (similar to how conversations currently display).

Since a rule for .statusTypeIcon with "mobile" was not in status.css, I added it to the tooltip css file for now. It wasn't clear whether or not this was missing from status.css for a reason.
Attachment #8354378 - Flags: review?(florian)
Comment on attachment 8354362 [details] [diff] [review]
Tooltips with the status in the existing placement

*** Original change on bio 1221 attmnt 2593 at 2013-07-19 01:45:53 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354362 - Attachment is obsolete: true
Comment on attachment 8354378 [details] [diff] [review]
New layout with status icon present without buddy icon.

*** Original change on bio 1221 attmnt 2609 at 2013-07-19 15:42:45 UTC ***

All my code comments are pretty trivial (I still haven't tried the patch locally, but if there's no surprise and it looks like on the screenshots I saw, the next version of the patch should be ready to land! :)).

>+         if (aElt.hasAttribute("status")) {
>            this.setAttribute("status", aElt.getAttribute("status"));
>+           this.setMessage(Status.toLabel(aElt.getAttribute("status")));
>+         }

Nit: Add a |let status = aElt.getAttribute("status");| line here to reduce the duplication.


>-         if (aConv.topic)
>-           this.addRow(this.bundle.GetStringFromName("conversation.topic"), aConv.topic);
>+         this.setMessage(aConv.topic ? aConv.topic : "");

aConv.topic || ""



>+.userIcon[src] + .statusTypeIcon {
>+  margin-left: 36px;
>+}
>+
>+.userIcon:not([src]) + .statusTypeIcon {
>+  margin-left: 6px;
>+}

Can we simplify this to:
.statusTypeIcon {
  margin-left: 6px;
}

.userIcon[src] + .statusTypeIcon {
  margin-left: 36px;
}

If the answer is yes, then you can merge the first rule with the other .statusTypeIcon stuff that's further up in the file, and put the second rule next to it.

If not you may still be able to simplify ".userIcon:not([src]) + .statusTypeIcon" to ".userIcon + .statusTypeIcon"


I don't see a reason for not having the mobile rule in status.css, so it's possible it has just been forgotten.
Attachment #8354378 - Flags: review?(florian) → review-
*** Original post on bio 1221 as attmnt 2614 at 2013-07-19 22:51:00 UTC ***

I tried the patch on my Mac debug build. I'm attaching a screenshot of what I saw.

There are a few theming issues (missing margin after the buddy icon, missing margin left/right of the underline under the display name). When the buddies get listed, they are animated, and while the buddy rows expand, the buddy icon shrinks (you can see on my screenshot that it's no longer a square).
The green dot and "Available" are also not correctly aligned vertically.
Attached image Screenshot without buddy icon (obsolete) —
*** Original post on bio 1221 as attmnt 2615 at 2013-07-19 22:54:00 UTC ***

- The status line seems missing.
- The first and second lines of text should be aligned.
Attached patch Progress on CSS fixes. (obsolete) — Splinter Review
*** Original post on bio 1221 as attmnt 2627 at 2013-07-23 02:12:00 UTC ***

I'm marking this for feedback as there is an issue or two from the last review that probably was not fixed by the margin changes in this patch.

> When the buddies get listed, they are animated, and while the
> buddy rows expand, the buddy icon shrinks (you can see on my
> screenshot that it's no longer a square).

My best guess for fixing this was to ensure min-height and min-width on the userIcon and statusTypeIcon. Besides this, I have explicitly defined several description margins to hopefully keep things the same across platforms.

> The green dot and "Available" are also not correctly aligned vertically.

This was also not explicitly addressed beyond specifying the margins of the text elements. If it isn't fixed, I can't think of a way other than making a larger top margin for Mac.

> The status line seems missing.

I took a look at the ChatBuddy interfaces and didn't see any way of getting status. For now, I am directly assigning "unknown" status for participants.

> The first and second lines of text should be aligned.

Instead of matching every element to the grid row margin-left, I added a rule to force that margin to 0. This made positioning the other elements simpler and hopefully resolved this issue.

Besides these CSS formatting attempts, the code comments were fixed and the mobile rule added.
Attachment #8354396 - Flags: feedback?(florian)
Comment on attachment 8354378 [details] [diff] [review]
New layout with status icon present without buddy icon.

*** Original change on bio 1221 attmnt 2609 at 2013-07-23 02:12:39 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354378 - Attachment is obsolete: true
Comment on attachment 8354396 [details] [diff] [review]
Progress on CSS fixes.

*** Original change on bio 1221 attmnt 2627 at 2013-08-17 01:20:59 UTC ***

(In reply to comment #15)

Looking very promising now :).

> > When the buddies get listed, they are animated, and while the
> > buddy rows expand, the buddy icon shrinks (you can see on my
> > screenshot that it's no longer a square).
> 
> My best guess for fixing this was to ensure min-height and min-width on the
> userIcon and statusTypeIcon.

This is fixed.


> > The green dot and "Available" are also not correctly aligned vertically.
> 
> This was also not explicitly addressed beyond specifying the margins of the
> text elements. If it isn't fixed, I can't think of a way other than making a
> larger top margin for Mac.

This isn't fixed. See the conv-info-large binding in conversation.xml for ideas. I wonder if this binding could even be reused directly; maybe not.

> > The status line seems missing.
> 
> I took a look at the ChatBuddy interfaces and didn't see any way of getting
> status. For now, I am directly assigning "unknown" status for participants.

The status line is still missing for private conversation tabs with nicks that aren't in the contact list (eg. NickServ).


I think the margins around the buddy icon could be slightly improved. I would like if the margin between the top of the tooltip and the top of the icon's border was the same as the margin between the left of the tooltip and the left of the icon's border, and the same as the margin between the right of the icon's border and the left of the display name, and the same as the margin between the left of the tooltip and the left of the lines of text in bold. http://i5.minus.com/izCiAjI1d4FY6.png is what I currently see.
Attachment #8354396 - Flags: review-
Attachment #8354396 - Flags: feedback?(florian)
Attachment #8354396 - Flags: feedback+
*** Original post on bio 1221 at 2013-08-17 11:32:11 UTC ***

Based on the attached screenshots, I think these tooltips look a bit too cluttered. This is probably just due to insufficient whitespace (e.g. there could be more space between the "conversation-top" part and the "table" part) and asymmetric margins, but it might also be worth trying e.g. a slightly different background colour or shading for the top part, to give it some visual separation.
*** Original post on bio 1221 as attmnt 3008 at 2013-11-02 22:06:00 UTC ***

This tooltip patch shares more in common with conv-info-large. The design still appears the same on Windows as in the previous patch, but should have improvements elsewhere.

The issue I've been trying to fix for quite awhile now is that having multiple lines in the description causes some height calculation issue. This can be seen by viewing an IRC channel tooltip with a long topic. The issue might be related to this BMO bug (https://bugzilla.mozilla.org/show_bug.cgi?id=343964). The
previous version of the patch didn't have this issue, and I haven't figured out what makes the issue appear or not (might be using a stack vs vbox).

With that bug this patch could never land, but I'm hoping it at least fixes some CSS issues from the previous one.

> The status line is still missing for private conversation tabs with
> nicks that aren't in the contact list (eg. NickServ).

This should be fixed now (checking isChat before assigning the message to a topic).

> I think the margins around the buddy icon could be slightly improved.

The only change I've made for this is to make the border of the buddy icon left
align with the rest of the tooltip text. The tooltips seem to have OS-dependent internal padding, and I haven't figured out a way to have the content have equal margins on all sides that would work for all OS styles.
Attachment #8354789 - Flags: feedback?(florian)
Comment on attachment 8354383 [details]
Screenshot with a buddy icon, and 2 buddies

*** Original change on bio 1221 attmnt 2614 at 2013-11-02 22:06:51 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354383 - Attachment is obsolete: true
Comment on attachment 8354384 [details]
Screenshot without buddy icon

*** Original change on bio 1221 attmnt 2615 at 2013-11-02 22:06:51 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354384 - Attachment is obsolete: true
Comment on attachment 8354396 [details] [diff] [review]
Progress on CSS fixes.

*** Original change on bio 1221 attmnt 2627 at 2013-11-02 22:06:51 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354396 - Attachment is obsolete: true
*** Original post on bio 1221 as attmnt 3028 at 2013-11-10 02:51:00 UTC ***

This has the vbox/spacer fixes that correct the description tag wrapping height issue. The status icon for chats is now chat-16.png, and the "left" state of chats is now recognized. Channels with no topic now have the italic "No topic..." message.
Attachment #8354809 - Flags: review?(florian)
Comment on attachment 8354789 [details] [diff] [review]
Design closer to conv-info-large, has multiline issues.

*** Original change on bio 1221 attmnt 3008 at 2013-11-10 02:51:30 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354789 - Attachment is obsolete: true
Attachment #8354789 - Flags: feedback?(florian)
Comment on attachment 8354809 [details] [diff] [review]
Fixes for text wrapping, chat icon display in tooltips.

*** Original change on bio 1221 attmnt 3028 at 2013-11-10 03:01:52 UTC ***

Thanks! My short testing of this version of the patch didn't show any issue. I'm really excited to have this in the next nightlies! :-)
Attachment #8354809 - Flags: review?(florian) → review+
*** Original post on bio 1221 at 2013-11-10 03:12:01 UTC ***

Can't wait to test this out!

http://hg.instantbird.org/instantbird/rev/499b80eaa20f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.5
Depends on: 955692
Depends on: 955693
Depends on: 955694
Depends on: 955695
You need to log in before you can comment on or make changes to this bug.