Last Comment Bug 756136 - not possible to see if the left pane of the Chat tab has focus if "Conversations" is selected on Windows/Linux
: not possible to see if the left pane of the Chat tab has focus if "Conversati...
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Theme (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: Thunderbird 15.0
Assigned To: Andreas Nilsson (:andreasn)
:
Mentors:
Depends on:
Blocks: 740765
  Show dependency treegraph
 
Reported: 2012-05-17 09:54 PDT by Florian Quèze [:florian] [:flo]
Modified: 2012-06-04 02:04 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch (1.87 KB, patch)
2012-05-17 09:54 PDT, Florian Quèze [:florian] [:flo]
bugs: review-
Details | Diff | Review
fixes win7 aero appearance (2.59 KB, patch)
2012-05-30 06:01 PDT, Andreas Nilsson (:andreasn)
no flags Details | Diff | Review
and fixes for gnomestripe too (2.79 KB, patch)
2012-05-30 07:55 PDT, Andreas Nilsson (:andreasn)
mconley: review+
Details | Diff | Review

Description Florian Quèze [:florian] [:flo] 2012-05-17 09:54:59 PDT
Created attachment 624788 [details] [diff] [review]
Patch

Jb noticed that on Linux it's impossible to see if the left pane (the listbox) of the Chat tab has focus when the "Conversations" item is selected.
I looked at the code and it seems that's broken on Windows too.

The attached patch should make that list look the list of folders of the Mail 3pane view for Windows and Linux.

This bug makes the changes I did in bug 740765 difficult to understand/discover.
Comment 1 Andreas Nilsson (:andreasn) 2012-05-23 03:09:41 PDT
Comment on attachment 624788 [details] [diff] [review]
Patch

diff -r 499441818d84 mail/themes/gnomestripe/mail/chat.css
+:-moz-any(imconv, imcontact, imgroup)[selected] {
+  background-color: -moz-cellhighlight;
+}

This also wants a:
color: -moz-cellhighlighttext;

+  border: 1px dotted #F3D982;
How come this color value?
(same goes for qute)

diff -r 499441818d84 mail/themes/qute/mail/chat.css
+:-moz-any(imconv, imcontact, imgroup)[selected] {
+  background-color: -moz-cellhighlight;
+}

This seems to break the style under Aero, so the corresponding selectors needs to be added to chat-aero.css
Comment 2 Florian Quèze [:florian] [:flo] 2012-05-23 03:30:26 PDT
(In reply to Andreas Nilsson (:andreasn) from comment #1)

> +  border: 1px dotted #F3D982;
> How come this color value?
> (same goes for qute)

http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/themes/winstripe/global/tree.css#56
http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/themes/gnomestripe/global/tree.css#60

> 
> diff -r 499441818d84 mail/themes/qute/mail/chat.css
> +:-moz-any(imconv, imcontact, imgroup)[selected] {
> +  background-color: -moz-cellhighlight;
> +}
> 
> This seems to break the style under Aero, so the corresponding selectors
> needs to be added to chat-aero.css

I'm not sure of how the rules at http://mxr.mozilla.org/comm-central/source/mail/themes/qute/mail/chat-aero.css#92 and the next 20 or so lines were built, they don't seem to copy the toolkit CSS for trees.
Comment 3 Andreas Nilsson (:andreasn) 2012-05-23 09:14:14 PDT
(In reply to Florian Quèze from comment #2)
> (In reply to Andreas Nilsson (:andreasn) from comment #1)
> 
> > +  border: 1px dotted #F3D982;
> > How come this color value?
> > (same goes for qute)
> 
> http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/themes/winstripe/
> global/tree.css#56
> http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/themes/
> gnomestripe/global/tree.css#60

Ah, thanks!


> > diff -r 499441818d84 mail/themes/qute/mail/chat.css
> > +:-moz-any(imconv, imcontact, imgroup)[selected] {
> > +  background-color: -moz-cellhighlight;
> > +}
> > 
> > This seems to break the style under Aero, so the corresponding selectors
> > needs to be added to chat-aero.css
> 
> I'm not sure of how the rules at
> http://mxr.mozilla.org/comm-central/source/mail/themes/qute/mail/chat-aero.
> css#92 and the next 20 or so lines were built, they don't seem to copy the
> toolkit CSS for trees.

We use the same styling in messenger-aero for the folderpane and messagepane http://mxr.mozilla.org/comm-central/source/mail/themes/qute/mail/messenger-aero.css#74
Comment 4 Florian Quèze [:florian] [:flo] 2012-05-23 09:24:13 PDT
(In reply to Andreas Nilsson (:andreasn) from comment #3)

> > > diff -r 499441818d84 mail/themes/qute/mail/chat.css
> > > +:-moz-any(imconv, imcontact, imgroup)[selected] {
> > > +  background-color: -moz-cellhighlight;
> > > +}
> > > 
> > > This seems to break the style under Aero, so the corresponding selectors
> > > needs to be added to chat-aero.css
> > 
> > I'm not sure of how the rules at
> > http://mxr.mozilla.org/comm-central/source/mail/themes/qute/mail/chat-aero.
> > css#92 and the next 20 or so lines were built, they don't seem to copy the
> > toolkit CSS for trees.
> 
> We use the same styling in messenger-aero for the folderpane and messagepane
> http://mxr.mozilla.org/comm-central/source/mail/themes/qute/mail/messenger-
> aero.css#74

I don't see how that code themes differently the focused and non-focused selected item. Is there no difference on aero?

Or do I just need to add "imgroup" in the lists of selectors that contain imconv and imcontact already?
Comment 5 Andreas Nilsson (:andreasn) 2012-05-23 09:30:07 PDT
Yes, I think that should be enough. Will test it once I'm back on windows.
Comment 6 Florian Quèze [:florian] [:flo] 2012-05-24 03:00:44 PDT
I had something else to test on my Windows VM so I looked at this too. The folder pane appearance is identical whether it is focused or not (and I think it's a bug). The Windows 7 file explorer uses a blue highlight for selected and focused items, and a gray highlight for selected but without focus.
Comment 7 Andreas Nilsson (:andreasn) 2012-05-29 09:19:37 PDT
That would probably be bug #686959 and Jim said he had a patch to fix that.
Comment 8 Andreas Nilsson (:andreasn) 2012-05-30 06:01:37 PDT
Created attachment 628316 [details] [diff] [review]
fixes win7 aero appearance

This makes sure the patch don't create any regressions for the Aero case. The issue still remains that focused and selected items are identical, but perhaps we can tackle that in #686959 as it needs to be fixed for all other cases (folder view, address book etc.) as well.
Comment 9 Andreas Nilsson (:andreasn) 2012-05-30 07:55:32 PDT
Created attachment 628339 [details] [diff] [review]
and fixes for gnomestripe too

This should do the trick for both qute and gnomestripe.
Comment 10 Florian Quèze [:florian] [:flo] 2012-05-31 07:56:28 PDT
Comment on attachment 628339 [details] [diff] [review]
and fixes for gnomestripe too

Given that both Andreas an me have touched that patch for it to reach its current state, it's not clear any more who is the patch author and who is reviewer, so we would appreciate having someone else looking at it. Requesting review from Mike :-).
Comment 11 Mike Conley (:mconley) - (needinfo me!) 2012-05-31 08:14:37 PDT
Comment on attachment 628339 [details] [diff] [review]
and fixes for gnomestripe too

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

From inspection, this looks good - just one super-tiny nit. r=me with that fixed.

::: mail/themes/qute/mail/chat.css
@@ +117,3 @@
>  }
>  
> +

Remove this extra line.
Comment 12 Florian Quèze [:florian] [:flo] 2012-06-04 02:04:57 PDT
https://hg.mozilla.org/comm-central/rev/9b6d79b4c4cf

Note You need to log in before you can comment on or make changes to this bug.