Chris Reporter
	Description • 21 years ago

User-Agent:       
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6) Gecko/20040113

Any page with an input box uses the system colors, even if the selection box for
using system colors is deselected.  While this may not be important, I use green
on black for my default colors, causing some sites (Probably with text color set
to black for the boxes) to show black on black, which can get rather annoying.

Reproducible: Always
Steps to Reproduce:
1. Set your system colors to something other than the default ones.
2. Make sure "Use System Colors" is off
3. Visit a page with a text box.  A bugzilla submission page is a good place to
start.
4. Notice how the box is using system colors.

Actual Results:  
It used the system colors.

Expected Results:  
Use the preset colors.

	Boris Zbarsky [:bzbarsky]
	Comment 1 • 21 years ago

This is an nsITheme bug.

	Gervase Markham [:gerv]
	Comment 2 • 19 years ago

This is an automated message, with ID "auto-resolve01".

This bug has had no comments for a long time. Statistically, we have found that
bug reports that have not been confirmed by a second user after three months are
highly unlikely to be the source of a fix to the code.

While your input is very important to us, our resources are limited and so we
are asking for your help in focussing our efforts. If you can still reproduce
this problem in the latest version of the product (see below for how to obtain a
copy) or, for feature requests, if it's not present in the latest version and
you still believe we should implement it, please visit the URL of this bug
(given at the top of this mail) and add a comment to that effect, giving more
reproduction information if you have it.

If it is not a problem any longer, you need take no action. If this bug is not
changed in any way in the next two weeks, it will be automatically resolved.
Thank you for your help in this matter.

The latest beta releases can be obtained from:
Firefox:     http://www.mozilla.org/projects/firefox/
Thunderbird: http://www.mozilla.org/products/thunderbird/releases/1.5beta1.html
Seamonkey:   http://www.mozilla.org/projects/seamonkey/

	Gervase Markham [:gerv]
	Comment 3 • 19 years ago

This bug has been automatically resolved after a period of inactivity (see above
comment). If anyone thinks this is incorrect, they should feel free to reopen it.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 4 • 18 years ago

Confirmed with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.2pre) Gecko/20070104 BonEcho/2.0.0.2pre ID:2007010403

Changing the system colors for text and window background also changes the colors in Firefox while the checkbox "Use system colors" is unchecked.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 5 • 18 years ago

For any form element the default values of their css attributes color and background-color are set to the system colors.

color: -moz-FieldText;
background-color: -moz-Field;

That causes Firefox to render the elements with the system colors although if the checkbox isn't checked. Changing both values to 'inherit' in http://lxr.mozilla.org/seamonkey/source/layout/style/forms.css solves it for me.

Is that the right place for changes? If yes, this wouldn't only happen for Windows. Could anyone test this with another OS?

	Carsten Book [:Tomcat]
	Comment 6 • 18 years ago

WFM on Linux with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.2pre) Gecko/20070104 BonEcho/2.0.0.2pre and Linux Fedora FC 6

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 7 • 18 years ago

David, you made the changes by a patch on bug 67448 a long time ago. Could you help us?

	Bruno 'Aqualon' Escherl
	Comment 8 • 18 years ago

To see this issue in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.2pre) Gecko/20061223 SeaMonkey/1.1 the pref browser.display.use_document_colors has to be set to true.

	Mike Perry
	Comment 9 • 10 years ago

For Tor Browser, we wrote a patch to handle the various CSS colors that come from the OS theme, and to use a fixed color map instead. We did this for fingerprinting reasons, but it appears there are people out there who generally don't want to use their OS colors in webpages for other reasons, too?

This patch is against FF24ESR.

We did not bind this patch to this pref. Should we?

	Sid Stamm [:geekboy or :sstamm]
	Comment 10 • 9 years ago

