Closed Bug 954509 Opened 10 years ago Closed 10 years ago

Default message styles lack context message support

Categories

(Instantbird Graveyard :: Conversation, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

References

Details

(Whiteboard: [1.2-wanted])

Attachments

(1 file, 10 obsolete files)

*** Original post on bio 1074 at 2011-10-14 09:57:00 UTC ***

Now required for conversations on hold.

Default styles atm:
- Dark
- Simple
- Paper sheets
Attached patch Patch for Simple message style (obsolete) — Splinter Review
*** Original post on bio 1074 as attmnt 900 at 2011-10-15 16:00:00 UTC ***

Adds context message support to Simple message style
Attachment #8352642 - Flags: review?
Attached patch Patch for Papersheets style (obsolete) — Splinter Review
*** Original post on bio 1074 as attmnt 902 at 2011-10-15 18:41:00 UTC ***

- Adds context message support to Papersheets style. (Since the style is quite pastel-coloured, more desaturation than in Bubbles is required.)

- Removes explicit fonts from font-family tag from css (so that font selection via IB Preferences works).

- Bugfix: White variant now works not just in the Preferences preview pane ;)
Attachment #8352644 - Flags: review?
Summary: Default message styles lack context message support → Default message styles lack context message support and override font choice
Attached patch Patch for Bubbles style (obsolete) — Splinter Review
*** Original post on bio 1074 as attmnt 903 at 2011-10-15 18:44:00 UTC ***

Bubbles style: Removes explicit fonts from font-family tag from css (so that font selection via IB Preferences works).
Attachment #8352645 - Flags: review?
Assignee: nobody → aleth
Attached patch Patch for Papersheets style (v2) (obsolete) — Splinter Review
*** Original post on bio 1074 as attmnt 904 at 2011-10-15 20:03:00 UTC ***

Improved version: I think one can get away with just a little bit less desaturation on background and text and still have reasonably distinct context bubbles.
Attachment #8352646 - Flags: review?
Comment on attachment 8352644 [details] [diff] [review]
Patch for Papersheets style

*** Original change on bio 1074 attmnt 902 at 2011-10-15 20:03:39 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352644 - Attachment is obsolete: true
Attachment #8352644 - Flags: review?
Attached patch Patch for Dark style (obsolete) — Splinter Review
*** Original post on bio 1074 as attmnt 905 at 2011-10-15 20:06:00 UTC ***

- Adds context message support to Dark style. This was a tricky one as the colours are already so muted in this style. I decided to make the context "bubbles" all black; on hover they are highlighted by the selected variant colour.

- Removes explicit fonts from font-family tag from css (so that font selection
via IB Preferences works).
Attachment #8352647 - Flags: review?
*** Original post on bio 1074 at 2011-10-15 21:39:31 UTC ***

(In reply to comment #2)

> - Bugfix: White variant now works not just in the Preferences preview pane ;)

I can't reproduce. If you can find and understand this, maybe handle it in a separate bug?
[you just informed me on IRC that it's only in MUCs. I still think it would be better in another bug though.]


* attachment 8352642 [details] [diff] [review] (bio-attmnt 900)
- simple/Variants/Dark.css: while I usually prefer the rgb(r, g, b) format which I find more readable, when everything else is in #hex format in the file, it's better to keep the file consistent.

* attachment 8352646 [details] [diff] [review] (bio-attmnt 904)
- papersheets/Incoming/NextContext.html doesn't seem useful.
- please revert the changes in papersheets/Variants/White.css

* attachment 8352647 [details] [diff] [review] (bio-attmnt 905)
- dark/Incoming/NextContext.html doesn't seem useful. If it is, we need to understand why.


* New files need to be listed in http://lxr.instantbird.org/instantbird/source/instantbird/themes/jar.mn, otherwise they won't be packaged.

* I would like to understand why "Sans" was listed in the font-family rules that are being removed before r+ing this.

Thanks for taking care of this bug! :-)
Attached patch Unified patch for message styles (obsolete) — Splinter Review
*** Original post on bio 1074 as attmnt 906 at 2011-10-15 22:04:00 UTC ***

Comprises the patches above, plus:

- remove NextContext.html from Papersheets as unnecessary
- change rgb to hex in simple/Variants/Dark.css for consistency
Attachment #8352648 - Flags: review?
*** Original post on bio 1074 as attmnt 907 at 2011-10-16 01:01:00 UTC ***

