The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in mozilla37

Status

()

Core
Layout
VERIFIED FIXED
6 years ago
2 years ago

People

(Reporter: Max, Assigned: Gijs)

Tracking

(Blocks: 1 bug, {addon-compat})

Trunk
mozilla37
x86_64
Windows 7
addon-compat
Points:
8
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite -
qe-verify +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

6 years ago
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.
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/
(Reporter)

Comment 2

6 years ago
Yes, the problem persists with either the new profile or latest nightly build.
Version: unspecified → Trunk
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.
Component: Layout: View Rendering → Style System (CSS)
QA Contact: layout.view-rendering → style-system
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?
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
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.
(Reporter)

Comment 7

6 years ago
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.
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 9

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

Updated

2 years ago
Duplicate of this bug: 790706
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1062485
(Assignee)

Comment 12

2 years ago
Marco, can you add this to the spreadsheet?
Assignee: nobody → gijskruitbosch+bugs
Blocks: 343205
Points: --- → 8
Component: CSS Parsing and Computation → Widget
Flags: qe-verify+
Flags: needinfo?(mmucci)
Flags: in-testsuite?
Flags: firefox-backlog+
(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?
Flags: needinfo?(mmucci) → needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 14

2 years ago
Taking! :-)
Status: NEW → ASSIGNED
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #14)
> Taking! :-)

Thanks.  Added to IT 37.1
Iteration: --- → 37.1
(Assignee)

Comment 16

2 years ago
Hrmpf, I thought the pref code was in widget, but it's in layout. Moving some more, sorry for the bugspam.
Component: Widget → Layout
(Assignee)

Comment 17

2 years ago
Created attachment 8529295 [details] [diff] [review]
Part 1: update the layout code and test for the pref

First, we need to update the actual pref...
Attachment #8529295 - Flags: review?(dbaron)
Will the old pref be migrated to the new one?
(Assignee)

Comment 19

2 years ago
(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. :-)
(Assignee)

Comment 20

2 years ago
Created attachment 8529319 [details] [diff] [review]
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.
Attachment #8529319 - Flags: review?(jaws)
(Assignee)

Comment 21

2 years ago
Created attachment 8529326 [details] [diff] [review]
Part 2: UI changes and IE pref import/migration code

Got the IE migration wrong, d'oh.
Attachment #8529326 - Flags: review?(jaws)
(Assignee)

Updated

2 years ago
Attachment #8529319 - Attachment is obsolete: true
Attachment #8529319 - Flags: review?(jaws)
(Assignee)

Comment 22

2 years ago
Created attachment 8529327 [details] [diff] [review]
Part 3: Migrate users of the old pref
Attachment #8529327 - Flags: review?(jaws)
(Assignee)

Comment 23

2 years ago
Created a pull request for pdfjs here: https://github.com/mozilla/pdf.js/pull/5511
(Assignee)

Comment 24

2 years ago
Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1cf93fb5a88d
(Assignee)

Comment 25

2 years ago
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).
Flags: in-testsuite? → in-testsuite-
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1062011
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
Attachment #8529295 - Flags: review?(dbaron) → review+
(Assignee)

Comment 28

2 years ago
(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?
Flags: needinfo?(dbaron)
(Assignee)

Comment 29

2 years ago
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...
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.
Flags: needinfo?(dbaron)
(Assignee)

Comment 31

2 years ago
(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.
(Assignee)

Comment 32

2 years ago
Created attachment 8529880 [details] [diff] [review]
Part 1: update the layout code and test for the pref
Attachment #8529880 - Flags: review?(dbaron)
(Assignee)

Updated

2 years ago
Attachment #8529295 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8529880 - Attachment description: change document color pref to tristate, → Part 1: update the layout code and test for the pref
(Assignee)

Comment 33

2 years ago
Created attachment 8529882 [details] [diff] [review]
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.
Attachment #8529882 - Flags: review?(jaws)
(Assignee)

Updated

2 years ago
Attachment #8529326 - Attachment is obsolete: true
Attachment #8529326 - Flags: review?(jaws)
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 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.
Attachment #8529327 - Flags: review?(jaws) → review+
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.
Attachment #8529882 - Flags: review?(jaws) → review+
(Assignee)

Comment 37

2 years ago
(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 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
Attachment #8529880 - Flags: review?(dbaron) → review+
(Assignee)

Comment 39

2 years ago
(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
(Assignee)

Comment 40

2 years ago
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
https://hg.mozilla.org/mozilla-central/rev/2bf9a54ee65b
https://hg.mozilla.org/mozilla-central/rev/460d573b8822
https://hg.mozilla.org/mozilla-central/rev/e15514e6321f
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37

Comment 42

2 years ago
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.
QA Contact: cornel.ionce
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
Status: RESOLVED → VERIFIED

Comment 44

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

Updated

2 years ago
Duplicate of this bug: 1116570
(Assignee)

Comment 46

2 years ago
(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.
Flags: needinfo?(philip.chee)

Updated

2 years ago
Blocks: 1118032

Comment 47

2 years ago
(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
(Assignee)

Updated

2 years ago
Flags: needinfo?(philip.chee)
Summary: "Allow pages to choose their own colors" does not work with high contrast windows 7 themes → "Allow pages to choose their own colors" does not work with high contrast windows themes

Comment 48

2 years ago
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.
(Assignee)

Comment 49

2 years ago
(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

2 years ago
nice
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!
(Assignee)

Comment 51

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

Comment 52

2 years ago
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".
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.
You need to log in before you can comment on or make changes to this bug.