(In reply to Mike Perry from comment #9)
> We did not bind this patch to this pref. Should we?

I think it would be best if it was bound to a pref.  Mike: would this be something you or your team can do?  By any chance do you have a fresh version of this patch laying around that might work in ESR31 or mozilla-central?

	Sid Stamm [:geekboy or :sstamm]
	Comment 11 • 9 years ago

Mike: I rebased the patch on mozilla-central and tried to wedge in a pref.  Can you take a look and let me know if this is a reasonable rebasing and attempt at adding a pref?

	Dave Huseby [:huseby] Assignee
	Comment 12 • 9 years ago

I'm picking this up and rebasing it to the current mozilla-central.  Will follow up with the TBB devs for feedback when done.

	Dave Huseby [:huseby] Assignee
	Comment 13 • 9 years ago

Rebased to current tip.

	Dave Huseby [:huseby] Assignee
	Comment 14 • 9 years ago

this patch just cleans up the whitespace in all of the files touched by the real patch.

	Dave Huseby [:huseby] Assignee
	Comment 15 • 9 years ago

I just rebased to master.

	Kathleen :Brade
	Comment 16 • 9 years ago

Comment on attachment 8632002 [details] [diff] [review]
Bug_232227.patch

Review of attachment 8632002 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think that this part is correct:
-  if (IS_COLOR_CACHED(aID)) {
+  if (aUseStandinsForNativeColors &&
+      ColorIsNotCSSAccessible(aID) &&
+      !sUseStandinsForNativeColors) {
+    aUseStandinsForNativeColors = false;
+  }

This can be fixed in multiple ways...  here is one:
-  if (IS_COLOR_CACHED(aID)) {
+  if (aUseStandinsForNativeColors &&
+      (ColorIsNotCSSAccessible(aID) || !sUseStandinsForNativeColors)) {
+    aUseStandinsForNativeColors = false;
+  }

Another approach would be to add a line like this instead of your change above:
  aUseStandinsForNativeColors = aUseStandinsForNativeColors && sUseStandinsForNativeColors;

	Dave Huseby [:huseby] Assignee
	Comment 17 • 9 years ago

Updated patch with tests!

	Kathleen :Brade
	Comment 18 • 9 years ago

Comment on attachment 8644998 [details] [diff] [review]
Bug_232227.patch

The patch looks good.  However, can you please clarify some things about the tests?

 * The tests are only executed conditionally (due to these two lines):
       if (c1 != "rgb(255, 0, 254)")
       if (c2 != c1 && c2 != "rgb(0, 0, 0)")
   In what case do we expect to get the default background color for the div 
   or a black pixel for the canvas?

 * Why does the test contain code for testing on IE?
     var range = document.body.createTextRange();

	Arthur Edelstein [:arthur]
	Comment 19 • 9 years ago

Comment on attachment 8644998 [details] [diff] [review]
Bug_232227.patch

Review of attachment 8644998 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +685,2 @@
>    nscolor color;
> +  if (!nsRuleNode::ComputeColor(value, presShell ? presShell->GetPresContext() : nullptr, 

Nit: End of line has blank space.

::: dom/canvas/test/test_bug232227.html
@@ +97,5 @@
> +	var colorTestDiv = document.createElement("div");
> +	document.body.appendChild(colorTestDiv); // ie
> +
> +	for (var i in colorNames) {
> +		(function(colorName, r, g, b) {

You could use array destructuring here instead of an anonymous function. (Though the script may need to be declared as JS 1.7).

Something like:

`for (let [colorName, r, g, b] of colorNames) {`

@@ +100,5 @@
> +	for (var i in colorNames) {
> +		(function(colorName, r, g, b) {
> +      // test value
> +      var ctest = "rgb(" + r + ", " + g + ", " + b + ")";
> +

Some of the indentation is kind of confusing. Maybe re-indent the file?

@@ +139,5 @@
> +var prefs = [
> +  [ "ui.use_standins_for_native_colors", true ],
> +  [ "ui.use_native_colors", true ],
> +];
> +SpecialPowers.pushPrefEnv({ "set" : prefs }, beginTest);

I'm pretty sure beginTest() runs asynchronously after the prefs are successfully set. At the beginning of the script, you need to call `SimpleTest.waitForExplicitFinish();` ? Then you can call `SimpleTest.finish();` at the end of `beginTest()`.

@@ +142,5 @@
> +];
> +SpecialPowers.pushPrefEnv({ "set" : prefs }, beginTest);
> +
> +ok(true, "Eliminates false negative.");
> +

I think you shouldn't need this line. Maybe you added it because the test was ending before beginTest() could run?

::: widget/nsXPLookAndFeel.cpp
@@ +937,5 @@
> +
> +  *aResult = NS_RGB(0, 0, 0);
> +  return nsLookAndFeel::GetInstance()->NativeGetColor(colorID, *aResult);
> +}
> + 

Nit: blank space

	Dave Huseby [:huseby] Assignee
	Comment 20 • 9 years ago

Rebased and review feedback incorporated.  The test code is much cleaner now.  Thanks for the reviews and being patient. :)

	Dave Huseby [:huseby] Assignee
	Comment 21 • 9 years ago

With regards to review board review https://reviewboard.mozilla.org/r/17165/
Please note that this patch has already been reviewed and r+ by the Tor reviewers.

	Dave Huseby [:huseby] Assignee
	Comment 22 • 9 years ago

Bug 232227 - Do not expose system colors to CSS or canvas; r?jrmuizel, BenWa

This patch also contains a hack to use properly contrasting colors if the
desktop theme specifies white on black for text colors (see
https://trac.torproject.org/projects/tor/ticket/7920). These color choices are
also not exposed to content.

	Dave Huseby [:huseby] Assignee
	Comment 23 • 9 years ago

not :BenWa's area.

	Jeff Muizelaar [:jrmuizel]
	Comment 24 • 9 years ago

I'll try to get to this patch soon.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 25 • 9 years ago

For the canvas gradient stuff:  Getting the CSSLoader from the document in order to pass it around in order to get the document back from the CSSLoader seems pretty odd.  Why not just pass the document around, which would be much more normal in this context.  And is there any justification for the "not ref counted, it owns us" comment?  How does it "own us"?  For that matter, why is it needed at all?  Why not just call GetPresShell() on the canvas rendering context and get the document from that?  (Or if that's sometimes null, can you fix it being sometimes null instead of adding a new thing?)

The changes to nsLayoutUtils::GetColor and GetNativeTextColor and their callers look interesting, but they don't seem directly related to the subject of this bug (although they're somewhat tangentially related).  But they're making significant Web-exposed functionality changes unconditionally (not behind a pref), which doesn't seem like what this bug is about.  Shouldn't they be evaluated in their own bug, along with an explanation for the motivation behind those changes?

	Jeff Muizelaar [:jrmuizel]
	Comment 26 • 9 years ago

Comment on attachment 8652521 [details]
MozReview Request: Bug 232227 - Do not expose system colors to CSS or canvas; r?jrmuizel, BenWa

I agree with David's comments.

	Dave Huseby [:huseby] Assignee
	Comment 27 • 9 years ago

Thanks for the review, let me incorporate the suggestions and get it re-submitted for review.

	Dave Huseby [:huseby] Assignee
	Comment 28 • 9 years ago

This is the "use stand-in system colors patch" with review feedback incorporated.  The contrasting colors will come in a second patch on this bug.

	Jeff Muizelaar [:jrmuizel]
	Comment 29 • 9 years ago

Comment on attachment 8661513 [details] [diff] [review]
Bug_232227-1.patch  System Stand-in Colors Only

Review of attachment 8661513 [details] [diff] [review]:
-----------------------------------------------------------------

The other canvas part seems ok. You should perhaps find additional reviewers for the rest of layout and widget code.

::: dom/canvas/CanvasGradient.h
@@ +70,5 @@
>  
>    nsRefPtr<CanvasRenderingContext2D> mContext;
>    nsTArray<mozilla::gfx::GradientStop> mRawStops;
>    mozilla::RefPtr<mozilla::gfx::GradientStops> mStops;
> +  mozilla::css::Loader* mCSSLoader; // not ref counted, it owns us

This does not seem to be used.

	Dave Huseby [:huseby] Assignee
	Comment 30 • 9 years ago

Comment on attachment 8661513 [details] [diff] [review]
Bug_232227-1.patch  System Stand-in Colors Only

oops, wrong patch.

	Dave Huseby [:huseby] Assignee
	Comment 31 • 9 years ago

who else do you think should review this?

	Dave Huseby [:huseby] Assignee
	Comment 32 • 9 years ago

I flagged :enn for the widget review and :abillings for the layout review.

	Al Billings [:abillings - ex-MoCo]
	Comment 33 • 9 years ago

Somehow, I suspect I'm not the developer you are looking for...

	Dave Huseby [:huseby] Assignee
	Comment 34 • 9 years ago

You had reviewed something in nsRuleNode.cpp recently in that files history.  Per :jmuizelaar's suggestion, I flagged you to review the tiny change to that file.

	Al Billings [:abillings - ex-MoCo]
	Comment 35 • 9 years ago

Ha! I did not so someone must have made a mistake. I don't even know C++. :-)

	Neil Deakin
	Comment 36 • 9 years ago

Comment on attachment 8661559 [details] [diff] [review]
Bug_232227-1.patch System Stand-in Colors Support

>+  // The stand-in colors are taken from the Windows 7 Aero theme
>+  // except Mac-specific colors which are taken from Mac OS 10.7.

Don't we want to pick colours specific to all platforms. Having Windows colours on non-Windows doesn't seem correct to me.

Other than that the lookandfeel changes are ok.

	Dave Huseby [:huseby] Assignee
	Comment 37 • 9 years ago

(In reply to Neil Deakin from comment #36)
> Don't we want to pick colours specific to all platforms. Having Windows
> colours on non-Windows doesn't seem correct to me.

This is an anti-fingerprinting patch.  The idea is that when the pref is turned on, the browser will use the same colors, regardless of the platform so as to not give any data that is unique to the platform.

I'm clearning the ni? for :jmuizelaar, I just need an r+ from him and you Neil.

	Dave Huseby [:huseby] Assignee
	Comment 38 • 9 years ago

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=450988937555

	Carsten Book [:Tomcat]
	Comment 39 • 9 years ago

Hey Dave, in the try run are a lot of failures like https://treeherder.mozilla.org/logviewer.html#?job_id=11854113&repo=try which seems to be related to this bug. 

Is this intended or do we need to fix this before pushing ?

	Dave Huseby [:huseby] Assignee
	Comment 40 • 9 years ago

Hrm, those are output from using the is() call in the unit test.  I didn't think using is() would be a problem.  Let me find an alternative and update the patch.

	Arthur Edelstein [:arthur]
	Comment 41 • 9 years ago

(In reply to Dave Huseby [:huseby] from comment #40)
> Hrm, those are output from using the is() call in the unit test.  I didn't
> think using is() would be a problem.  Let me find an alternative and update
> the patch.

Hi Dave -- I had a look at attachment 8661559 [details] [diff] [review] and I noticed that a lot of improvements you made in attachment 8647774 [details] [diff] [review] are gone. I wonder if this also explains the try server failures.

	Dave Huseby [:huseby] Assignee
	Comment 42 • 9 years ago

(In reply to Arthur Edelstein from comment #41)
> Hi Dave -- I had a look at attachment 8661559 [details] [diff] [review] and
> I noticed that a lot of improvements you made in attachment 8647774 [details] [diff] [review]
> [details] [diff] [review] are gone. I wonder if this also explains the try
> server failures.

I removed the black on black contrast fix because it needs to be its own patch.  I'm going to submit those changes after the first patch lands.  I think the failures are due to "Result logged after SimpleTest.finish()"  I added calls to SimpleTest.waitForExplicitFinish() and SimpleTest.finish() so that the test function runs before the test finishes.  I think this will fix it.

	Dave Huseby [:huseby] Assignee
	Comment 43 • 9 years ago

updated the unit test so that it doesn't cause false errors by outputting text after the test finishes.

	Dave Huseby [:huseby] Assignee
	Comment 44 • 9 years ago

new try with updated test: https://treeherder.mozilla.org/#/jobs?repo=try&revision=69db331929b5

	Carsten Book [:Tomcat]
	Comment 45 • 9 years ago

thanks ! pushed to inbound :)

	Pulsebot
	Comment 46 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/deb7126157d7

	Carsten Book [:Tomcat]
	Comment 47 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/deb7126157d7