Closed Bug 1420619 Opened 2 years ago Closed 2 years ago

Make the conversation listitems look like vertical tabs

Categories

(Thunderbird :: Instant Messaging, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 60.0

People

(Reporter: Paenglab, Assigned: Paenglab)

Details

Attachments

(3 files, 9 obsolete files)

With making the contact- and conversationitem looking like vertical tabs the chat content looks more like Photon. It also connects the items more to the conversation content because no separating border exists.
Attached patch conv-tabs.patch (obsolete) — Splinter Review
Florian, what do think about this proposal?

This patch needs to be applied on top of bug 1417693.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8931877 - Flags: review?(florian)
Could you please attach a screenshot?
Attached image conversation.png
This is how it looks on Win10.
The groups, in the screenshot the "Conversations" with the twisty, don't look like a tab when selected because they don't have content, except the connections status.
Attached patch conv-tabs.patch (obsolete) — Splinter Review
Patch updated to tip.
Attachment #8931877 - Attachment is obsolete: true
Attachment #8931877 - Flags: review?(florian)
Attachment #8938335 - Flags: review?(florian)
Comment on attachment 8938335 [details] [diff] [review]
conv-tabs.patch

Nihanth, it seems Florian has no time for a review. Please, could you look at it?
Attachment #8938335 - Flags: review?(nhnt11)
I'd be able to do a code review on this, but I don't think that would be productive until the visuals have been vetted (by Florian?) and we are sure that we want to land this. I'm not aware about how decisions on visual changes are made in the Thunderbird community. Personally, I dislike how it looks in the screenshot - but that opinion is based on very little information and I'll try your patch later to determine my final opinion. :)

In any case, my current priority when I do have time to work on Tb is to move bug 1412710 forward. Realistically, I'll be able to pay attention to this bug once that lands. Hope that makes sense!

Florian, needinfo'ing you to chime in and let us know what you think of this visual change - specifically, are there particular things you like, or any particular things you'd change? Thanks!
Flags: needinfo?(florian)
Attached patch conv-tabs.patch (rebased) (obsolete) — Splinter Review
The best feedback is unsolicited feedback, so here goes ;-)

I like this idea. With this small UI change it's much clearer which conversation is currently displayed in the conversation pane (or how is this called?) in the middle.

Personally I think the little [x] to close the conversation could still appear on the right side of the "conversation tab" like it does now.

