383913 - Cosmetic fixes for threadpane

Status: NEW

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Matthias Bockelkamp]

Is I wrote in bug 348720, there are some issues with symbol columns in threadpane:

- The columns don't have the same size
- The symbols somehow look misaligned 

I first fixed the size of the columns by making the images for the column headers all 16x14 pixel. During that it turned out, that the wrong image was used for the junk column (folder-junk.png instead of junkcol.png). Second i moved some images around in their 16x14 canvas, in order to make them look centered in the column headers. Third i also resized dot.png to 16x14 to be consistent with the other symbols and applied some paddings to get the coulumns in line with the column headers. It turns out, that some paddings may in fact be removed. For details see the screenshots that i'll attach.

Comment 1

[Matthias Bockelkamp]

Attachment: Changes to threadpane.css

Comment 2

[Matthias Bockelkamp]

Attachment: Changes to the icons

Comment 3

[Matthias Bockelkamp]

Attachment: Screenshot of the situation without this fix

Comment 4

[Matthias Bockelkamp]

Attachment: Screenshot of the situation with this fix

Comment 6

[[e-mail]]

Two questions:
1. If we made all the images 16x16 would that eliminate all the padding?
2. Could the Modern theme be improved using similar changes?

Comment 7

[Matthias Bockelkamp]

1. I just replaced all images with 16x16 placeholders. The padding needed for the columns below the headers doesn't go away, but that's due to the fact, that my placeholders are not centered in the column headers. And I found an interesting problem: if the column has "cycler=true" it will be 1px taller. I'll investigate further...

2. I think so, but didn't try yet.

Comment 8

[[e-mail]]

(In reply to comment #7)
>1. I just replaced all images with 16x16 placeholders. The padding needed for
>the columns below the headers doesn't go away, but that's due to the fact,
>that my placeholders are not centered in the column headers.
I wonder whether that's a bug in the theme...

(In reply to comment #6)
>If we made all the images 16x16 would that eliminate all the padding?
Wait, I just realised that the *height* is 14... forget that question ;-)

Comment 9

[Matthias Bockelkamp]

It seems the modern theme (modern.jar) isn't available on my machine, so i cannot quickly construct a patch. Also, i won't be at home until October from saturday on... So if someone likes to complete this, just feel free to do so ;)

Comment 10

[Justin Wood (:Callek)]

Comment on attachment 267854 [details] [diff] [review]
Changes to threadpane.css

Matthias, is this issue still present on SM 2.0?

And if so, are you willing to update the patch(es) to fit it.

Comment 11

[Matthias Bockelkamp]

Yes, this issue is still present. A new patch is coming soon. Unfortunately, I currently have no CVS tree here, so I cannot diff against it.

Comment 12

[Matthias Bockelkamp]

Attachment: Changes to threadPane.css

Comment 13

[Matthias Bockelkamp]

Attachment: Modified icons (resized to 14x16)

Comment 14

[Matthias Bockelkamp]

Attachment: Screenshot 1 (current trunk)

Comment 15

[Matthias Bockelkamp]

Attachment: Screenshot 2 (with changes to threadPane.css)

Comment 16

[Matthias Bockelkamp]

Attachment: Screenshot 3 (with changes to threadPane.css and modified icons)

Comment 17

[Stanimir Stamenkov]

Comment on attachment 560217 [details]
Screenshot 2 (with changes to threadPane.css)

The "has attachment" column still seems of different width.

Comment 18

[Stanimir Stamenkov]

(In reply to Stanimir Stamenkov from comment #17)
Ah, replied too soon - attachment 560218 [details] (Screenshot 3) seems to complete it.

Comment 19

[Justin Wood (:Callek)]

Comment on attachment 560213 [details] [diff] [review]
Changes to threadPane.css

I can't review this code...

Comment 20

[[e-mail]]

Comment on attachment 560213 [details] [diff] [review]
Changes to threadPane.css

This seems vaguely reasonable, at least on Linux, which is where I've initially tested (I'll look at Windows tomorrow), although these days we do have to use the RTL-friendly styles rather than padding-right, and you only have to put the padding on the base instance of the image, because it cascades.

Comment 21

[[e-mail]]

Comment on attachment 560215 [details]
Modified icons (resized to 14x16)

So, what's the point of resizing the icons?

Comment 22

[Matthias Bockelkamp]

(In reply to [e-mail] from comment #20)
> initially tested (I'll look at Windows tomorrow), although these days we do
> have to use the RTL-friendly styles rather than padding-right, and you only

What is meant by "RTL-friendly styles"?

(In reply to [e-mail] from comment #21)
> So, what's the point of resizing the icons?

First to have an equal width for all column headers, based on the column picker header. In order to achieve this, all icons have to have a width of 14px. Second to center all icons in their canvas so that the column headers look nice.

Comment 23

[Philip Chee]

> What is meant by "RTL-friendly styles"?
-moz-padding-start
-moz-padding-end
These will automatically do the right thing in RTL mode.

Comment 24

[Matthias Bockelkamp]

(In reply to [e-mail] from comment #20)
> have to use the RTL-friendly styles rather than padding-right, and you only
> have to put the padding on the base instance of the image, because it
> cascades.

The only base instance that I could apply the padding to is treechildren::-moz-tree-image, but that would also affect many icons that shall not be changed.

Comment 25

[[e-mail]]

(In reply to Matthias Bockelkamp from comment #24)
> (In reply to [e-mail] from comment #20)
> > have to use the RTL-friendly styles rather than padding-right, and you only
> > have to put the padding on the base instance of the image, because it
> > cascades.
> The only base instance that I could apply the padding to is
> treechildren::-moz-tree-image, but that would also affect many icons that
> shall not be changed.
I was referring to your application of padding to both treechildren::-moz-tree-image(foo) and treechildren::-moz-tree-image(foo, bar)

Comment 26

[Matthias Bockelkamp]

Attachment: Changes to threadPane.css, v2

Comment 27

[[e-mail]]

Comment on attachment 560995 [details] [diff] [review]
Changes to threadPane.css, v2

> treechildren::-moz-tree-image(attachmentCol, attach) {
>-  list-style-image: url("chrome://messenger/skin/icons/attachment.png");
>+  list-style-image: url("chrome://messenger/skin/icons/attachment-col.png");
Sorry for overlooking this last time, but unfortunately this change doesn't make sense on its own, since the unselected and selected images need to resemble each other. Also, this is the only use of attachment.png so if you wanted to stop using it then you should remove it from jar.mn and the tree.

Comment 28

[[e-mail]]

(In reply to comment #27)
> the unselected and selected images need to resemble each other.
Or you could switch to a different image, but you would need one that looks OK both selected and unselected.

Comment 29

[[e-mail]]

Comment on attachment 560215 [details]
Modified icons (resized to 14x16)

thread-new-closed.png is definitely wrong because its colour changed.
thread-closed.png is also wrong because it is one of ten thread column icons so they would all need to be changed at once (and I don't like the 14x16 icons anyway, even though I know you wanted to resize the thread column header for consistency, but you could try asking Mnyromyr for review).
If you're going to resize dot.png then you should resize all the images that alternate with it, such as check.png, for consistency.

Comment 30

[Justin Dolske [:Dolske]]

Comment on attachment 560995 [details] [diff] [review]
Changes to threadPane.css, v2

I'm going through unassigned reviews; seems neil already r-'d this, so I'm just clearing the review request. Please attach a new patch, and/or specifically flag neil for review again. Thanks!