494430 - Threadpane column icons not centered in new Windows XP theme

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect, P4)

Description

[Phil Ringnalda (:philor)]

Attachment: Screenshot

Bug 488107 shuffled around the margins and paddings for the icons that are displayed in the header of the read, attachment, starred, and junk columns, but whatever alignment that was trying to do, the end result was making them very much offcenter: the attachment and star icons are a little low and a little right, the read icon is way high (possibly partly because of the image itself) and jammed against the right side, the junk icon is low and jammed against the right side.

Comment 1

[Bryan Clark (DevTools PM) [:clarkbw]]

that's really strange.  they are perfectly centered on mine... I can't understand what the difference would be.

Comment 2

[Phil Ringnalda (:philor)]

Where are you seeing them centered? They look fine for me on Win7 in Aero, and only slightly off on two separate XP installs in Classic (which burns more of the space with shaded bars between sections, so there's less off to be off) but totally busted on both XPs in Luna.

Comment 3

[Bryan Clark (DevTools PM) [:clarkbw]]

ah, sorry.  i've been looking at them in Win7

Comment 4

[Phil Ringnalda (:philor)]

Well then, lucky thing we're going to have an Aero theme, where I look forward to seeing those padding changes after we take them out of non-Aero! :)

Comment 5

[Bryan Clark (DevTools PM) [:clarkbw]]

now that you've landed bug 468751 it's probably a good time to look at getting this fixed.  I'm trying to get an XP VM together but I already have some questions.

Do we copy the changed parts of mailWindow1 from attachment 377356 [details] [diff] [review] into a mailWindow1-aero.css?

Giving us:
mail/themes/qute/mail/mailWindow1.css
mail/themes/qute/mail/mailWindow1-aero.css

And what is the name mailWindow1 about?  I've been resisting the urge to rename that file something sane.

Comment 6

[Phil Ringnalda (:philor)]

See, for instance, http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/browser/places/organizer-aero.css - you add mailWindow1-aero.css, preprocessed so you can |%include mailWindow1.css| at the start, then put just the changed bits that you want to override in mailWindow1-aero.css:

treecol.readColumnHeader {
  -moz-padding-end: 1px;
  -moz-padding-start: 1px;
}

and put mailWindow1.css back like it was, since the later rules in mailWindow1-aero.css will override the earlier ones from mailWindow1.css.

Comment 7

[Phil Ringnalda (:philor)]

Oh, mailWindow1.css - back in the day, there were two layouts, classic and vertical, and vertical was produced by a different XUL file which applied mailWindow2.css (which @imported mailWindow1.css). So, the name's an artifact, but renaming it would make life even more difficult for third-party themers, without any really solid benefit.

Comment 8

[Bryan Clark (DevTools PM) [:clarkbw]]

Well I'm already making so many lives completely painful right now I figure it's a good time to slip something else in.  Though I'm really too lazy to actually do that renaming work.

And despite your instructions leading me exactly to water my mailWindow1-aero.css is ignoring the * in front of it and doesn't seem to be getting processed.

Just to check that the CSS override works I did this hack to make the CSS import at runtime and everything seems to work as expected.

jar.mn:
  skin/classic/aero/messenger/mailWindowXP.css                     (mail/mailWindow1.css)
  skin/classic/aero/messenger/mailWindow1.css                      (mail/mailWindow1-aero.css)

mailWindow1-aero.css:
@import url("chrome://messenger/skin/mailWindowXP.css");
treecol.readColumnHeader {
  -moz-padding-end: 1px;
  -moz-padding-start: 1px;
}

treechildren::-moz-tree-image(unreadButtonColHeader) {
  -moz-padding-start: 0px;
  -moz-margin-start: -3px;
}
 
treecol.attachmentColumnHeader {
  -moz-padding-start: 5px;
}

treecol.flagColumnHeader {
  -moz-padding-start: 5px;
}

treecol.junkStatusHeader {
  -moz-padding-end: 1px;
  -moz-padding-start: 1px;
}

treechildren::-moz-tree-image(junkStatusCol) {
  -moz-margin-start: -3px;
}

I'll post the patch up here since it *looks like it should work* and yet it's not.

Comment 9

[Bryan Clark (DevTools PM) [:clarkbw]]

Attachment: XP and Vista differences pulled apart

Here's the patch.  Turns out I had some classic Windows garbage characters sitting in front of my %include statement.  I'd like to thank Visual Studio Express for taking the last hour or so of my life away from, not sure what that was about.

Comment 10

[Phil Ringnalda (:philor)]

Attachment: Post-patch XP screenshot

Considerably improved, but either we were a touch offcenter before, or we need to do a little tweaking to really center the new images on XP.

Comment 11

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 381630 [details] [diff] [review]
XP and Vista differences pulled apart

minus since this does the revert but isn't enough.  

I need to get a real XP build since "running in XP environment" is doing some real strange stuff to the build files

Comment 12

[Bryan Clark (DevTools PM) [:clarkbw]]

assigning to davida who has an XP build. take patch and run

Comment 13

[Bryan Clark (DevTools PM) [:clarkbw]]

Does anyone have an XP build?

Comment 14

[Bryan Clark (DevTools PM) [:clarkbw]]

blake, hot potato time!  do you have an XP build?

Comment 15

[Blake Winton (:bwinton) (:☕️)]

Yup, I do.

Comment 16

[Blake Winton (:bwinton) (:☕️)]

Attachment: New XP screenshot, post new patch.

I'll make sure Win7 also looks pretty, before I submit the new patch.

Comment 17

[Blake Winton (:bwinton) (:☕️)]

Attachment: And the Win7 post new patch screenshot.

Comment 18

[Blake Winton (:bwinton) (:☕️)]

Attachment: The patch.

Let's see how you all like this version of the patch.

Comment 19

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 404912 [details] [diff] [review]
The patch.

works for me

Comment 20

[Blake Winton (:bwinton) (:☕️)]

Attachment: The new patch.

Philor pointed out some real odditites in XP Classic with the previous patch.
This one should fix them, along with XP New, and Win 7.

Thanks,
Blake.

Comment 21

[Phil Ringnalda (:philor)]

Comment on attachment 405343 [details] [diff] [review]
The new patch.

Now that, that's purty!

Comment 22

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 405343 [details] [diff] [review]
The new patch.

well things appear the same on vista and that's about all I ever ui-r'd before so i'll just go with the flow since I don't have XP.

Comment 23

[Phil Ringnalda (:philor)]

http://hg.mozilla.org/comm-central/rev/c9a156770bc3