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.

This is an nsITheme bug.

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.

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.

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?

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.

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?

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

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?

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

Rebased to current tip.

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

I just rebased to master.

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;

Updated patch with tests!

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

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

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.

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.

not :BenWa's area.

I'll try to get to this patch soon.

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.

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

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

who else do you think should review this?

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

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

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.

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

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.

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

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 ?

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.

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

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

thanks ! pushed to inbound :)