404097 - Larry panel has no border with some Gtk engines (Clearlooks, ...)

Status: RESOLVED WORKSFORME

(Firefox :: Theme, defect)

Description

[Sylvain Pasche]

Attachment: drop background-color

On Linux, I don't see any border around the Larry panel.

Apparently, the background color of a panel is drawn above its border. Without a background-color, the panel is still white as before.

Comment 1

[Asaf Romano (gone)]

Comment on attachment 289057 [details] [diff] [review]
drop background-color

Still no Linux Box here. Maybe try Gavin or Myk?

Comment 2

[Sylvain Pasche]

Comment on attachment 289057 [details] [diff] [review]
drop background-color

I'm seeing now that this depends on the Gtk engine used. With the Ubuntu Human engine the border is visible, but not with Clearlooks or Glossy engines. That might also be a widget/Gtk issue.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Johnathan, do you remember why you added this background color?

Comment 4

[Johnathan Nightingale [:johnath]]

(In reply to comment #3)
> Johnathan, do you remember why you added this background color?

Hmm.  I feel like, on either mac or windows, not specifying a background-color made it grey (or at least, some system default like list-background).  Isn't there a platform bug here though, if we're painting this wrong?

I'll check Mac and Vista and see what happens.

Comment 5

[Johnathan Nightingale [:johnath]]

Oh, I'm an idiot, this patch only touches gnomestripe.  In that case, my comment about the platform bug stays, but I certainly had no linux-specific reason to do it, nor any linux-specific reason to resist changing it, if it fixes the problem across the various engines.

Comment 6

[Takeshi Kurosawa :takenspc]

Comment on attachment 289057 [details] [diff] [review]
drop background-color

With this patch, most part of Larry is unreadable on dark theme.
Dropping |color| is also needed.

#identity-popup-content-box > description,
#identity-popup-encryption-label {
    white-space: -moz-pre-wrap;
    color: black;
    padding-left: 15px;
    margin: 2px 0 4px;
}

should be

#identity-popup-content-box > description,
#identity-popup-encryption-label {
    white-space: -moz-pre-wrap;
    padding-left: 15px;
    margin: 2px 0 4px;
}

Comment 7

[Takeshi Kurosawa :takenspc]

Attachment: drop background-color and color

Merge my comment into Sylvain's patch.

Comment 8

[Sylvain Pasche]

So your patch does not change the appearance with the default theme? (I didn't test it)
Can you ask for review?

Comment 9

[Takeshi Kurosawa :takenspc]

Attachment: Screenshot with patch

Screenshot with patch on Windows XP (classic theme).

Dropping |background-color| affects look of the Larry on Windows Classic theme the most.
# background is changed white to gray.

On other default themes on Windows and Clearlooks theme on Linux, almost same.

Comment 10

[Takeshi Kurosawa :takenspc]

Comment on attachment 305165 [details] [diff] [review]
drop background-color and color

Gavin, could you review this?
# ui-review is needed?

Comment 11

[Sylvain Pasche]

hmm, I didn't see you were changing winstripe/pinstripe too. This is out of scope of this specific issue (Linux only). What about you file another bug for this, and we stick to my previous gnomestripe-only patch?
(Sorry, I should have looked closer before reassigning and asking you for the review)

Comment 12

[Takeshi Kurosawa :takenspc]

Attachment: drop background-color and color (only for gnomestripe)

Patch for only gnomestripe.

Sorry, I had misunderstood.
Dropping |background-color|, we must drop |color| too. Because |color: black| causes invisible text on dark theme.

Comment 13

[Takeshi Kurosawa :takenspc]

Comment on attachment 305196 [details] [diff] [review]
drop background-color and color (only for gnomestripe)

This patch won't produce any side-effects on Clearlooks.

Comment 14

[Takeshi Kurosawa :takenspc]

Attachment: Screenshot with patch

Comment 15

[Takeshi Kurosawa :takenspc]

(In reply to comment #11)
> What about you file another bug for this

I filed bug 419186

Comment 16

[Reed Loden [:reed]]

Comment on attachment 289057 [details] [diff] [review]
drop background-color

Unset the request if you don't want review. Don't leave it dangling.

Comment 17

[Dão Gottwald [:dao]]

The background color isn't hardcoded to white anymore, so this should be fine.