1330876 - use properly contrasting colors if the desktop theme specifies white on black for text colors [tor 6786]

Status: RESOLVED FIXED

(Core :: Graphics: Color Management, defect, P1)

Description

[Arthur Edelstein [:arthur]]

Bug 232227 introduced a patch to prevent system colors from being exposed to CSS or canvas. But that mode doesn't operate quite correctly when the desktop theme calls for white on black text. The current Tor Browser patch is at https://torpat.ch/6786

We propose uplifting this patch.

Comment 1

[Jonathan Hao (inactive) [:jhao]]

Hello Arthur,

I know this patch fixes a problem for Tor Browser.
But do you know why the lack of this patch doesn't affect Firefox?

Thanks.

Comment 2

[Arthur Edelstein [:arthur]]

> Hello Arthur,
> 
> I know this patch fixes a problem for Tor Browser.
> But do you know why the lack of this patch doesn't affect Firefox?
> 
> Thanks.

Hi Jonathan -- my understanding is that Firefox suffers from this problem as well, but the problem is observable only when "ui.use_standins_for_native_colors" is set to true. So this is effectively a fixup to bug 232227.

Comment 3

[Chung-Sheng Fu [:cfu]]

Hi Arthur,

I tried to apply the patch but it does not work.  So I would like to ask how the patch works in the Tor Browser.

1. I traced our code base and the entry point causing this bug is LookAndFeel::GetColor, while the patch mainly modifies nsLayoutUtils::GetColor.
Since the routine affected by the preference "ui.use_standins_for_native_colors" does not include nsLayoutUtils::GetColor, how can the patch fix the bug?

2. The patch finally calls nsXPLookAndFeel::NativeGetColor, which is just the same as what LookAndFeel::GetColor calls with the preference "ui.use_standins_for_native_colors" disabled.
So how can this patch get native text colors without exposing them if Firefox is doing the same thing but suffers from the fingerprinting issue?

Thanks.

Comment 4

[Arthur Edelstein [:arthur]]

Hi Mark and Kathy -- could you advise on Chung-Sheng's questions in comment 3? Thanks in advance.

In case it's useful, here's the original ticket where the patch was written:
https://trac.torproject.org/projects/tor/ticket/7920

Comment 5

[Mark Smith [:mcs]]

One way to test this is to set your theme to "High Contrast Black" on Windows and ensure that ui.use_standins_for_native_colors = true. Then load a page that contains an unstyled input field and try to type in the field.

What I know so far is that with Tor Browser 6.5.2 (based on ESR 45) a contrasting color is correctly used for the caret and text, but with Firefox 53.0.3 and Tor Browser 7.0a4 (based on ESR 52) black-on-black text us used. More research is needed, but it seems like the patch needs to be revised to be compatible with current Firefox code.

Comment 6

[Chung-Sheng Fu [:cfu]]

Hi Mark,

In current Firefox code base, when ui.use_standins_for_native_colors=false, nsXPLookAndFeel::NativeGetColor will be called, as the patch https://torpat.ch/6786 does, to get colors from theme settings, so text and caret can be rendered with correct colors.

On the other hand, when ui.use_standins_for_native_colors=true, another function will be called, which returns fixed values, preventing the real theme colors from being used for fingerprinting, and results in the improperly contrasting text and caret colors (because the function always returns black for field text).

Therefore, I would like to clarify what the purpose of the patch https://torpat.ch/6786 is.
It seems to me that it solves the color issue by getting real theme colors, instead of hiding them from being exposed to getComputedStyle.

So maybe the problem in Firefox should be that when ui.use_standins_for_native_colors=true, input field text colors are got from fixed values, but background colors are still the real theme colors.

Comment 7

[Mark Smith [:mcs]]

