232227 - System colors for form elements used when browser.display.use_system_colors is set to false

Status: RESOLVED FIXED

(Core :: Graphics: Color Management, defect)

Description

[Chris]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

This is an nsITheme bug.

Comment 2

[Gervase Markham [:gerv]]

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/

Comment 3

[Gervase Markham [:gerv]]

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.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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?

Comment 6

[Carsten Book [:Tomcat]]

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

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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

Comment 8

[Bruno 'Aqualon' Escherl]

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.

Comment 9

[Mike Perry]

Attachment: Patch to avoid use of OS theme colors (against FF24; no pref)

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?

Comment 10

[Sid Stamm [:geekboy or :sstamm]]

(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?

Comment 11

[Sid Stamm [:geekboy or :sstamm]]

Attachment: Patch to avoid use of OS theme colors (against mozilla-central; first whack at pref)

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?

Comment 12

[Dave Huseby [:huseby]]

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

Comment 13

[Dave Huseby [:huseby]]

Attachment: Bug_232227.patch

Rebased to current tip.

Comment 14

[Dave Huseby [:huseby]]

Attachment: cleans up whitespace in files touched by patch.

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

Comment 15

[Dave Huseby [:huseby]]

I just rebased to master.

Comment 16

[Kathleen :Brade]

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;

Comment 17

[Dave Huseby [:huseby]]

Attachment: Bug_232227.patch

Updated patch with tests!

Comment 18

[Kathleen :Brade]

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();

Comment 19

[Arthur Edelstein [:arthur]]

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

Comment 20

[Dave Huseby [:huseby]]

Attachment: Bug_232227.patch

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

Comment 21

[Dave Huseby [:huseby]]

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.

Comment 22

[Dave Huseby [:huseby]]

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

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.

Comment 23

[Dave Huseby [:huseby]]

not :BenWa's area.

Comment 24

[Jeff Muizelaar [:jrmuizel]]

I'll try to get to this patch soon.

Comment 25

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

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?

Comment 26

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 27

[Dave Huseby [:huseby]]

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

Comment 28

[Dave Huseby [:huseby]]

Attachment: Bug_232227-1.patch System Stand-in Colors Only

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.

Comment 29

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 30

[Dave Huseby [:huseby]]

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

oops, wrong patch.

Comment 31

[Dave Huseby [:huseby]]

who else do you think should review this?

Comment 32

[Dave Huseby [:huseby]]

Attachment: Bug_232227-1.patch System Stand-in Colors Support

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

Comment 33

[Al Billings [:abillings - ex-MoCo]]

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

Comment 34

[Dave Huseby [:huseby]]

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.

Comment 35

[Al Billings [:abillings - ex-MoCo]]

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

Comment 36

[Neil Deakin]

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.

Comment 37

[Dave Huseby [:huseby]]

(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.

Comment 38

[Dave Huseby [:huseby]]

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

Comment 39

[Carsten Book [:Tomcat]]

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 ?

Comment 40

[Dave Huseby [:huseby]]

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.

Comment 41

[Arthur Edelstein [:arthur]]

(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.

Comment 42

[Dave Huseby [:huseby]]

(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.

Comment 43

[Dave Huseby [:huseby]]

Attachment: Bug_232227-1.patch

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

Comment 44

[Dave Huseby [:huseby]]

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

Comment 45

[Carsten Book [:Tomcat]]

thanks ! pushed to inbound :)

Comment 46

[Pulsebot]

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

Comment 47

[Carsten Book [:Tomcat]]

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