60160 - Preferences Does Not Allow System Colours To Be Used

Status: VERIFIED FIXED

(SeaMonkey :: Preferences, defect, P3)

Description

[Barry Marshall (at work)]

OS/2 1114 build, clean install, TP770x, WSeB, SDD, 1024 * 768 * 24bpp

If you load the above URL under NS/2 4.61, the background is white.
Under OS/2 1114 the background is still grey.

I don't recall this being a problem on Win32 1114, but I don't have my 
machine closeby to check.

I'll attach a picture.

Comment 1

[Barry Marshall (at work)]

Attachment: Mozilla 1114 version partial screen (note background grey)

Comment 2

[Barry Marshall (at work)]

Attachment: NS/2 4.61 version partial screen

Comment 3

[Mike Kaply [:mkaply]]

This is not necessarily a bug.

This page has not speicified a background color, so the default is used.

The default can be a couple things.

1. A color specified in preferences.

2. If the box in preferences "Use Windows colors" or "Use system colors" is 
checked, it uses a color in the OS. This color is different between 4.61 and 
Mozilla. 4.61 actually had a bug related to this.

Please experiment with your scheme palette and see what results you get. I think 
we use window background color.

Thanks

Comment 4

[Barry Marshall (at work)]

Okay, I can confirm #1  (switch background colour yields predicted 
results), but where is #2 to test?

Under NS/2 4.61 I have under Preferences -> Colors an option "Use 
default colors", which I have selected, hence my system white 
background.

Under Warpzilla 1114 I can't find a GUI option to pick system colours.  
There's nothing under "Text and Background" to do this, Color, or any 
other Preferences option I can find.

Comment 5

[Barry Marshall]

Okay, I can confirm that Win32 1114 does indeed have a checkbox under the 
default colour selections to enable default system colour values, but OS/2 1114 
doesn't.

Comment 6

[Barry Marshall (at work)]

Changed description.  Picture of problem to follow.

Comment 7

[Barry Marshall (at work)]

Attachment: As you can see, there's no "Use System Color" option under the colour picker.

Comment 8

[Mike Kaply [:mkaply]]

The code for "Use Windows Colors" should be "Use System Colors" and should be XP.

The preference should be named differently as well, although I 'm not sure how
to do that cleanly and still support the win pref.

I'll attach diffs for this.

Comment 9

[Barry Marshall (at work)]

SPAM  (Fixed my typo in Summary...  I really must learn to type one of 
these days...   :-)  )

Comment 10

[Mike Kaply [:mkaply]]

Attachment: Change to make a system_colors preference NOT windows only

Comment 11

[Mike Kaply [:mkaply]]

OK, I attached a diff for this.

It does the following:

1. Removes Windows specific code in nsPresContext and uses nsLookAndFeel to get 
foreground and background colors. (changed name of pref)

2. Changes some stuff in editor that was using the pref.

3. Changed preferences so the checkbox is in all pref-colors, not just Windows.

Comment 12

[Doron Rosenberg (IBM)]

keywords

Comment 13

[timeless]

indentation for pref-colors.dtd looks a bit off.
pref-colors.xul has a section w/ ~120 columns.
-+        <checkbox id="browserUseSystemColors" value="&useSystemColors.label;" 
accesskey="&useSystemColors.accesskey;"
-+                  pref="true" preftype="bool" 
prefstring="browser.display.use_system_colors" prefattribute="checked"/>
++        <checkbox id="browserUseSystemColors"
++          value="&useSystemColors.label;"
++          accesskey="&useSystemColors.accesskey;"
++          pref="true" preftype="bool" prefattribute="checked"
++          prefstring="browser.display.use_system_colors"
++        />
[or some other syntax]
otherwise you could take an r=timeless if you want it.

Comment 14

[Mike Kaply [:mkaply]]

matt:

Any chance I could get an r= on the C code?

Does anyone else think this is the right thing to do? :)

Comment 15

[Barry Marshall (at work)]

Hi Matt.

Were you able to review Mike's patches?

Comment 16

[Blake Ross]

Looks fine to me.  I assume you're sure nsPresContext.cpp no longer needs 
windows.h?  Also, please leave autostretch="never" on the <box/> in pref-
colors.xul.  r=blake, cc'ing alec for sr

Comment 17

[Alec Flett]

coolness!
sr=alecf as long as the windows.h issue has been sorted out :)

Comment 18

[Mike Kaply [:mkaply]]

windows.h is definitely only included in nsPresContext.cpp for this color stuff. 
It builds fine without it.

Hurrah. Removing platform specific code from XP :)

Comment 19

[Matthew T (active 1999-2002)]

Sorry to be annoying, but take a look at
<http://www.chasms3.com/mac022600/apple7c3.htm>. Is the Mac implementation 
getting the system colors from the Internet control panel? Judging by the call to 
nsILookAndFeel::whatever, it's not, it's just getting it from the Appearance 
Manager theme. That's not right.

Until that's fixed, the GUI for this shouldn't be present on Mac OS, as it won't 
be doing what the user expects.

Comment 20

[Mike Kaply [:mkaply]]

I have to humbly disagree.

We can certainly override that pref in mac.js and make it false so that the Mac 
users see no difference in current behavior. But Mac not using Internet Control 
Panel for nsLookAndFeel is a different issue.

I searched various documentation on Internet Config at:

http://www.quinn.echidna.id.au/Quinn/Config/

and found no information on querying the color preferences, or I would implement 
it. Note that there is an InternetConfig API interface already in Mozilla, so 
someone just needs the ability to query those color prefs to it.

Comment 21

[Blake Ross]

cc'ing pchen, the internet config god

Comment 22

[Alec Flett]

I not-so-humbly disagree.  (and agree with mkaply) - the fact that the system
colors are not retrieved from internet config is a seperate issue. 

if anything, we should be updating nsILookAndFeel to support browser-specific
colors, then use that. On platforms which do not have this system color, we
should use the default forground/background color, but reflect that in the
implementation of nsILookAndFeel for that particular platform.

i.e. we need to add new enums to:
http://lxr.mozilla.org/seamonkey/source/widget/public/nsILookAndFeel.h#43

Comment 23

[Matthew T (active 1999-2002)]

Yes indeed. I'm not saying that the Mac should *never* have this checkbox, just 
that it shouldn't have it *until* the color settings are read from Internet 
Config (rather than from the Appearance Manager). Otherwise we're basically 
lying.

Comment 24

[Alec Flett]

whatever.
mkaply, you have my sr=
if mkaply checks it in, mpt feel free to file a new bug.

Comment 25

[Blake Ross]

Just to jump in and shamelessly repeat what others have said, properly 
retrieving the colors from the right place should indeed be the responsibility 
of nsILookAndFeel; mkaply shouldn't need to do anything except request the 
right metric.  File a new bug on that.

Comment 26

[Mike Kaply [:mkaply]]

Bug:

http://bugzilla.mozilla.org/show_bug.cgi?id=5721

Has been open for an eternity for the internet config. Need to add 
nsLookAndFeel/foreground/background/link color to that bug.

Marking this bug fixed.

Comment 27

[Barry Marshall]

Will verify on next OS/2 build.  
The current 1/30 build is missing the fixes from 67061.

Comment 28

[sairuh (rarely reading bugmail)]

vrfy fixed on linux and winnt using 2001.02.09.0x comm bits. [mac stuff covered
by bug 5721.]

barry, how does this look for you on os/2? [unless you're still waiting on a
build...]

Comment 29

[Barry Marshall (at work)]

* SPAM *
Finally got my hands on a new OS/2 build and everything's peachy...  well,
except for the stock now.  :-)