22963 - text & bkgn color changes only occurs in skin (mostly), not content

Status: VERIFIED FIXED

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

Description

[sairuh (rarely reading bugmail)]

occurred w/2000010308 on mac and linux (unable to test on windows due to bug
21160). let me know if this should be reclassified under XUL...

to repro:
1. go to Preferences > Appearance > Colors
2. change the text and page background colors --eg, i made the text lime green
and the background black.
3. make sure that "Always use my colors" checkbox is selected.
4. click OK

expected: the text and background colors of the browser content should have the
new color scheme.

results: well, several strange things here...

a. some text in the chrome --specifically, Search in the navigation toolbar and
the text (URL and build ID) at the bottom in the status bar-- have the new text
color.

b. in the Prefs dialog, the text color (both the tree and rest of the dialog)
is the new color.

c. text and bkgnd colors in the page content aren't immediately updated once
leaving Prefs. moreover, even though i selected "Always use my colors" not all
pages would use my selected text and backgnd colors. why would this occur? for
example:

* http://www.mozilla.com always remained unchaged (black text, white
background)

* http://www.palmgear.com only the text color changed (background still white)

* Dogfood Snacks page, however, was a simple page in which the background and
text colors were updated

Comment 1

[don]

Move to M15 since this is not a beta 1 requirement.

Comment 2

[leger]

Bulk move of all Pref UI component bugs to new Preferences component.  Pref UI 
component will be deleted.

Comment 3

[Peter Trudelle]

Adding 'skins' keyword to appropriate bugs en masse, sorry about any 
mistakes...

Comment 4

[don]

Move to M16 for now ...

Comment 5

[Peter Trudelle]

Mass-adding beta2 keyword to all skins bugs.

Comment 6

[Michael La Guardia]

putting on nsbeta2- radar per pdt

Comment 7

[matt]

found out for kathy brade that
we need to do this in c++ so i wanted to pawn this
off to mcafee.  I implemented something in javascript
but never got it to work correctly.


"
It seems to me that this change would need to be queried and set in layout 
itself (though I haven't really thought about it).  Oh, and by the way, there
is currently a bug (cmanske has it) that setting the background color doesn't 
properly render the whole window as it should (in case you are
experimenting).  This bug may or may not affect you in the browser window. "

The code is in nsHTMLEditor::SetBackgroundColor (line 3603 or so).  I don't 
think it will help you though. 
Here is the snippet of code you are inquiring about: 
  { 
    res = nsEditor::GetBodyElement(getter_AddRefs(element)); 
     if (NS_FAILED(res)) return res; 
     if (!element)       return NS_ERROR_NULL_POINTER; 
   } 
   // Use the editor method that goes through the transaction system 
   return SetAttribute(element, NS_ConvertASCIItoUCS2("bgcolor"), aColor); 

Comment 8

[don]

Move to M20 target milestone.

Comment 9

[fantasai]

Attachment: Testcase for 'c' (prefs override broken)

Comment 10

[fantasai]

The background/text color override bug may be related to bug 43220.

--Could someone please add the 'testcase' keyword to this?

Comment 11

[johng]

nav triage team:
[NEED INFO]
QA, please verify that this is still a problem.
Reassigning to Matt who had it earlier.

Comment 12

[johng]

nav triage team:
is anyone there?  If this problem still exists, we probably should fix it. Could 
someone verify on more recent builds?  How hard is this to fix?

Comment 13

[sairuh (rarely reading bugmail)]

this is still a problem using the commercial 2000.08.08.08-m18 bits. tested on
linux and mac. but somehow i cannot get text or background color changes to work
on winnt. separate bug? bueller...?

Comment 14

[matt]

I'm not sure why i got this bug again.
I was trying to pawn it off on macfee and will continue
to do so since he is much more efficient at doing the backend
colors stuff.

Comment 15

[matt]

giving to mcafee to see if he knows more as to what is going on.

Comment 16

[Phil Peterson]

PDT downgrading to P4 and leaving [PDTP4] in status whiteboard

Comment 17

[sairuh (rarely reading bugmail)]

spam: adding mostfreq, not necessarily due to many dups, but because i run into 
this problem frequently (thus want to get it my queries easily).

Comment 18

[fantasai]

Retested on Mozilla nightly build (id: 2000083111) on Windows 2000:

Classic - 
	Chrome is not affected except in the small corner between the 
	scrollbars. This change occurs whether or not "always use my colors" is 
	selected.

Modern - 
	In addition to changing to the background color in the corner as in
	Classic, the Modern chrome changes to the new text color in the same 
	places it uses system colors in bug 46956. This change occurs whether or
	not "always use my colors" is selected.

The document - 
	As aforementioned, the prefs override is broken. Any page specifying a 
	background color overrides the background color in the preferences, just 
	as if the "always use my colors" was not selected.

	This is what the focus of this bug should be on; this is what needs to
	be fixed for nsbeta3 because it presents accessability problems for
	anyone reading a document with low-contrast colors.

	The page colors /do update/ once the preferences colors are changed.
	Again, this change occurs whether or not "always use my colors" is 
	selected.
	There is an exception to that; it's reported as bug 51066.
	If there is not problem with updating the page colors on other systems,
	then that aspect of 'c' can be eliminated from this bug report. I can
	only test on Windows right now; if anyone else would be willing to run 
	a quick test on another OS, there's a test case in bug 51066.

Comment 19

[johng]

nav triage team:
nsbeta3- at this point, will create a new bug to remove the text from the pref 
panel.  This feature is being cut at this point in the schedule, see bug 53276.

Comment 20

[don]

Uncutting this and making it "nsbeta3+" again.  John misunderstood the bug.
Without a fix for this the "Colors" is pref panel is completely useless.  This
is not an XPApps/Navigator team bug though.  We are correctly setting the pref.
 There is something wrong with the style system or the toolkit.  Chris McAfee is
not going to be able to fix this.  And we can't ship without a color prefs panel.

Sending over the the style system folks.

Comment 21

[don]

Pierre,

Ben Goodger and I have looked through the code and it appears that Troy wrote
the original implementation which retrieved this prefs in "nsPresContext.cpp".
Hope that helps.

Comment 22

[karnaze (gone)]

Assigning to Marc.

Comment 23

[Marc Attinasi]

I see two general problems we need to solve to consider this bug fixed:

1) changes to the text and background color pref should NOT affect the chrome
2) when a user chooses to ALWAYS use their specified text and background color
we need to either *subvert* the style system, or we need to insert some style
rules that will cause the author's text and background colors to be overridden.