(In reply to Chung-Sheng Fu [:cfu] from comment #6)
> Therefore, I would like to clarify what the purpose of the patch
> https://torpat.ch/6786 is.
> It seems to me that it solves the color issue by getting real theme colors,
> instead of hiding them from being exposed to getComputedStyle.

Yes, I think so. It has been a long time since we wrote the original patch, but we thought that the original https://torpat.ch/6786 patch would not expose the colors via getComputedStyle. I am not 100% sure if that is correct or not.

> So maybe the problem in Firefox should be that when
> ui.use_standins_for_native_colors=true, input field text colors are got from
> fixed values, but background colors are still the real theme colors.

Yes. In some cases a native background is used for text fields, etc. The https://torpat.ch/6786 tries to detect that and use the correct native (contrasting) color for caret and text. I am not sure how it would affect people who rely on high contrast themes, but I think another way to fix the problem would be to ensure that the fixed colors are used for the background.

Comment 8

[Chung-Sheng Fu [:cfu]]

(In reply to Mark Smith (:mcs) from comment #7)
> (In reply to Chung-Sheng Fu [:cfu] from comment #6)
> > Therefore, I would like to clarify what the purpose of the patch
> > https://torpat.ch/6786 is.
> > It seems to me that it solves the color issue by getting real theme colors,
> > instead of hiding them from being exposed to getComputedStyle.
> 
> Yes, I think so. It has been a long time since we wrote the original patch,
> but we thought that the original https://torpat.ch/6786 patch would not
> expose the colors via getComputedStyle. I am not 100% sure if that is
> correct or not.

May I ask why it would not expose colors via getComputedStyle?
According to my test on Firefox, Chrome, and Edge, getComputedStyle can always get rendered colors such as .color and .backgroundColor.
So I could not understand how the patch protects the colors got from nsXPLookAndFeel::NativeGetColor.

> > So maybe the problem in Firefox should be that when
> > ui.use_standins_for_native_colors=true, input field text colors are got from
> > fixed values, but background colors are still the real theme colors.
> 
> Yes. In some cases a native background is used for text fields, etc. The
> https://torpat.ch/6786 tries to detect that and use the correct native
> (contrasting) color for caret and text. I am not sure how it would affect
> people who rely on high contrast themes, but I think another way to fix the
> problem would be to ensure that the fixed colors are used for the background.

Thanks!
Maybe we can also ask the author of the ui.use_standins_native_colors patch what is the expected result.

Comment 9

[Chung-Sheng Fu [:cfu]]

Attachment: Test with Tor Browser

I tested with the latest stable Tor Browser (6.5.2).
When ui.use_standins_for_native_colors=true, text and caret in input fields are not properly contrasted.
And I checked the code on branch tor-browser-45.9.0esr-6.5-1 (because it says Tor Browser 6.5.2 is based on Firefox 45.9.0).
The patch introduced in Bug 232227 and the patch https://torpat.ch/6786 are both applied.
That is, it may indicate that Tor Browser is suffering the same problem as Firefox.

Comment 10

[Chung-Sheng Fu [:cfu]]

Attachment: Bug 1330876 - Listen to change event of preference "ui.use_standins_for_native_colors" and update default foreground and background colors

Review commit: https://reviewboard.mozilla.org/r/145174/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/145174/

Comment 11

[Chung-Sheng Fu [:cfu]]

Attachment: 0001-Listen-to-preference-change-event-and-update-values-.patch

When ui.use_standins_for_native_colors=true, colors from system theme or preference should not be used to render elements.
This patch listens to the change event of ui.use_standins_for_native_colors and updates the values of mDefaultColor and mBackgroundColor.
Is it a reasonable solution?

Besides, I think it is better to AddWeakObserver and strong observer seems to produce some leak issues.
But I am not sure if nsPresContext should inherit nsSupportsWeakReference.

Comment 12

[Cameron McCormack (:heycam)]

Comment on attachment 8874782 [details] [diff] [review]
0001-Listen-to-preference-change-event-and-update-values-.patch

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

::: layout/base/nsPresContext.cpp
@@ +488,5 @@
> +    //
> +    // TODO
> +    // Maybe AddWeakObserver is better, can nsPresContext inherit
> +    // nsSupportsWeakReference?
> +    Preferences::AddStrongObserver(this, kUseStandinsForNativeColors);

Instead of this observer, let's register a callback in nsPresContext::Init() with a Preferences::RegisterCallback call.  (And add a corresponding UnregisterCallback call in nsPresContext::Destroy().)

@@ +527,5 @@
> +    if (NS_FAILED(LookAndFeel::GetColor(LookAndFeel::eColorID_window, true, &mBackgroundColor))) {
> +      mBackgroundColor = NS_RGB(0xff, 0xff, 0xff);
> +    }
> +  }
> +  else if (usePrefColors) {

Nit: "else if" on the previous line.

@@ +1129,5 @@
> +
> +    // "ui.use_standins_for_native_colors" changed.
> +    if (pref.EqualsLiteral(kUseStandinsForNativeColors)) {
> +      // Update default foreground and background colors.
> +      GetDocumentColorPreferences();

If we use the Preferences::RegisterCallback method as above, we shouldn't need this, since nsPresContext::PreferenceChanged will end up calling GetDocumentColorPreferences() (under UpdateAfterPreferencesChanged).

Comment 13

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8873764 [details]
Bug 1330876 - Listen to change event of preference "ui.use_standins_for_native_colors" and update default foreground and background colors

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145174/diff/1-2/

Comment 14

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8873764 [details]
Bug 1330876 - Listen to change event of preference "ui.use_standins_for_native_colors" and update default foreground and background colors

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145174/diff/2-3/

Comment 15

[Cameron McCormack (:heycam)]

Comment on attachment 8873764 [details]
Bug 1330876 - Listen to change event of preference "ui.use_standins_for_native_colors" and update default foreground and background colors

https://reviewboard.mozilla.org/r/145174/#review151230

::: dom/canvas/test/test_bug232227.html:82
(Diff revision 3)
> +		[ "-moz-html-CellHighlight", 0x33, 0x99, 0xFF ],
> +		[ "-moz-html-CellHighlightText", 0xFF, 0xFF, 0xFF ],

Did these lines need to move?

::: layout/base/nsPresContext.cpp:519
(Diff revision 3)
> +    if (NS_FAILED(LookAndFeel::GetColor(
> +        LookAndFeel::eColorID_windowtext, true, &mDefaultColor))) {
> +      mDefaultColor = NS_RGB(0x00, 0x00, 0x00);
> +    }

This is fine.  Though maybe having a version of LookAndFeel::GetColor() that can take the aUseStandinsForNativeColors argument and the default color argument would be better, as the structure would match the rest of the calls around here.

And a nit: I find it's helpful to readers to add a comment for non-obvious boolean arguments, e.g. something like:

  if (NS_FAILED(LookAndFeel::GetColor(LookAndFeel::eColorID_windowtext,
                                      /* aUseStandinsForNativeColors */ true,
                                      &mDefaultColor))) {
    ...

Comment 16

[Chung-Sheng Fu [:cfu]]

(In reply to Cameron McCormack (:heycam) from comment #15)
> Comment on attachment 8873764 [details]
> Bug 1330876 - Listen to change event of preference
> "ui.use_standins_for_native_colors" and update default foreground and
> background colors
> 
> https://reviewboard.mozilla.org/r/145174/#review151230
> 
> ::: dom/canvas/test/test_bug232227.html:82
> (Diff revision 3)
> > +		[ "-moz-html-CellHighlight", 0x33, 0x99, 0xFF ],
> > +		[ "-moz-html-CellHighlightText", 0xFF, 0xFF, 0xFF ],
> 
> Did these lines need to move?

The array colorNames originally ends with the entry [ "-moz-win-CommunicationsText", 0xFF, 0xFF, 0xFF ].
That is, the last iteration of this test sets div.style.backgroundColor and canvas.fillStyle to -moz-win-CommunicationsText.
When the test finishes, the preference ui.use_standins_for_native_colors (and also ui.use_native_colors) will be cleared due to pushPrefEnv.
It will then trigger restyling and try to get real color value of -moz-win-CommunicationsText since at this moment, background color of style of test div and fill style of test canvas are both -moz-win-CommunicationsText.
However, this value is only available in Windows and assertion will be generated in other platforms, leading to test failure (reference: https://treeherder.mozilla.org/logviewer.html#?job_id=105149391&repo=try&lineNumber=25256 ).
Therefore I move common color names to the end of the array, in order to avoid test failure.

> 
> ::: layout/base/nsPresContext.cpp:519
> (Diff revision 3)
> > +    if (NS_FAILED(LookAndFeel::GetColor(
> > +        LookAndFeel::eColorID_windowtext, true, &mDefaultColor))) {
> > +      mDefaultColor = NS_RGB(0x00, 0x00, 0x00);
> > +    }
> 
> This is fine.  Though maybe having a version of LookAndFeel::GetColor() that
> can take the aUseStandinsForNativeColors argument and the default color
> argument would be better, as the structure would match the rest of the calls
> around here.
> 
> And a nit: I find it's helpful to readers to add a comment for non-obvious
> boolean arguments, e.g. something like:
> 
>   if (NS_FAILED(LookAndFeel::GetColor(LookAndFeel::eColorID_windowtext,
>                                       /* aUseStandinsForNativeColors */ true,
>                                       &mDefaultColor))) {
>     ...

I will fix them.
Besides, if I modify the interface of LookAndFeel::GetColor, should I ask others to review?

Thank you very much for your help!

Comment 17

[Cameron McCormack (:heycam)]

(In reply to Chung-Sheng Fu [:cfu] from comment #16)
> Therefore I move common color names to the end of the array, in order to
> avoid test failure.

OK.

> Besides, if I modify the interface of LookAndFeel::GetColor, should I ask
> others to review?

I'm happy to review changes to LookAndFeel.

Comment 18

[Chung-Sheng Fu [:cfu]]

Attachment: Bug 1330876 - Add a new variant of LookAndFeel::GetColor that uses stand-ins for native colors and accepts default value.

Review commit: https://reviewboard.mozilla.org/r/147912/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/147912/

Comment 19

[Cameron McCormack (:heycam)]

Comment on attachment 8876579 [details]
Bug 1330876 - Add a new variant of LookAndFeel::GetColor that uses stand-ins for native colors and accepts default value.

https://reviewboard.mozilla.org/r/147912/#review152244

Looks good!

::: widget/LookAndFeel.h:538
(Diff revision 1)
> +  static nscolor GetColorUsingStandins(ColorID aID,
> +                                       nscolor aDefault = NS_RGB(0, 0, 0)) {
> +    nscolor result = NS_RGB(0, 0, 0);

Nit: I think even for inline functions in class declarations (unless they're entirely on one line), the "{" should go on the next line.

Comment 20

[Chung-Sheng Fu [:cfu]]

(In reply to Cameron McCormack (:heycam) from comment #19)
> Comment on attachment 8876579 [details]
> Bug 1330876 - Add a new variant of LookAndFeel::GetColor that uses stand-ins
> for native colors and accepts default value.
> 
> https://reviewboard.mozilla.org/r/147912/#review152244
> 
> Looks good!
> 
> ::: widget/LookAndFeel.h:538
> (Diff revision 1)
> > +  static nscolor GetColorUsingStandins(ColorID aID,
> > +                                       nscolor aDefault = NS_RGB(0, 0, 0)) {
> > +    nscolor result = NS_RGB(0, 0, 0);
> 
> Nit: I think even for inline functions in class declarations (unless they're
> entirely on one line), the "{" should go on the next line.

Thanks for the rapid response!
I will fix the style error right now.
Thank you very much again :)

Comment 21

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8876579 [details]
Bug 1330876 - Add a new variant of LookAndFeel::GetColor that uses stand-ins for native colors and accepts default value.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147912/diff/1-2/

Comment 22

[Carsten Book [:Tomcat]]

has one open issue in mozreview that need to be fixed first before we can land this.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/281d514b9190
Listen to change event of preference "ui.use_standins_for_native_colors" and update default foreground and background colors r=heycam
https://hg.mozilla.org/integration/autoland/rev/bbe8f5a69341
Add a new variant of LookAndFeel::GetColor that uses stand-ins for native colors and accepts default value. r=heycam

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/281d514b9190
https://hg.mozilla.org/mozilla-central/rev/bbe8f5a69341

Comment 25

[Ethan Tseng [:ethan]]

Thanks for fixing this.  Good job, CS!
Cameron, thanks for the review. :)

Comment 26

[Tom Grabowski [:TomGrab]]

Ethan, the commenters in this bug talk a lot about "ui.use_standins_for_native_colors" preference in Firefox. However, I'm unable to find this preference in current Firefox Nightly. Instead, I have found "ui.use_native_colors" that seems to be working in a way similar to described above. Should I be using this new preference to verify this bug?

So far, my test is failing.  I'm using Mac OS 10.12.6 with Nightly 58.0a1 (2017-10-21) (64-bit)

Here are my steps:
1. Go to about:config
2. Set parameter "ui.use_native_colors" to false
3. Set parameter "privacy.resistFingerprinting" to true
4. Go to this page: https://www.w3schools.com/html/tryit.asp?filename=tryhtml_form_submit
5. Try to enter text in the "First name" input field

Expected result:
Entered text should be visible

Actual result:
Entered text is not visible


Note that this problem happens with "privacy.resistFingerprinting" set to false
What is the expected behavior here? If my steps and expected behavior are incorrect, could you please provide some information how this should be tested?

Comment 27

[Chung-Sheng Fu [:cfu]]

Hi Tom,

Thank you very much for your help with the tests.

This one is a little bit complicated because the problem can only be observed under high contrast mode (or similar system color settings?  I'm not sure).  In Windows 10 you can do the follows:

1. Open "Settings"
2. Enter "Ease of Access"
3. Select "High contrast"
4. Choose a theme, e.g., "High Contrast #1"

And then in Firefox:

1. Set "ui.use_native_colors" to true (I think it is true by default)
2. Go to some pages with text input fields, e.g., https://bugzilla.mozilla.org/
3. The text input field should be rendered with system color settings
   For example, if you are using "High Contrast #1", the background color will be black and the text color will be yellow

Now test the spoofed color values:

1. Set "ui.use_standins_for_native_colors" to true
2. Refresh the page (it should be automatically re-rendered after "ui.use_standins_for_native_colors" is toggled)
3. The text input field should have white background color and black text color

So, yes, this protection is not controlled by "privacy.resistFingerprinting" but "ui.use_standins_for_native_colors".  I remember this pref was listed in about:config when I was doing this bug, but I don't know when and why it is removed.  So please add a new boolean pref with this name.

I tried your link https://www.w3schools.com/html/tryit.asp?filename=tryhtml_form_submit but the text input fields have black background color when "ui.use_standins_for_native_colors" is true.  However, its background color I get programmatically (using getComputedStyle) is white, so I will investigate why it looks black.

Comment 28

[Tom Grabowski [:TomGrab]]

I didn't realize that I can just add "ui.use_standins_for_native_colors" pref and it will work.  I tried it now, and the behavior is as you described above. I should not have used "ui.use_native_colors" - it seems that this is what caused my results to be incorrect.