Closed
Bug 954509
Opened 10 years ago
Closed 10 years ago
Default message styles lack context message support
Categories
(Instantbird Graveyard :: Conversation, defect)
Instantbird Graveyard
Conversation
Tracking
(Not tracked)
RESOLVED
FIXED
1.2
People
(Reporter: aleth, Assigned: aleth)
References
Details
(Whiteboard: [1.2-wanted])
Attachments
(1 file, 10 obsolete files)
11.79 KB,
patch
|
benediktp
:
review+
|
Details | Diff | Splinter Review |
*** 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
Assignee | ||
Comment 1•10 years ago
|
||
*** 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?
Assignee | ||
Comment 2•10 years ago
|
||
*** 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?
Assignee | ||
Updated•10 years ago
|
Summary: Default message styles lack context message support → Default message styles lack context message support and override font choice
Assignee | ||
Comment 3•10 years ago
|
||
*** 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 | ||
Updated•10 years ago
|
Assignee: nobody → aleth
Assignee | ||
Comment 4•10 years ago
|
||
*** 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?
Assignee | ||
Comment 5•10 years ago
|
||
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?
Assignee | ||
Comment 6•10 years ago
|
||
*** 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?
Comment 7•10 years ago
|
||
*** 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! :-)
Assignee | ||
Comment 8•10 years ago
|
||
*** 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?
Assignee | ||
Comment 9•10 years ago
|
||
*** 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?
Assignee | ||
Comment 10•10 years ago
|
||
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?
Assignee | ||
Comment 11•10 years ago
|
||
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?
Assignee | ||
Comment 12•10 years ago
|
||
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?
Assignee | ||
Comment 13•10 years ago
|
||
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?
Assignee | ||
Comment 14•10 years ago
|
||
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?
Assignee | ||
Comment 15•10 years ago
|
||
*** 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?
Comment 16•10 years ago
|
||
*** 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
Comment 17•10 years ago
|
||
*** 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.
Updated•10 years ago
|
Whiteboard: [1.2-wanted]
Assignee | ||
Comment 18•10 years ago
|
||
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)
Assignee | ||
Comment 19•10 years ago
|
||
*** 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)
Assignee | ||
Comment 20•10 years ago
|
||
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)
Assignee | ||
Comment 21•10 years ago
|
||
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?
Assignee | ||
Updated•10 years ago
|
Summary: Default message styles lack context message support and override font choice → Default message styles lack context message support
Comment 22•10 years ago
|
||
*** 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?
Assignee | ||
Comment 23•10 years ago
|
||
*** 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)
Assignee | ||
Comment 24•10 years ago
|
||
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)
Comment 25•10 years ago
|
||
*** 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 26•10 years ago
|
||
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)
Assignee | ||
Comment 27•10 years ago
|
||
*** 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.
Comment 28•10 years ago
|
||
*** 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 29•10 years ago
|
||
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
Updated•10 years ago
|
Whiteboard: [1.2-wanted] → [1.2-wanted][checkin-needed]
Comment 30•10 years ago
|
||
*** 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]
Updated•10 years ago
|
Target Milestone: --- → 1.2
You need to log in
before you can comment on or make changes to this bug.
Description
•