Closed Bug 1003295 Opened 10 years ago Closed 10 years ago

Win8: Text color on inactive tabs makes text very hard to read (FX bug 940393)

Categories

(Thunderbird :: Theme, defect)

All
Windows 8
defect
Not set
normal

Tracking

(thunderbird31 fixed)

RESOLVED FIXED
Thunderbird 32.0
Tracking Status
thunderbird31 --- fixed

People

(Reporter: Paenglab, Assigned: Paenglab)

Details

Attachments

(1 file, 3 obsolete files)

Win( uses always black as titlebar text color. This makes it hard to read inactive tabs text (see attachment 8334525 [details]).

Fx fixed this with bug 940393 and bug 988191.
Attached patch Win8-tabReadability.patch (obsolete) — Splinter Review
The JS part is a copy of bug 940393 and bug 988191 (no change was needed). So I would say this code itself isn't needed to review but the position where it is inserted.

Philipp, I added you for the Calendar CSS code.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8414632 - Flags: ui-review?(mconley)
Attachment #8414632 - Flags: review?(philipp)
Attachment #8414632 - Flags: review?(mconley)
Attached patch Win8-tabReadability.patch (obsolete) — Splinter Review
Not all Lighning icons where correctly shown on tabbar-toolbar in customize mode.
Attachment #8414632 - Attachment is obsolete: true
Attachment #8414632 - Flags: ui-review?(mconley)
Attachment #8414632 - Flags: review?(philipp)
Attachment #8414632 - Flags: review?(mconley)
Attachment #8414743 - Flags: ui-review?(mconley)
Attachment #8414743 - Flags: review?(philipp)
Attachment #8414743 - Flags: review?(mconley)
Attachment #8414743 - Flags: review?(philipp) → review+
Attached patch Win8-tabReadability.patch (obsolete) — Splinter Review
Updated to follow also bug 1007229.

Mike, could you only look if the position of the new JS code in msgMail3PaneWindow.js is at the correct position? Where is no code change (except the added comment at the beginning) to the FX code.

Josiah, is it okay I added you for the r and ui-r of the CSS code?
Attachment #8414743 - Attachment is obsolete: true
Attachment #8414743 - Flags: ui-review?(mconley)
Attachment #8414743 - Flags: review?(mconley)
Attachment #8420575 - Flags: ui-review?(josiah)
Attachment #8420575 - Flags: review?(mconley)
Attachment #8420575 - Flags: review?(josiah)
Comment on attachment 8420575 [details] [diff] [review]
Win8-tabReadability.patch

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

The code looks okay, but I haven't looked at the product myself, hopefully Mike can. I'm very busy for the next couple of days, so if he hasn't ui-reviewed by Wednesday then feel free to re-assign the flag.
Attachment #8420575 - Flags: ui-review?(josiah)
Attachment #8420575 - Flags: review?(josiah)
Attachment #8420575 - Flags: review+
Comment on attachment 8420575 [details] [diff] [review]
Win8-tabReadability.patch

Unfortunately Mike had no time. Josiah, if you have time, could you look at it? The try build should still work.
Attachment #8420575 - Flags: ui-review?(josiah)
Comment on attachment 8420575 [details] [diff] [review]
Win8-tabReadability.patch

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

Looks okay to me.
Attachment #8420575 - Flags: ui-review?(josiah) → ui-review+
Comment on attachment 8420575 [details] [diff] [review]
Win8-tabReadability.patch

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

This looks fine.

I filed bug 1013654 to get some real platform support on the menu / tabs toolbar colours, but until we get something there, I guess forking / cloning the Windows8WindowFrameColor.jsm isn't so bad. Maybe file a bug that depends on bug 1013654 to remote the JSM when platform support is available.

You might want Fallen's r+ on the Lightning bits.

::: mail/base/content/msgMail3PaneWindow.js
@@ +400,5 @@
>    UpdateMailPaneConfig(false);
>    document.loadBindingDocument('chrome://global/content/bindings/textbox.xml');
> +
> +#ifdef XP_WIN
> +  // On Win8 set an attribute when the window frame color is to dark for black text.

Nit: s/to/too

::: mail/themes/windows/mail/tabmail-aero.css
@@ +229,5 @@
> +  }
> +
> +  #messengerWindow[darkwindowframe="true"] .tabmail-arrowscrollbox >
> +  .scrollbutton-up:not(:-moz-lwtheme):not(:-moz-window-inactive),
> +  #messengerWindow[darkwindowframe="true"] .tabmail-arrowscrollbox

Have we always broken up lines like this? I find it so hard to read. :/ I'd honestly prefer to have the rules on one line, and not worry about the 80 char limit here. Feel free to do that.
Attachment #8420575 - Flags: review?(mconley) → review+
(In reply to Mike Conley (:mconley) from comment #8)
> Comment on attachment 8420575 [details] [diff] [review]
> Win8-tabReadability.patch
> 
> Review of attachment 8420575 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mail/base/content/msgMail3PaneWindow.js
> > +
> > +#ifdef XP_WIN
> > +  // On Win8 set an attribute when the window frame color is to dark for black text.
> 
> Nit: s/to/too

Done

> ::: mail/themes/windows/mail/tabmail-aero.css
> @@ +229,5 @@
> > +  }
> > +
> > +  #messengerWindow[darkwindowframe="true"] .tabmail-arrowscrollbox >
> > +  .scrollbutton-up:not(:-moz-lwtheme):not(:-moz-window-inactive),
> > +  #messengerWindow[darkwindowframe="true"] .tabmail-arrowscrollbox
> 
> Have we always broken up lines like this? I find it so hard to read. :/ I'd
> honestly prefer to have the rules on one line, and not worry about the 80
> char limit here. Feel free to do that.

Yes, Blake was very strict on this, I leave it here as it is.
Attachment #8420575 - Attachment is obsolete: true
Attachment #8426062 - Flags: ui-review+
Attachment #8426062 - Flags: review+
Keywords: checkin-needed
Comment on attachment 8426062 [details] [diff] [review]
patch for check-in

[Approval Request Comment]
User impact if declined: On dark wndow frames the user could have problems to read the tab titiles
Testing completed (on c-c, etc.): in c-c, the JS code is in m-c since FX 31
Risk to taking this patch (and alternatives if risky): low, code is already tested i FX
Attachment #8426062 - Flags: approval-comm-aurora?
https://hg.mozilla.org/comm-central/rev/007931222e16
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 32.0
Attachment #8426062 - Flags: approval-comm-aurora? → approval-comm-aurora+
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: