639134 - "Allow pages to choose their own colors" does not work with high contrast windows themes

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Max]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-GB; rv:1.9.2.14) Gecko/20110218 Firefox/3.6.14
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-GB; rv:1.9.2.14) Gecko/20110218 Firefox/3.6.14

When I select High contrast black theme in Windows 7 theme selector, the document colors revert to the theme default even if the checkbox "Allow pages to choose their own colors" is on. Moreover, all images are removed from the document view making some sites unusable.

Reproducible: Always

Steps to Reproduce:
1. Select High Contrast Black theme in Personalize.. in Windows 7

Actual Results:  
All document colors in each tab revert to theme default and images disappear.

Expected Results:  
I expect the colors and images to stay document-specified like they do on my Ubuntu box.

Comment 1

[Ed Morley [:emorley]]

Could you see if the issue occurs with a new, empty profile:
http://support.mozilla.com/en-US/kb/Basic%20Troubleshooting#w_make-a-new-profile

...or using the latest nightly:
http://nightly.mozilla.org/

Comment 2

[Max]

Yes, the problem persists with either the new profile or latest nightly build.

Comment 3

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

This is a "feature", implemented so far only for Windows (see bug 239914 comment 22).  I'm not sure what I think about it, though.

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

I recall a guideline that all functionality should be perceivable without images or color but I don't know the details. Note OS X does high contrast at a more raw level which affects text and images equally but doesn't otherwise change content.