Maybe this could be reviewed before it rots again :-(
Attachment #8938335 - Attachment is obsolete: true
Attachment #8938335 - Flags: review?(nhnt11)
Attachment #8938335 - Flags: review?(florian)
Attachment #8942876 - Flags: review?(nhnt11)
Attachment #8942876 - Flags: review?(florian)
Attachment #8942876 - Flags: feedback+
Comment on attachment 8942876 [details] [diff] [review]
conv-tabs.patch (rebased)

M-C is pushing hard to remove the -moz-border-*-colors and this patch removes all remaining occurrences of them in this files.

Magnus, as a UI peer could look at this soon? This is more a UI change than a Chat change.
Attachment #8942876 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8942876 [details] [diff] [review]
conv-tabs.patch (rebased)

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

::: mail/components/im/content/imconv.xml
@@ +17,5 @@
>    <binding id="conv" extends="chrome://global/content/bindings/richlistbox.xml#richlistitem">
>      <content>
> +      <xul:vbox class="box-line" xbl:inherits="selected"/>
> +        <xul:button anonid="closeConversationButton" class="closeConversationButton close-icon"
> +                    tooltiptext="&closeConversationButton.tooltip;"/>

The markup is strange here. Either the indent is wrong, or this button was meant to be inside the vbox.
Attached patch conv-tabs.patch (obsolete) — Splinter Review
(In reply to Florian Quèze [:florian] from comment #10)
> Comment on attachment 8942876 [details] [diff] [review]
> conv-tabs.patch (rebased)
> 
> Review of attachment 8942876 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mail/components/im/content/imconv.xml
> @@ +17,5 @@
> >    <binding id="conv" extends="chrome://global/content/bindings/richlistbox.xml#richlistitem">
> >      <content>
> > +      <xul:vbox class="box-line" xbl:inherits="selected"/>
> > +        <xul:button anonid="closeConversationButton" class="closeConversationButton close-icon"
> > +                    tooltiptext="&closeConversationButton.tooltip;"/>
> 
> The markup is strange here. Either the indent is wrong, or this button was
> meant to be inside the vbox.

The indentation was wrong.

(In reply to Jorg K (GMT+1) from comment #8)
> Created attachment 8942876 [details] [diff] [review]
> conv-tabs.patch (rebased)
> Personally I think the little [x] to close the conversation could still
> appear on the right side of the "conversation tab" like it does now.

Like this the close button aligns with the twisties of the groups. And the conversation icons align with the groups text and make the conversation text indented.
Attachment #8942876 - Attachment is obsolete: true
Attachment #8942876 - Flags: review?(nhnt11)
Attachment #8942876 - Flags: review?(mkmelin+mozilla)
Attachment #8942876 - Flags: review?(florian)
Attachment #8943206 - Flags: review?(nhnt11)
Attachment #8943206 - Flags: review?(mkmelin+mozilla)
Attachment #8943206 - Flags: review?(florian)
Attached image Screenshot on Mac
Flags: needinfo?(florian)
Attached patch conv-tabs.patch (obsolete) — Splinter Review
This patch should fix all issues. Maybe except the scrollbar issue which i'm not sure what's the problem.
Attachment #8943206 - Attachment is obsolete: true
Attachment #8943206 - Flags: review?(nhnt11)
Attachment #8943206 - Flags: review?(mkmelin+mozilla)
Attachment #8943206 - Flags: review?(florian)
Attachment #8943668 - Flags: review?(florian)
Attached patch conv-tabs.patch (obsolete) — Splinter Review
I found on Linux that the text on imcontact war too high. Fixed this.
Attachment #8943668 - Attachment is obsolete: true
Attachment #8943668 - Flags: review?(florian)
Attachment #8944201 - Flags: review?(florian)
-moz-border-*-colors support is removed now. Florian, please can you look at this bug to not break the display too long on Windows?
Comment on attachment 8944201 [details] [diff] [review]
conv-tabs.patch

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

Looks much better, but I've still found a few issues.

https://i.imgur.com/hA1Nlj6.png shows the margin issues with the unread count and the status message, and shows my suggestion of adding a -1px margin end border on the scrollbox.

::: mail/components/im/content/imcontact.xml
@@ +15,5 @@
>            xmlns:html="http://www.w3.org/1999/xhtml">
>  
>    <binding id="contact" extends="chrome://global/content/bindings/richlistbox.xml#richlistitem">
>      <content>
> +      <xul:vbox class="box-line" xbl:inherits="selected"/>

Would be nicer to do this with a border or background. Is there a reason why it's not possible?

::: mail/components/im/themes/chat.css
@@ +145,5 @@
> +  padding-inline-start: 0;
> +}
> +
> +imconv:not(:hover) > .closeConversationButton {
> +  fill: transparent;

I would prefer visibility:hidden, I'm afraid this would paint the svg image for every item, and so perform poorly.

@@ +159,5 @@
>  .contactDisplayName,
>  .convDisplayName {
>    margin-inline-end: 0;
> +%ifdef XP_MACOSX
> +  margin-top: 3px;

- You need this margin in the .convStatusText and .convStatusText rule too.
- There's a problem with the unread message count, but the devtools Inspector hasn't been helpful enough to let me confirm the fix. My guess is that you need the margin-top rule to apply to .convUnreadTargetedCount too.
- It seems this would be better in the platform specific files.

::: mail/themes/osx/mail/chat.css
@@ +23,5 @@
> +  border-inline-start: 1px solid var(--imbox-selected-border-color);
> +}
> +
> +#contactlistbox > scrollbox {
> +  margin-inline-start: -1px;

During my testing on Mac, adding:
margin-inline-end: -1px;
improved the appearance. It removes the 1 pixel of whitespace between the window border and the blue vertical bar of the selected item (matching the appearance for selected tabs that have their blue area touching the window border).
Attachment #8944201 - Flags: review?(florian) → review-
Attached patch conv-tabs.patch (obsolete) — Splinter Review
(In reply to Florian Quèze [:florian] from comment #16)
> Comment on attachment 8944201 [details] [diff] [review]
> conv-tabs.patch
> 
> Review of attachment 8944201 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks much better, but I've still found a few issues.
> 
> https://i.imgur.com/hA1Nlj6.png shows the margin issues with the unread
> count and the status message, and shows my suggestion of adding a -1px
> margin end border on the scrollbox.
> 
> ::: mail/components/im/content/imcontact.xml
> @@ +15,5 @@
> >            xmlns:html="http://www.w3.org/1999/xhtml">
> >  
> >    <binding id="contact" extends="chrome://global/content/bindings/richlistbox.xml#richlistitem">
> >      <content>
> > +      <xul:vbox class="box-line" xbl:inherits="selected"/>
> 
> Would be nicer to do this with a border or background. Is there a reason why
> it's not possible?

I implemented how tabbrowser have done it. Now implemented with border.

> ::: mail/components/im/themes/chat.css
> @@ +145,5 @@
> > +  padding-inline-start: 0;
> > +}
> > +
> > +imconv:not(:hover) > .closeConversationButton {
> > +  fill: transparent;
> 
> I would prefer visibility:hidden, I'm afraid this would paint the svg image
> for every item, and so perform poorly.

Done.

> @@ +159,5 @@
> >  .contactDisplayName,
> >  .convDisplayName {
> >    margin-inline-end: 0;
> > +%ifdef XP_MACOSX
> > +  margin-top: 3px;
> 
> - You need this margin in the .convStatusText and .convStatusText rule too.
> - There's a problem with the unread message count, but the devtools
> Inspector hasn't been helpful enough to let me confirm the fix. My guess is
> that you need the margin-top rule to apply to .convUnreadTargetedCount too.
> - It seems this would be better in the platform specific files.

Moved and added .convStatusText and .convStatusText.

> ::: mail/themes/osx/mail/chat.css
> @@ +23,5 @@
> > +  border-inline-start: 1px solid var(--imbox-selected-border-color);
> > +}
> > +
> > +#contactlistbox > scrollbox {
> > +  margin-inline-start: -1px;
> 
> During my testing on Mac, adding:
> margin-inline-end: -1px;
> improved the appearance. It removes the 1 pixel of whitespace between the
> window border and the blue vertical bar of the selected item (matching the
> appearance for selected tabs that have their blue area touching the window
> border).

I added a especially a margin. Removed.
Attachment #8944201 - Attachment is obsolete: true
Attachment #8944880 - Flags: review?(florian)
Comment on attachment 8944880 [details] [diff] [review]
conv-tabs.patch

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

- When an imconv item becomes selected, its content shifts 1px towards the left (this issue doesn't exist for imcontact items).
- The blue area of selected items doesn't have square corners.
Attachment #8944880 - Flags: review?(florian) → review-
Attached patch conv-tabs.patch (obsolete) — Splinter Review
(In reply to Florian Quèze [:florian] from comment #18)
> Comment on attachment 8944880 [details] [diff] [review]
> conv-tabs.patch
> 
> Review of attachment 8944880 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> - When an imconv item becomes selected, its content shifts 1px towards the
> left (this issue doesn't exist for imcontact items).

Fixed.

> - The blue area of selected items doesn't have square corners.

You asked for a border. Now using background-image.
Attachment #8944880 - Attachment is obsolete: true
Attachment #8945020 - Flags: review?(florian)
Attached patch conv-tabs.patch (obsolete) — Splinter Review
Sorry, patch wasn't updated.
Attachment #8945020 - Attachment is obsolete: true
Attachment #8945020 - Flags: review?(florian)
Attachment #8945021 - Flags: review?(florian)
Comment on attachment 8945021 [details] [diff] [review]
conv-tabs.patch

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

::: mail/components/im/themes/chat.css
@@ +132,5 @@
> +imconv[selected=true],
> +imcontact[selected=true] {
> +  background-color: var(--imbox-selected-background-color);
> +  border-color: var(--imbox-selected-border-color);
> +  background-image: linear-gradient(to right, var(--tab-line-color) 2px, transparent 2px);

This results in a 1px line on lowdpi, and 1.5px line on hidpi. The blue line on tabs is 2px.
I haven't been able to figure out a fix, changing the '2px' values to '3px' makes the line 3px, and changing to 2.5px doesn't seem to improve anything.
(In reply to Florian Quèze [:florian] from comment #21)
> Comment on attachment 8945021 [details] [diff] [review]
> conv-tabs.patch
> 
> Review of attachment 8945021 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mail/components/im/themes/chat.css
> @@ +132,5 @@
> > +imconv[selected=true],
> > +imcontact[selected=true] {
> > +  background-color: var(--imbox-selected-background-color);
> > +  border-color: var(--imbox-selected-border-color);
> > +  background-image: linear-gradient(to right, var(--tab-line-color) 2px, transparent 2px);
> 
> This results in a 1px line on lowdpi, and 1.5px line on hidpi. The blue line
> on tabs is 2px.
> I haven't been able to figure out a fix, changing the '2px' values to '3px'
> makes the line 3px, and changing to 2.5px doesn't seem to improve anything.

Maybe then I use the vbox again:
(In reply to Florian Quèze [:florian] from comment #18)
> Would be nicer to do this with a border or background. Is there a reason why
> it's not possible?
(In reply to Richard Marti (:Paenglab) from comment #22)

> Maybe then I use the vbox again:

It's a disappointing reason for adding more markup, but I don't want to block this forever... so if you want to go this route, I'll r+.
Attached patch conv-tabs.patchSplinter Review
Implementation with vbox.
When you know why this happens with the background-image, I can switch to it later.
Attachment #8945021 - Attachment is obsolete: true
Attachment #8945021 - Flags: review?(florian)
Attachment #8945053 - Flags: review?(florian)
Comment on attachment 8945053 [details] [diff] [review]
conv-tabs.patch

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

Looks good to me on Mac now, and I found no obvious issue by code inspection, so r=me. I haven't tested on Windows/Linux. I'm hoping it looks as good there. If it doesn't I guess nightly testers will file bugs with screenshots.
Attachment #8945053 - Flags: review?(florian) → review+
Thanks!

It looks the same on Linux and Windows. Only the vibrancy is missing.
What would be great would be a active tab color/background depending of the selected chat theme to integrate seamless.
Keywords: checkin-needed
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/af865ebcd267
Make the conversation listitems look like vertical tabs. r=florian
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 60.0
You need to log in before you can comment on or make changes to this bug.