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

RESOLVED FIXED in Firefox 44

Status

()

defect
RESOLVED FIXED
16 years ago
8 months ago

People

(Reporter: WeblionOST, Assigned: huseby)

Tracking

(Depends on 1 bug, Blocks 1 bug)

Trunk
mozilla44
x86
Windows XP
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

(Whiteboard: [tor])

Attachments

(1 attachment, 9 obsolete attachments)

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.
Assignee: general → nobody
Component: Browser-General → Layout: Form Controls
QA Contact: general → core.layout.form-controls
This is an nsITheme bug.
Assignee: nobody → win32
Component: Layout: Form Controls → GFX: Win32
QA Contact: core.layout.form-controls → ian
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/
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.
Status: UNCONFIRMED → RESOLVED
Closed: 14 years ago
Resolution: --- → EXPIRED
Status: RESOLVED → UNCONFIRMED
Resolution: EXPIRED → ---
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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?
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
David, you made the changes by a patch on bug 67448 a long time ago. Could you help us?
Summary: Mozilla uses system colors for text input fields with "Use System Colors" deselected → System colors for form elements used when browser.display.use_system_colors is set to false
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.
Product: Core → Core Graveyard
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?
Component: GFX: Win32 → GFX: Color Management
Product: Core Graveyard → Core
Whiteboard: [tor]
(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?
Flags: needinfo?(mikeperry)
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?
Attachment #8370465 - Attachment is obsolete: true
Attachment #8597303 - Flags: feedback?(mikeperry)
Assignee: win32 → 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.
Posted patch Bug_232227.patch (obsolete) — Splinter Review
Rebased to current tip.
Attachment #8597303 - Attachment is obsolete: true
Attachment #8597303 - Flags: feedback?(mikeperry)
Attachment #8632002 - Flags: review?(mikeperry)
this patch just cleans up the whitespace in all of the files touched by the real patch.
I just rebased to master.
Attachment #8632002 - Flags: review?(arthuredelstein)
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;
Attachment #8632002 - Flags: review-
Posted patch Bug_232227.patch (obsolete) — Splinter Review
Updated patch with tests!
Attachment #8632002 - Attachment is obsolete: true
Attachment #8632002 - Flags: review?(mikeperry)
Attachment #8632002 - Flags: review?(arthuredelstein)
Flags: needinfo?(mikeperry)
Attachment #8644998 - Flags: review?(mikeperry)
Attachment #8644998 - Flags: review?(brade)
Attachment #8644998 - Flags: review?(arthuredelstein)
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 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
Attachment #8644998 - Flags: review?(arthuredelstein)
Posted patch Bug_232227.patch (obsolete) — Splinter Review
Rebased and review feedback incorporated.  The test code is much cleaner now.  Thanks for the reviews and being patient. :)
Attachment #8632003 - Attachment is obsolete: true
Attachment #8644998 - Attachment is obsolete: true
Attachment #8644998 - Flags: review?(mikeperry)
Attachment #8644998 - Flags: review?(brade)
Attachment #8647774 - Flags: review?(brade)
Attachment #8647774 - Flags: review?(arthuredelstein)
Attachment #8647774 - Flags: review?(brade) → review+
Attachment #8647774 - Flags: review?(arthuredelstein) → review+
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.
Flags: needinfo?(jmuizelaar)
Flags: needinfo?(bgirard)
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.
Attachment #8652521 - Flags: review?(jmuizelaar)
Attachment #8652521 - Flags: review?(bgirard)
not :BenWa's area.
Flags: needinfo?(bgirard)
Attachment #8652521 - Flags: review?(bgirard)
I'll try to get to this patch soon.
Flags: needinfo?(jmuizelaar)
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 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.
Attachment #8652521 - Flags: review?(jmuizelaar)
Thanks for the review, let me incorporate the suggestions and get it re-submitted for review.
Status: NEW → ASSIGNED
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.
Attachment #8647774 - Attachment is obsolete: true
Attachment #8652521 - Attachment is obsolete: true
Attachment #8661513 - Flags: review?(dbaron)
Attachment #8661513 - Flags: review?(dbaron) → review?(jmuizelaar)
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 on attachment 8661513 [details] [diff] [review]
Bug_232227-1.patch  System Stand-in Colors Only

oops, wrong patch.
Attachment #8661513 - Flags: review?(jmuizelaar)
who else do you think should review this?
Flags: needinfo?(jmuizelaar)
I flagged :enn for the widget review and :abillings for the layout review.
Attachment #8661513 - Attachment is obsolete: true
Attachment #8661559 - Flags: review?(jmuizelaar)
Attachment #8661559 - Flags: review?(enndeakin)
Attachment #8661559 - Flags: review?(abillings)
Somehow, I suspect I'm not the developer you are looking for...
Flags: needinfo?(huseby)
Attachment #8661559 - Flags: review?(abillings)
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.
Flags: needinfo?(huseby) → needinfo?(abillings)
Ha! I did not so someone must have made a mistake. I don't even know C++. :-)
Flags: needinfo?(abillings)
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.
(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.
Flags: needinfo?(jmuizelaar) → needinfo?(enndeakin)
Attachment #8661559 - Flags: review?(jmuizelaar) → review+
Flags: needinfo?(enndeakin)
Attachment #8661559 - Flags: review?(enndeakin) → review+
Keywords: checkin-needed
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 ?
Flags: needinfo?(huseby)
Keywords: checkin-needed
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.
Flags: needinfo?(huseby) → needinfo?(cbook)
(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.
(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.
updated the unit test so that it doesn't cause false errors by outputting text after the test finishes.
Attachment #8661559 - Attachment is obsolete: true
Attachment #8665830 - Flags: review+
thanks ! pushed to inbound :)
Flags: needinfo?(cbook)
https://hg.mozilla.org/mozilla-central/rev/deb7126157d7
Status: ASSIGNED → RESOLVED
Closed: 14 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Blocks: 1158076
Depends on: 1235520
Blocks: meta_tor
See Also: → 1485266
See Also: → 1511434
You need to log in before you can comment on or make changes to this bug.