Comprises the patches above, plus:

- Reverted changes to White.css as requested (will submit as separate bug)

- Revised Dark style: Better context message implementation more consistent with the coding of the rest of the style. NextContext.html now obviously required as desaturation latches onto the p tag. Added slightly dimmed nicks for context messages as desaturation hardly acts on white.
Attachment #8352649 - Flags: review?
Comment on attachment 8352642 [details] [diff] [review]
Patch for Simple message style

*** Original change on bio 1074 attmnt 900 at 2011-10-16 01:01:55 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352642 - Attachment is obsolete: true
Attachment #8352642 - Flags: review?
Comment on attachment 8352645 [details] [diff] [review]
Patch for Bubbles style

*** Original change on bio 1074 attmnt 903 at 2011-10-16 01:01:55 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352645 - Attachment is obsolete: true
Attachment #8352645 - Flags: review?
Comment on attachment 8352646 [details] [diff] [review]
Patch for Papersheets style (v2)

*** Original change on bio 1074 attmnt 904 at 2011-10-16 01:01:55 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352646 - Attachment is obsolete: true
Attachment #8352646 - Flags: review?
Comment on attachment 8352647 [details] [diff] [review]
Patch for Dark style

*** Original change on bio 1074 attmnt 905 at 2011-10-16 01:01:55 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352647 - Attachment is obsolete: true
Attachment #8352647 - Flags: review?
Comment on attachment 8352648 [details] [diff] [review]
Unified patch for message styles

*** Original change on bio 1074 attmnt 906 at 2011-10-16 01:01:55 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352648 - Attachment is obsolete: true
Attachment #8352648 - Flags: review?
Attached patch Patch for jar.mn (obsolete) — Splinter Review
*** Original post on bio 1074 as attmnt 909 at 2011-10-16 01:23:00 UTC ***

>* New files need to be listed in
>http://lxr.instantbird.org/instantbird/source/instantbird/themes/jar.mn,
>otherwise they won't be packaged.

Not tested due to lack of dev environment.
Attachment #8352651 - Flags: review?
*** Original post on bio 1074 at 2011-10-17 12:41:30 UTC ***

