97534 - [review]classic: after changing text color, text [labels] in dialogs/chrome also changed in color

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[sairuh (rarely reading bugmail)]

spun off of bug 90081. on Mac --both OS X and OS 9.1-- after changing the text
color pref, the "Category" text above the prefs category tree appears changes
color, rather than remaining black. seen using 2001.08.29.0x-comm bits.

1. open prefs dialog and go to Colors panel.
2. change the Text color from black to something else --eg, i chose bright green.
3. click OK to save/dismiss prefs. [sidenote: you won't see this change
immediately unless you reload due to bug 90081.]
4. open up the prefs again. observe the color of the "Category" text.

will attach shot soon.

Comment 1

[sairuh (rarely reading bugmail)]

this turns out to affect a lot more in the product --i notice that text [labels]
color in other dialogs is changed! for example:

Open Web Location dialog
Helper app dialog
Download progress dialog
alert dialogs

Comment 2

[sairuh (rarely reading bugmail)]

heh, i'm also noticing the color change in text in the browser chrome... anyhow,
the upcoming attachment should display colored text in both a dialog and the
toolbar text...

Comment 3

[sairuh (rarely reading bugmail)]

forgot to note in previous comment: what i see reminds me of bug 22963!

Comment 4

[sairuh (rarely reading bugmail)]

Attachment: shot showing colored text in dialog and toolbar

Comment 5

[sairuh (rarely reading bugmail)]

tentatively nominating for 0.9.4...

another note re: toolbar text color change: that particular change took effect
after i had exited and restarted the app.

Comment 6

[Marc Attinasi]

David, what do you make of this? I cannot see how the chrome is being affected,
especially after a restart!

Comment 7

[David Hyatt]

Wow, that's crazy.  I have no idea how this could happen.

Comment 8

[Mike Pinkerton (not reading bugmail)]

pierre might know why color changes are showing up in different parts of the app. 
any thoughts?

Comment 9

[Pierre Saslawsky]

I cannot reproduce it with a 2001-08-30 Netscape6 build on OS 9.1.  I tried some 
Mozilla builds too.  I find it very surprising that the bug is platform 
dependent.  Make sure you try on other platforms with the same set of preferences 
as on the Mac.

Does it happen on a new profile for instance?
Does it happen with the background color too, or only with the text color?
Does it happen only when "Use my colors" is selected, or also with "Use 
document's colors"?

If it happens only with "Use my colors", then we can suspect something related to 
the backstopStyleSheet created in PresShell::CreatePreferenceStyleSheet() and 
filled up in PresShell::SetPrefColorRules().  The style rules created for these 
prefs are unoverridable (backstop-important).

If it happens only with "Use document's colors" or with both settings, I would 
look at nsRuleNode::ComputeColorData(). Note also the following feature of the 
color structure, which I don't think is relevant but who knows?
http://lxr.mozilla.org/seamonkey/source/content/shared/public/nsStyleStruct.h#141

Comment 10

[Brad Tiffany]

I can see this with the (mac) classic theme but not with the modern theme.  For
what it is worth, the mac classic's global.css file does not specify a color for
the XUL window element (it does specify a background-color), whereas the modern
and windows classic versions of global.css do specify a color for XUL windows. 
Adding such a specification to mac classic's global.css seems to fix the problem.

Comment 11

[Marc Attinasi]

Good call, Brad. I confirmed that this is occuring on the Classic theme on OS9,
and not on the Toy Factory or Modern themes. 

What is really strange is that the color pref style rules are not suppossed to
be kicking in for chrome documents...

Comment 12

[Pierre Saslawsky]

Marc: see my comments from [2001-08-31 04:16] for the prime suspects in this bug.

I can reproduce the bug with both settings: it means that the the pref rules 
cannot be the cause of the bug (PresShell::SetPrefColorRules creates the rules 
only if useDocColors is false). 

Comment 13

[Marc Attinasi]

I think that this is not my bug. From Pierre's comments, it sounds like a style
bug (best guess at this point)

>If it happens only with "Use document's colors" or with both settings, I would 
>look at nsRuleNode::ComputeColorData().

Over to Style Component (althought it could be a Skins issue, at least the
symptom can be solved there, apparently).

Comment 14

[Marc Attinasi]

_Really_ over to style system now...

Comment 15

[Pierre Saslawsky]

I'll take it.

Comment 16

[David Hyatt]

Is this actually a style system bug?  If the skin specifies no color at all,
isn't the default color going to come from the presshell (which looks at the
pref)?  I'm wondering if this is just a skin bug with the Mac Classic skin.

Comment 17

[David Hyatt]

I guess the assumption is that the color specified in prefs only should apply to
content areas and not to chrome areas.... so what color does chrome get by
default then? Black? :)

Comment 18

[Pierre Saslawsky]

In PresShell::SetPreferenceStyleRules(), we check whether this is not a chrome 
shell before calling SetPrefColorRules().  The problem is that the style system 
get the default colors from the PresContext which reads them directly from the 
prefs.

The default colors should be eColor_WindowForeground and eColor_WindowBackground, 
which is what the PresContext does when the 'use_system_colors' pref is set.

Comment 19

[sairuh (rarely reading bugmail)]

fooey, i wanted to dblcheck this with the modern theme, but bug 98415 is
blocking me there.

Comment 20

[Pierre Saslawsky]

sairuh: apparently the bug cannot be reproduced with a modern skin but if you 
want to try it, you can change the prefs with the classic skin, switch to modern 
and restart.

Comment 21

[Bill McGonigle (not currently reading bugmail; please contact directly)]

I get this in Modern - I have screenshots on bug 98731.  I'll close that.  These 
are the attachments I made to illustrate.  Can attachments be moved?

http://bugzilla.mozilla.org/attachment.cgi?id=48606&action=view
http://bugzilla.mozilla.org/attachment.cgi?id=48616&action=view

Comment 22

[Bill McGonigle (not currently reading bugmail; please contact directly)]

Oh, and I don't think I've ever been in a color picker in Mozilla...

Comment 23

[Pierre Saslawsky]

Bug 98731 looks like a very different problem.

Comment 24

[sairuh (rarely reading bugmail)]

pierre, thx for the test-workaround. yep, don't see this in modern.

Comment 25

[Pierre Saslawsky]

I'll attach a fix.

someone please review
hyatt please super-review

Comment 26

[Pierre Saslawsky]

Attachment: patch 1.0

Comment 27

[Pierre Saslawsky]

someone please review
hyatt please super-review

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Why did you change nsPresContext::SetContainer?  (Why should that be an
indicator that something has changed?  And what is it that has changed?)

Comment 29

[David Hyatt]

r/sr=hyatt

Comment 30

[Pierre Saslawsky]

The PresShell is created/initialized without a container and the default colors 
are read from the prefs.  Later, when a container is set on the PresShell, we 
reload the colors.  If the container is a chrome, we get the colors from the 
LookAndFeel.  If it is a content, we get the colors from the prefs.

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

r=dbaron, although I still don't see where the chrome/non-chrome check is being
done.

Comment 32

[Pierre Saslawsky]

The check is done in nsPresContext::GetDocumentColorPreferences() with this line:
  if (nsIDocShellTreeItem::typeChrome == docShellType)

Fix checked into the trunk.

Comment 33

[Madhur Bhatia]

v