(In reply to comment #0)
> When I select High contrast black theme in Windows 7 theme selector, the
> document colors revert to the theme default even if the checkbox "Allow pages
> to choose their own colors" is on.

Note that checkbox is only relevant to specific font and color changes the user has selected in the content preferences. I think it would be confusing to have it provide a dual purpose.

I doubt we could justify a specific 'prevent windows HCM interfering with content' preference unless we heard a lot more complaints.

Max you might be able to style Windows to do what you want without actually setting HCM (High Contrast Mode). If so would you be agreeable to closing this bug?

Comment 5

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

What I'm saying in comment 3 is that we did this quite intentionally -- we ignore the value of the pref when there's a high-contrast theme, and instead act as though the user unchecked "Allow pages to choose their own colors".  The code is here:
http://hg.mozilla.org/mozilla-central/file/4e771e65764a/layout/base/nsPresContext.cpp#l631

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

Oh! I see. Hmm. I think we do the "right thing" then, for Windows. The "right thing" for GNOME seems unresolved.

So this bug is probably INVALID or WONTFIX IMO.

Comment 7

[Max]

The problem with this feature is that it takes away all of control from user and breaks the "Allow pages to choose their own colors"/"Use system colors" checkboxes entirely which will always be perceived as a bug. So I would really prefer it to do what it does normally.

I've read the 239914 discussion. While I understand the desire of some users to have this behavior, without this feature it can be done with checking both of these checkboxes. In the case of current implementation it cannot be done at all.

> Max you might be able to style Windows to do what you want without actually
setting HCM (High Contrast Mode). If so would you be agreeable to closing this
bug?

I can make a workaround, but I'd prefer it to be resolved in some more comprehensible way. For example, if you want to leave the old behavior, disable the checkboxes in the HCM case so that user will know something is different.

Comment 8

[David Bolter [:davidb] (NeedInfo me for attention)]

Max you seem to make a fair point IMO. I've cc'ed Alexander Limi to see if we can get some Ux guidance here.

Comment 9

[Khvoinitsky Mikhail [:m_khvoinitsky]]

Please, take a look at this my comment https://bugzilla.mozilla.org/show_bug.cgi?id=697836#c2

Comment 12

[:Gijs (he/him)]

Marco, can you add this to the spreadsheet?

Comment 13

[Marco Mucci [:MarcoM]]

(In reply to :Gijs Kruitbosch from comment #12)
> Marco, can you add this to the spreadsheet?

Added to priority sheet.  Are you taking this bug now or just add to the sheet for selection later on?

Comment 14

[:Gijs (he/him)]

Taking! :-)

Comment 15

[Marco Mucci [:MarcoM]]

(In reply to :Gijs Kruitbosch from comment #14)
> Taking! :-)

Thanks.  Added to IT 37.1

Comment 16

[:Gijs (he/him)]

Hrmpf, I thought the pref code was in widget, but it's in layout. Moving some more, sorry for the bugspam.

Comment 17

[:Gijs (he/him)]

Attachment: Part 1: update the layout code and test for the pref

First, we need to update the actual pref...

Comment 18

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

Will the old pref be migrated to the new one?

Comment 19

[:Gijs (he/him)]

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #18)
> Will the old pref be migrated to the new one?

Yes, still working on that, sorry for not being clear immediately. This is why I chose a new pref name (somewhat clumsy, suggestions welcome). It'll go into nsBrowserGlue.js as a pref migration for Firefox; I'm not aware of a better way to do this (so Thunderbird/SeaMonkey will have to do similar things if they want to migrate the pref).

There was some discussion on the dupes as well as out-of-bugzilla with dbolter about what to do here. Basically, I think we should have a tristate pref between automatic/always/never, with automatic essentially replacing the current 'yes, let pages pick' state, being the default. That way high contrast mode users can pick "always". "never" would still always override page colors.

I will be attaching patches for the frontend change here (basically, the preferences UI and the IE profile migrator need updating, and I'll need to make a pull request to pdfjs), and one for the pref migration, and seek review there from browser peers, assuming you think that makes sense. :-)

Comment 20

[:Gijs (he/him)]

Attachment: Part 2: UI changes and IE pref import/migration code

These would be the UI changes. Jared, I noticed a small issue with the styling of menulists in subdialogs. It also affects the lists in the fonts subdialog, so I'll file a separate bug on that. Pref migration patch coming up next.

Comment 21

[:Gijs (he/him)]

Attachment: Part 2: UI changes and IE pref import/migration code

Got the IE migration wrong, d'oh.

Comment 22

[:Gijs (he/him)]

Attachment: Part 3: Migrate users of the old pref

Comment 23

[:Gijs (he/him)]

Created a pull request for pdfjs here: https://github.com/mozilla/pdf.js/pull/5511

Comment 24

[:Gijs (he/him)]

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1cf93fb5a88d

Comment 25

[:Gijs (he/him)]

I don't think we can influence the windows theme from tests, so testsuite-'ing (although this updates the testsuite affected, that's not super good coverage).

Comment 27

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

Comment on attachment 8529295 [details] [diff] [review]
Part 1: update the layout code and test for the pref

>+#define DOCUMENT_COLORS_PREF_NAME "browser.display.document_color_use"

I'm not convinced this is worth the bother; I'd be inclined to just inline this.

>diff --git a/layout/base/nsLayoutUtils.h b/layout/base/nsLayoutUtils.h
>   /**
>+   * Checks whether we should honor the document's colors
>+   */
>+  static bool UseDocumentColors();

The comment here should say whether to honor the document's colors for non-chrome documents.

>diff --git a/layout/base/nsPresContext.cpp b/layout/base/nsPresContext.cpp
>-  mUseDocumentColors = !useAccessibilityTheme &&
>-    Preferences::GetBool("browser.display.use_document_colors",
>-                         mUseDocumentColors);
>+  mUseDocumentColors = nsLayoutUtils::UseDocumentColors();

This should also make sure to have mUseDocumentColors be true for chrome documents and chrome-origin images, as the current code does.  So this should be

  mUseDocumentColors = isChromeDocShell || mIsChromeOriginImage ||
                       nsLayoutUtils::UseDocumentColors();

or something similar.

Otherwise you'll break the colors in the browser, no?


r=dbaron with that

I'm assuming that this combined with the other patches hits all the results in
http://mxr.mozilla.org/mozilla-central/search?string=use_document_colors
though I didn't check

Comment 28

[:Gijs (he/him)]

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #27)
> >diff --git a/layout/base/nsPresContext.cpp b/layout/base/nsPresContext.cpp
> >-  mUseDocumentColors = !useAccessibilityTheme &&
> >-    Preferences::GetBool("browser.display.use_document_colors",
> >-                         mUseDocumentColors);
> >+  mUseDocumentColors = nsLayoutUtils::UseDocumentColors();
> 
> This should also make sure to have mUseDocumentColors be true for chrome
> documents and chrome-origin images, as the current code does.  So this
> should be
> 
>   mUseDocumentColors = isChromeDocShell || mIsChromeOriginImage ||
>                        nsLayoutUtils::UseDocumentColors();
> 
> or something similar.
> 
> Otherwise you'll break the colors in the browser, no?

The current code doesn't do this, I think? AFAICT that part gets done by:

http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresContext.h#853

853   // Is it OK to let the page specify colors and backgrounds?
854   bool UseDocumentColors() const {
855     return GetCachedBoolPref(kPresContext_UseDocumentColors) || IsChrome() || IsChromeOriginImage();
856   }

(where the first call is basically a getter for mUseDocumentColors)

I'm wary of trying to push this down into this method because I would be worried that any of the other conditions can change at runtime for a single prescontext. Indeed, it seems IsChrome() is not guaranteed to stay the same for a prescontext:

http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#3248

so I don't think I should touch it here. Does that sound right?

Comment 29

[:Gijs (he/him)]

That said, considering the method has the same name as the one I'm adding on nsLayoutUtils, perhaps I should rename the method I'm adding to be more clear about the fact that it's just checking the pref and the theme? Or are you saying that just expanding the comment can take care of that?

Alternatively, I could just not bother with adding it to nsLayoutUtils and inline the logic into nsPresContext::GetDocumentColorPreferences...

Comment 30

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

The current code does do it because:

  int32_t useAccessibilityTheme = 0;

...

  if (isChromeDocShell || mIsChromeOriginImage) {
    usePrefColors = false;
  } else {
    useAccessibilityTheme =
      LookAndFeel::GetInt(LookAndFeel::eIntID_UseAccessibilityTheme, 0);
    usePrefColors = !useAccessibilityTheme;
  }

...

  mUseDocumentColors = !useAccessibilityTheme &&
    Preferences::GetBool("browser.display.use_document_colors",
                         mUseDocumentColors);


Note that useAccessibilityTheme is only set conditionally.

That said, it may well not be needed if that nsPresContext::UseDocumentColors handles it.  (Also, going through GetCachedBoolPref seems silly.)


I think we assume that IsChrome doesn't change over time -- I hope that only changes during prescontext setup.  But I might be wrong.

Comment 31

[:Gijs (he/him)]

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #30)
> <snip>
> Note that useAccessibilityTheme is only set conditionally.
> 
> That said, it may well not be needed if that
> nsPresContext::UseDocumentColors handles it.  (Also, going through
> GetCachedBoolPref seems silly.)
> 
> 
> I think we assume that IsChrome doesn't change over time -- I hope that only
> changes during prescontext setup.  But I might be wrong.

Sorry, you're absolutely right. And yeah, we seem to just be checking the same thing a bunch of times...

I've tested a patch that makes nsPresContext::UseDocumentColors just a simple getter for mUseDocumentColors, and inlining the pref logic into GetDocumentColorPreferences. That seems to work for me, so I'll attach that shortly.

Comment 32

[:Gijs (he/him)]

Attachment: Part 1: update the layout code and test for the pref

Comment 33

[:Gijs (he/him)]

Attachment: Part 2: UI changes and IE pref import/migration code

The try push revealed a silly lacking closing brace in IEProfileMigrator.js (saved by my own test, for once). Fixed here. I'd do a new try push but the tree is closed.

Comment 34

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8529327 [details] [diff] [review]
Part 3: Migrate users of the old pref

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

Thanks for adding the comments to all.js. I don't particularly like "document_color_use" as 'document' may be interpreted as a verb here. Perhaps we could invert it and go with "browser.display.override_document_colors"?

Comment 35

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8529327 [details] [diff] [review]
Part 3: Migrate users of the old pref

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

Whoops, meant to r+ this.

Comment 36

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8529882 [details] [diff] [review]
Part 2: UI changes and IE pref import/migration code

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

::: browser/components/preferences/colors.xul
@@ +86,5 @@
> +      <menulist id="useDocumentColors" preference="browser.display.document_color_use">
> +        <menupopup>
> +          <menuitem label="&allowPagesToUseColors.automatic.label;" value="0" id="documentColorAutomatic"/>
> +          <menuitem label="&allowPagesToUseColors.always.label;"    value="1" id="documentColorAlways"/>
> +          <menuitem label="&allowPagesToUseColors.never.label;"     value="2" id="documentColorNever"/>

Should these have access keys? I'm not sure what the convention is here. My gut instinct says that with only three options we don't need access keys.

Comment 37

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #36)
> Comment on attachment 8529882 [details] [diff] [review]
> Part 2: UI changes and IE pref import/migration code
> 
> Review of attachment 8529882 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/preferences/colors.xul
> @@ +86,5 @@
> > +      <menulist id="useDocumentColors" preference="browser.display.document_color_use">
> > +        <menupopup>
> > +          <menuitem label="&allowPagesToUseColors.automatic.label;" value="0" id="documentColorAutomatic"/>
> > +          <menuitem label="&allowPagesToUseColors.always.label;"    value="1" id="documentColorAlways"/>
> > +          <menuitem label="&allowPagesToUseColors.never.label;"     value="2" id="documentColorNever"/>
> 
> Should these have access keys? I'm not sure what the convention is here. My
> gut instinct says that with only three options we don't need access keys.

This was my assumption, too. Other things with 3 options also don't have access keys:

http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/main.xul#113

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #34)
> Comment on attachment 8529327 [details] [diff] [review]
> Part 3: Migrate users of the old pref
> 
> Review of attachment 8529327 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for adding the comments to all.js. I don't particularly like
> "document_color_use" as 'document' may be interpreted as a verb here.
> Perhaps we could invert it and go with
> "browser.display.override_document_colors"?

That negates the pref name's meaning... not a big fan. Taking document out also seems odd because we still have use_document_fonts. Could do document_overrides_default_colors ? I'll leave it up to dbaron, if that's OK. :-)

Comment 38

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

Comment on attachment 8529880 [details] [diff] [review]
Part 1: update the layout code and test for the pref

>+    // NB: this will only be true if !isChromeDocShell and !mIsChromeOriginImage,
>+    // as that's the only case where useAccessibilityTheme can be non-0 at this point.

Maybe assert this?

Also, please wrap the comment at less than 80 characters.

>+    MOZ_ASSERT(!mUseDocumentColors && (IsChrome() || IsChromeOriginImage()),
>+               "We should never have a chrome doc or image that can't use its colors.");

This assertion is the wrong way around.  (You clearly didn't test this
in a debug build!)

It should be
  mUseDocumentColors || !(IsChrome() || IsChromeOriginImage())


Please also remove kPresContext_UseDocumentColors and the case for it
in GetCachedBoolPref, now that you've removed the only user
(UseDocumentColors()).

r=dbaron with that

Comment 39

[:Gijs (he/him)]

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #38)
> This assertion is the wrong way around.  (You clearly didn't test this
> in a debug build!)

Yes, I'm sorry. :-(

(I was under some time pressure and didn't have time to do a windows clobber debug build, and try was closed)

Updated with the above changes (and checked locally for not hitting either of the asserts on my osx debug build...):
remote:   https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=74830881a0aa

Comment 40

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/2bf9a54ee65b
remote:   https://hg.mozilla.org/integration/fx-team/rev/460d573b8822
remote:   https://hg.mozilla.org/integration/fx-team/rev/e15514e6321f

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2bf9a54ee65b
https://hg.mozilla.org/mozilla-central/rev/460d573b8822
https://hg.mozilla.org/mozilla-central/rev/e15514e6321f

Comment 42

[alreiten]

I think this may break the add-on Blank Your Monitor + Easy Reading.  Under option 0 and 1 for the new preference, the add-on functionality does not work--you can't change to the darker mode.  Under option 2, the add-on functionality does allow changes, but only between the darker mode and a weird no-color white style.  I have e-mailed the add-on developer.  I would think this change will also break similar add-ons, as well.

Comment 43

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0
Reproduced the issue on Firefox 35 beta2, build ID: 20141208150535.

Confirming that "Allow pages to choose their own colors" option now works in High Contrast Themes using latest Nightly, build ID: 20141210030207.
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:37.0) Gecko/20100101 Firefox/37.0

Comment 44

[Jefferson]

Any chance of uplifting this to Aurora or Beta? It came up on SuMo again today: https://support.mozilla.org/questions/1038798

Comment 46

[:Gijs (he/him)]

(In reply to Jefferson from comment #44)
> Any chance of uplifting this to Aurora or Beta? It came up on SuMo again
> today: https://support.mozilla.org/questions/1038798

Unfortunately not, because we added new strings, so that isn't upliftable. Note that this is by no means a recent change, so I'm not sure why there is suddenly a flood of requests regarding this. :-)

Philip, you asked to change strings here on m.d.platform. Did you ever file a bug with a concrete proposal? If we want to do something here, we should do it before this branches.

Comment 47

[Philip Chee]

(In reply to Gijs Kruitbosch from comment #46)

> Philip, you asked to change strings here on m.d.platform. Did you ever file
> a bug with a concrete proposal? If we want to do something here, we should
> do it before this branches.
Sorry I didn't file a bug. Will do now => Bug 1118032

Comment 48

[Greg]

Gijs Kruitbosch, thank you for all of your efforts in resolving this issue.  

When will this fix be implemented?  I am waiting for the implementation of this fix before I update my version of Firefox (v31.0) to the current version.

Again, thank you for all of your efforts.

Comment 49

[:Gijs (he/him)]

(In reply to Greg from comment #48)
> Gijs Kruitbosch, thank you for all of your efforts in resolving this issue.  
> 
> When will this fix be implemented?  I am waiting for the implementation of
> this fix before I update my version of Firefox (v31.0) to the current
> version.
> 
> Again, thank you for all of your efforts.

Firefox 37, which is going to be in beta at the end of this week, and will be released (all being well) on March 31st.

Comment 50

[Greg]

Thanks again, Gijs, for fixing this.  I just installed v37 today, and it works like a charm!  It's great to be back, using the most current version of Firefox!

Comment 51

[:Gijs (he/him)]

This is impacting add-ons and I forgot to set the addon-compat flag. :-(

Comment 52

[David E. Ross]

Note that implementation of this RFE breaks the Colors checkbox of the Prefbar extension.  The author of that extension describes the Colors checkbox as "Perhaps the most useful checkbox of them all".

Comment 53

[Jorge Villalobos [:jorgev] (he/him)]

I'll notify the developer. The other add-ons I see using this pref have already been updated. I'll also update the blog post to include this.