First I see this bug as doing two totally separate things (the context messages + the font issue), we should try to separate these in the future.  I don't think it's said anywhere in the bug, but the changes to simple will cause context messages to be given a blue color.  I have some concern about this as blue+bold is occasionally used to me "new" on Windows (at least Thunderbird does it and I think I've seen it in actual Windows programs as well, could someone verify this?). I'll try this patch later.
Status: NEW → ASSIGNED
Hardware: x86 → All
*** Original post on bio 1074 at 2011-10-18 19:24:29 UTC ***

(In reply to comment #6)

> * I would like to understand why "Sans" was listed in the font-family rules
> that are being removed before r+ing this.

I discussed this with idechix, and it seems using the OS default sans-serif font is a good way forward for Linux.
Whiteboard: [1.2-wanted]
Comment on attachment 8352649 [details] [diff] [review]
Unified patch for message styles (v2)

*** Original change on bio 1074 attmnt 907 at 2011-12-15 21:40:51 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352649 - Flags: review? → review?(florian)
Depends on: 954731
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1074 as attmnt 1208 at 2012-02-28 21:30:00 UTC ***

(In reply to comment #10)

Font issue separated into another bug. Regarding the "blue" in simple, if I remember correctly you won't actually notice it as blue unless you look very closely. But it gives some extra separation between the system message and the standard message colours.
Attachment #8352959 - Flags: review?(clokep)
Comment on attachment 8352649 [details] [diff] [review]
Unified patch for message styles (v2)

*** Original change on bio 1074 attmnt 907 at 2012-02-28 21:30:52 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352649 - Attachment is obsolete: true
Attachment #8352649 - Flags: review?(florian)
Comment on attachment 8352651 [details] [diff] [review]
Patch for jar.mn

*** Original change on bio 1074 attmnt 909 at 2012-02-28 21:30:52 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352651 - Attachment is obsolete: true
Attachment #8352651 - Flags: review?
Summary: Default message styles lack context message support and override font choice → Default message styles lack context message support
*** Original post on bio 1074 at 2012-04-20 15:08:49 UTC ***

I applied the patch and looked at the different message styles. Here's a few comments.

In general I found it to hard to distinguish context and content messages.


-- Dark --
Making the border and background darker helps to distinguish the bubbles better:

p.context:not(:hover){
  border-top: 1px solid rgba(255, 255, 255, 0.15);
  background: -moz-linear-gradient(top, rgba(255, 255, 255, 0.1), rgba(255, 255, 255, 0.03) 30px);
}

I'm a bit concerned that we seem to lose the color of the sender here though.

-- Paper Sheets --
Paper Sheets: the gradient at the bottom of context messages flickers when I move the mouse of them. Making the text of context messages grey instead of black would also increase the difference.


-- Simple --
Simple - Normal: I can't really distinguish context and content messages.
Simple - Dark: A bit better but still to hard to distinguish. Might depend on display settings, of course.

I know that Simple should be as simple as possible but dimming the sender's name might help here. I tried by setting the opacity on the sender of context messages to 0.7 and it looked much better imo. Making the text color lighter for Normal also seems to help.
Alternatively: what about 'greying out' the background of these messages?
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1074 as attmnt 1377 at 2012-04-20 20:05:00 UTC ***

(In reply to comment #13)
Thanks for testing this!

> In general I found it to hard to distinguish context and content messages.

I've increased the contrast between context/noncontext messages in all of these.

> -- Dark --
> Making the border and background darker helps to distinguish the bubbles
> better:
> 
> p.context:not(:hover){
>   border-top: 1px solid rgba(255, 255, 255, 0.15);
>   background: -moz-linear-gradient(top, rgba(255, 255, 255, 0.1), rgba(255,
> 255, 255, 0.03) 30px);
> }
> 
> I'm a bit concerned that we seem to lose the color of the sender here though.

While I liked the look of your suggestion, I could see no way to retain the colour of the sender at the same time. So instead I went for a different approach altogether (changing the opacity).

> -- Paper Sheets --
> Paper Sheets: the gradient at the bottom of context messages flickers when I
> move the mouse of them. Making the text of context messages grey instead of
> black would also increase the difference.

The SVG filter was incompatible with the JS background setter. Instead this now sets the desaturated background from the JS too, and includes your suggestion of grey text.

> -- Simple --
> I know that Simple should be as simple as possible but dimming the sender's
> name might help here. I tried by setting the opacity on the sender of context
> messages to 0.7 and it looked much better imo. Making the text color lighter
> for Normal also seems to help.

Followed all of these suggestions.
Attachment #8353130 - Flags: review?(benediktp)
Comment on attachment 8352959 [details] [diff] [review]
Patch

*** Original change on bio 1074 attmnt 1208 at 2012-04-20 20:05:35 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352959 - Attachment is obsolete: true
Attachment #8352959 - Flags: review?(clokep)
*** Original post on bio 1074 at 2012-06-03 16:18:47 UTC ***

Style comments:
-- Dark --
Looks good :)

-- Papersheets --
The gradient at the bottom of context messages (or is it at the top of the message below?) has the same grey as content messages. I think it might be better if it was slightly lighter too.

-- Simple --
Yes, seems OK for me (others might think differently about the contrast?).


I'll review the code later today but one thing I spotted is in Papersheet's Footer.html: className.indexOf("context") > -1 can be replaced with classList.contains() here.


Thanks for uploading the zipped folders, it made checking the changes so much easier.
Comment on attachment 8353130 [details] [diff] [review]
Patch

*** Original change on bio 1074 attmnt 1377 at 2012-06-04 13:31:35 UTC ***

>diff --git a/chrome/instantbird/skin/classic/instantbird/messages/papersheets/Footer.html b/chrome/instantbird/skin/classic/instantbird/messages/papersheets/Footer.html
> const bg_gradient = "background: -moz-linear-gradient(top, rgba(0, 0, 0, 0), rgba(0, 0, 0, 0.1) 15px, hsla(#, 100%, 98%, 1) 15px, hsla(#, 100%, 98%, 1));";
>+const bg_context_gradient = "background: -moz-linear-gradient(top, rgba(0, 0, 0, 0), rgba(0, 0, 0, 0.1) 15px, hsla(#, 20%, 98%, 1) 15px, hsla(#, 20%, 98%, 1));";

I wondered if it might make sense to use two replacement variables and only one template string here but I don't think it would make things much nicer either.

>       var senderHue = parsed[1];
>-      target.setAttribute("style", bg_gradient.replace("#", senderHue, "g"));
>+      if (target.className.indexOf("context") > -1)
>+        target.setAttribute("style", bg_context_gradient.replace("#", senderHue, "g"));
>+      else
>+        target.setAttribute("style", bg_gradient.replace("#", senderHue, "g"));

classList.contains() would be better (that's what it's made for) and we should fix the other places where className.indexOf() is used one day.

>diff --git a/chrome/instantbird/skin/classic/instantbird/messages/papersheets/main.css b/chrome/instantbird/skin/classic/instantbird/messages/papersheets/main.css
>-div.outgoing {
>+div.outgoing {
>   background: -moz-linear-gradient(top, rgba(0, 0, 0, 0), rgba(0, 0, 0, 0.1) 15px, rgba(245, 245, 255, 1) 15px, rgba(245, 245, 255, 1));
> }
>
>-div.incoming {
>+div.incoming {
>   background: -moz-linear-gradient(top, rgba(0, 0, 0, 0), rgba(0, 0, 0, 0.1) 15px, rgba(255, 245, 245, 1) 15px, rgba(255, 245, 245, 1));
> }
>
>-div.event {
>+div.event {
>   background: -moz-linear-gradient(top, rgba(0, 0, 0, 0), rgba(0, 0, 0, 0.1) 15px, rgba(255, 255, 240, 1) 15px, rgba(255, 255, 240, 1));
> }

What's changing here? Were there wrong linebreaks on these lines maybe?

>-p.outgoing>span.pseudo {
>+p.outgoing>span.pseudo {
>   color: rgb(80,80,200);
> }
>
>-p.incoming>span.pseudo {
>+p.incoming>span.pseudo {
>   color: rgb(200,80,80);
> }

Same here.

>diff --git a/chrome/instantbird/skin/classic/instantbird/messages/simple/main.css b/chrome/instantbird/skin/classic/instantbird/messages/simple/main.css
> .event {
>-  color: gray;
>+  color: rgb(170,170,170);
> }

What's the reason for this change? It seems unrelated (at first glance) and actually changes the color since gray is normally rgb(128, 128, 128) (see https://developer.mozilla.org/en/CSS/color_value)

jar.mn looks fine.
Attachment #8353130 - Flags: review?(benediktp)
*** Original post on bio 1074 at 2012-06-04 19:28:43 UTC ***

(In reply to comment #16)
> The gradient at the bottom of context messages (or is it at the top of the
> message below?) has the same grey as content messages. I think it might be
> better if it was slightly lighter too.

Good idea.

> classList.contains() would be better (that's what it's made for) and we should
> fix the other places where className.indexOf() is used one day.

Yes, it's better, I just followed the style of the existing code. Now I have replaced all of them instead.

> >diff --git a/chrome/instantbird/skin/classic/instantbird/messages/simple/main.css b/chrome/instantbird/skin/classic/instantbird/messages/simple/main.css
> > .event {
> >-  color: gray;
> >+  color: rgb(170,170,170);
> > }
> 
> What's the reason for this change? It seems unrelated (at first glance) and
> actually changes the color since gray is normally rgb(128, 128, 128) (see
> https://developer.mozilla.org/en/CSS/color_value)

Yes, it's lighter. That's intentional, as otherwise it is hard to distinguish context message gray from system message gray.
*** Original post on bio 1074 as attmnt 1561 at 2012-06-05 13:06:00 UTC ***

The patch and the look of the applied patch seems fine.

I hesitated on the additional classList.contains() changes since the code around looks like it could need some attention too. Eventually I kept them so that we consistently use classList.contains() throughout Paper Sheet's Footer.html now.

Thank you for bringing context messages to all of our themes now :)
Attachment #8353316 - Flags: review+
Comment on attachment 8353130 [details] [diff] [review]
Patch

*** Original change on bio 1074 attmnt 1377 at 2012-06-05 13:06:52 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353130 - Attachment is obsolete: true
Whiteboard: [1.2-wanted] → [1.2-wanted][checkin-needed]
*** Original post on bio 1074 at 2012-06-05 23:08:48 UTC ***

Committed as http://hg.instantbird.org/instantbird/rev/a26ec4a13add

Thanks for working on this! Sorry it took so long for us to get to. :(
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [1.2-wanted][checkin-needed] → [1.2-wanted]
Target Milestone: --- → 1.2
You need to log in before you can comment on or make changes to this bug.