For problem 1 we need to change the code that calls GetDefaultColor and
GetDefaultBackgroundColor to pass a flag or something indicating whether the
pref should be ignored (TRUE for XUL). And what about XML elements? Should we
apply the pref to HTML elements only?

For problem 2 we could use the CSSOM to insert some rules into an override
stylesheet. The rules depend on how we want to do it, and I am not sure how it
is suppossed to work. Nav 4.x seems to make ALL backgrounds the color specified,
including backgrounds on tables and DIVs. It also seems to make all text the
color specified except for link text, and text that is colored using the FONT tag.

I'm not sure who designed this feature, or if it was just copied from Nav 4, but
there is clearly nothing in place right now to make this work in the Style
System. Do we really want to create new code to insert / remove rules to make
this work? If so, how are we going to decide what elements the background and
text color apply to?

This late in the game, I'd at least pull the 'override the document's colors'
checkbox and relnote the issue, however problem #1 above still needs to be
handled. The other alternative is to drop the color pref altogether, but
[e-mail] has indicated that we cannot ship without a color pref...

I'm happy to try and take care of this, but I need some kind of direction (a
design spec for the Color Pref would be a good start).

(NOTE: These issues are similar to ones we will have in bug 31816, regarding fonts.)

Comment 24

[Phil Peterson]

PDT thinks is a P2

Comment 25

[fantasai]

>1) changes to the text and background color pref should NOT affect the chrome

As I recall, the fix for bug 46956 takes care of the text color changes in the 
chrome. The color will still be /there/ (which is a bug), but it's overridden, 
the same way as it's overridden in the Classic skin.

> And what about XML elements? Should we apply the pref to HTML elements only?

XML and HTML should receive equal status as The Document, no? And then you'd 
wind up with a rather curious inconsistency if XHTML documents rendered 
differently than HTML.

> 2) when a user chooses to ALWAYS use their specified text and background color
> we need to either *subvert* the style system, or we need to insert some style
> rules that will cause the author's text and background colors to be 
> overridden.

