Bug 1698284 - Make the non-native theme not use system colors if we're not overriding the pages' colors. r=morgan
authorEmilio Cobos Alvarez <emilio@mozilla.com>
Tue, 16 Mar 2021 16:46:05 +0000
changeset 571460 400864685abceebe01810269e7a31a92613bf481
parent 571459 6c4aa4fa9e25dc26b7e4b0d296dbd89386ae3402
child 571461 380765501b826aea272f386e28245012b3c35724
push id138679
push userealvarez@mozilla.com
push dateTue, 16 Mar 2021 17:07:37 +0000
treeherderautoland@400864685abc [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmorgan
bugs1698284
milestone88.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1698284 - Make the non-native theme not use system colors if we're not overriding the pages' colors. r=morgan This prevents contrast issues with high contrast themes when the page only specifies some of the colors (which is an issue which we have historically had on GTK with dark themes for a long time). Feel free to push back on this if you prefer this, as on GTK we force a light theme on content anyways, but this is a problem on windows for users that use a high contrast theme but allow pages to override colors. Differential Revision: https://phabricator.services.mozilla.com/D108324
layout/style/GeckoBindings.cpp
layout/style/PreferenceSheet.h
widget/nsNativeBasicTheme.cpp
--- a/layout/style/GeckoBindings.cpp
+++ b/layout/style/GeckoBindings.cpp
@@ -710,17 +710,18 @@ static bool ShouldUseStandinsForNativeCo
     case ColorID::MozButtonhovertext:
     case ColorID::MozGtkButtonactivetext:
 
     case ColorID::MozCombobox:
     case ColorID::MozComboboxtext:
 
     case ColorID::Field:
     case ColorID::Fieldtext:
-      return !PreferenceSheet::PrefsFor(aDoc).mUseAccessibilityTheme;
+      return !PreferenceSheet::PrefsFor(aDoc)
+                  .NonNativeThemeShouldUseSystemColors();
 
     default:
       break;
   }
 
   return false;
 }
 
--- a/layout/style/PreferenceSheet.h
+++ b/layout/style/PreferenceSheet.h
@@ -36,16 +36,24 @@ struct PreferenceSheet {
 
     bool mUnderlineLinks = true;
     bool mUseFocusColors = false;
     bool mUseDocumentColors = true;
     uint8_t mFocusRingWidth = 1;
     uint8_t mFocusRingStyle = 1;
     bool mFocusRingOnAnything = false;
 
+    // Whether the non-native theme should use system colors for widgets.
+    // We only do that if we have a high-contrast theme _and_ we are overriding
+    // the document colors. Otherwise it causes issues when pages only override
+    // some of the system colors, specially in dark themes mode.
+    bool NonNativeThemeShouldUseSystemColors() const {
+      return mUseAccessibilityTheme && !mUseDocumentColors;
+    }
+
     void Load(bool aIsChrome);
   };
 
   static void EnsureInitialized() {
     if (sInitialized) {
       return;
     }
     Initialize();
--- a/widget/nsNativeBasicTheme.cpp
+++ b/widget/nsNativeBasicTheme.cpp
@@ -1588,17 +1588,17 @@ static LayoutDeviceRect ToSnappedRect(
   return LayoutDeviceRect::FromAppUnits(aRect, aTwipsPerPixel);
 }
 
 auto nsNativeBasicTheme::ShouldUseSystemColors(const dom::Document& aDoc)
     -> UseSystemColors {
   // TODO: Do we really want to use system colors even when the page can
   // override the high contrast theme? (mUseDocumentColors = true?).
   return UseSystemColors(
-      PreferenceSheet::PrefsFor(aDoc).mUseAccessibilityTheme);
+      PreferenceSheet::PrefsFor(aDoc).NonNativeThemeShouldUseSystemColors());
 }
 
 template <typename PaintBackendData>
 bool nsNativeBasicTheme::DoDrawWidgetBackground(PaintBackendData& aPaintData,
                                                 nsIFrame* aFrame,
                                                 StyleAppearance aAppearance,
                                                 const nsRect& aRect) {
   static_assert(std::is_same_v<PaintBackendData, DrawTarget> ||