It would be nice if the override settings entered the style system at the user 
!important level. However, this is blocked by bug 43220 -- so you can either 
subvert the style system or fix it. :-)

> This late in the game, I'd at least pull the 'override the document's colors'
> checkbox and relnote the issue

As I've noted, lack of this feature presents problems when you need to override 
the author's low-contrast settings in order to read something.

Comment 26

[sairuh (rarely reading bugmail)]

fantasai wrote:

"> This late in the game, I'd at least pull the 'override the document's colors'
> checkbox and relnote the issue

As I've noted, lack of this feature presents problems when you need to override 
the author's low-contrast settings in order to read something."

btw, this is addressed in bug 53276.

Comment 27

[fantasai]

:root {
	background-color: blue;
	color: white;
}

Works quite well in userContent.css (though I haven't tested it with XML).
Can you emulate that instead of fussing with the default-background code?

Comment 28

[Marc Attinasi]

fantasai, the style rules you mentioned really do not solve the problem.

In Nav, the colors override almost all other colors in the markup, but the rules
on the :root will only cascade according to normal css rules, which means they
will be overridden much of the time. If the rules were cascaded as user
!important rules it would probably work, but that is broken (bug 43220).

Here is a simple example, not even using CSS:

<body bgcolor='red'>
<font color='yellow'>
<p>This is red test </p>
</font>
</body>

In Nav with the 'use my colors no matter what' setting enabled, the bgcolor on
the body is ignored (as are bgcolors on tables, try tinderbox!) and the color on
the font is applied. In Mozilla, we honor both the bgcolor and font color even
if the :root rules are in userContent.css, even if marked !important.

Also, when I set those rules in userContent.css almost every page that I visit
looks like hell, as does mail - am I missing something?

Comment 29

[Phil Peterson]

Not holding PR3 for this one. Marking nsbeta3-, and adding rtm nomination to 
address this in an actual fix, or by removing the nonworking feature.

Comment 30

[karnaze (gone)]

Adding rtm+.

Comment 31

[lchiang]

phil's comment says nsbeta3- so I'm doing that.

Comment 32

[fantasai]

> fantasai, the style rules you mentioned really do not solve the problem.
I meant that in reference to point 1 ("changes to the text and background color 
pref should NOT affect the chrome")

For overriding with CSS in the userContent stylesheet, you'd have to do this:
:root, :root * {background-color: [color] !important}
It works--I tried it. Unfortunately, it also colors the GFX widgets on the 
page; apparently the fix for bug 21890 doesn't affect the user stylesheet.

Comment 33

[Marc Attinasi]

I see, fantasai, I misunderstood. The rules you just recommended do seem to do
the trick, in most cases anyway. Maybe we could limit the application of the
rules to the xml and xhtml namespaces and eliminate the application to the
xul-widgets?

With a userContent.css like this:

@namespace url(http://www.w3.org/1999/xhtml); /*default namespace HTML*/
*:root, *:root * {
 background-color: bisque !important;
 color: purple !important;
}

the scrollbars are at least not affected. However, we still differ from Nav in
that Nav continues to color links differently, and of course the native controls
are always in their native color... Adding rules like:

*:root *:link {
  color: blue !important;
}
*:root *:visited {
  color: red !important;

takes care of the link issue (and solves the related problem of the link-color
options pref being unreliable). Maybe we would need XML-namespaced rules as
well, to cover XML documents?

I am starting to like this solution, since it takes care of most of the issues,
and probably will help most people with the need to set their own colors. I'll
look into getting these rules inserted/removed via the CSSOM so we don't have to
deal with the problems of updating the userContent.css file once the app is
running (I think that is a cleaner approach anyway).

Comment 34

[Hixie (not reading bugmail)]

Unless I'm missing something, the mentions of :root in those rules are 
redundant. The only difference between

   :root, :root * { }

...and

   * { }

...is the specificity, which is itself only relevant if there are other rules
in the userContent.css file. I would guess that the first version is 
considerably slower to resolve, too.

Comment 35

[Phil Peterson]

PDT marking [rtm need info] until the patch has been code reviews

Comment 36

[Marc Attinasi]

I'm working this out now, inserting rules to enforce the user's color choices
ALWAYS when they have chose the option "use my chosen colors, ignoring the
colors specified". However there is this small issue of what to do about
Composer: should the user's color choices be applied in composer as well? In Nav
4.7, it only partially applies, such that colors are honored initially, but they
can be changed by using the color pickers in Composer...

If I follow through with my current implementation plan then the colors
(background and text) will apply in Composer *even if the user changes the color
of something* in composer, when they have checked the 'use my chosen colors,
ignoring the colors specified' option in the preference. Somebody scream if they
think this is wrong, because it will require additional handling in Composer...

Comment 37

[Matthew T (active 1999-2002)]

I'm using 4.7 for Mac with `Always use my colors' turned on, and they're ignored 
by Composer. It uses black/white/blue/purple. However, I think Marc's suggestion 
is the right one in the short term -- if the user changes the color of something 
in Composer and it doesn't look like it's changed color, they'll know why, 
because they will (probably) have been the ones who turned on the `Always use my 
colors' pref in the first place.

In the long term, though, you'll want separate views for Composer -- one which 
obeys the author's prefs, and one which does the usual user settings of custom 
fonts, custom colors, images on, etc.

Comment 38

[Simon Fraser [no longer active]]

I think there will be a problem if these prefs affect composer as well. They 
should not.

Comment 39

[sairuh (rarely reading bugmail)]

hi marc, this might be a very basic question...but re: your current plan: does 
that mean that the settings in the Colors panel (assuming they select the "use 
my chosen colors, ignoring the colors specified" radiobutton) will therefore 
*override* the "use custom colors" settings (if selected) in the Composer > New 
Page Settings panel?

Comment 40

[Marc Attinasi]

My current plan is to insert style rules that will enforce the color choices the
user has made when they select 'always use my colors'. This will, I believe,
also cause Composer and Mail/News to show those colors *always*, overriding
their Use Custom Colors settings (actually, it overrides it visually, in the
display only, not in what is written to the HTML).

I feel that this is probably wrong, and that we need to distinguish between
Composer and non-Composer shells, but that is clearly more work, and I don't
even know how to do it.

The simplest scheme I can think of is for Composer to disable the new
PrefStyleSheet I am creating on the PresShell. Alternatively, if there is way to
know that a PresShell is for the Composer, I can avoid creating the
PrefStyleSheet. I prefer the later, but I don't know how to determine when a
PresShell is for a Composer window. Anybody know? CC'ing Charley since he
probably knows.

Comment 41

[timeless]

As someone trying to separate editor from navigator: Please don't make 
navigator code implementation dependend on editor or knowing that editor could 
exist. Instead allow for end components to suppress the prefShell, or if 
navigator has a layer that editor doesn't use you could have it elect to use 
this prefShell. Actually if the end component could specify where its rules 
come from, so Mail could give one style rule, Navigator a different one and 
Editor an empty one, that might be ideal.

Comment 42

[sairuh (rarely reading bugmail)]

drat, just remembered that charley is now on sabbatical.

kathy, would you be able to help marc on the editor side of things? (or, know
who would?) thx!

Comment 43

[Marc Attinasi]

I spoke with Kathy B. and it seems like the Composer part should be no problem. 
I provided an API on the PresShell to enable/disable the pref style sheet 
(actually, the API will allow individual items in the pref style sheet to be 
enabled and disabled, but for now it is implemented as an all or nothing thing 
because it was lots easier - but in the future we may want the finer grained 
control). I think it will amount to one or two lines of code in the Composer 
(something like presShell->EnablePrefStyleSheet(PR_FALSE,0);)

I'm finishing the testing (no Composer enabling/disabling yet) and will be 
posting the patches very, very soon. The patches, unfortunately, are 19 pages 
when printed out from Windows Notepad - quite large, but the code is, of course, 
*perfect* ;)

Comment 44

[karnaze (gone)]

Removing rtm, since this is covered by bug 40340.

Comment 45

[Marc Attinasi]

Fixed by checkin for bug 40340

Comment 46

[Hixie (not reading bugmail)]

Netscape's standard compliance QA team reorganised itself once again, so taking 
remaining non-tables style bugs. Sorry about the spam. I tried to get this done 
directly at the database level, but apparently that is "not easy because of the 
shadow db", "plus it screws up the audit trail", so no can do...

Comment 47

[Hixie (not reading bugmail)]

reassigning qa to reporter, since it seems pref related :-)

Comment 48

[sairuh (rarely reading bugmail)]

haven't seen this crop up for some time. vrfying...