Bug 1636998 - Only suppress auto-style outlines for widgets that paint their own focus indicator. r=jfkthame

Turns out we did have a hook for this already! But it is used to draw or
not inner button styles, so not quite equivalent.

I had to expand the amount of things it applies to because buttons and
such do paint focus indicators in all widgets. This patch could cause
some undesired outlines in some widgets. I hope not (I tried to audit to
the best of my knowledge), but in that case they'd be just more values
to add to the list.

Differential Revision: https://phabricator.services.mozilla.com/D74733

gfx/src/nsITheme.h

@@ -213,6 +213,17 @@ class nsITheme : public nsISupports {
    */
   virtual bool ThemeDrawsFocusForWidget(StyleAppearance aWidgetType) = 0;
 
+  /**
+   * Whether we want an inner focus ring for buttons and such.
+   *
+   * Usually, we don't want it if we have our own focus indicators, but windows
+   * is special, because it wants it even though focus also alters the border
+   * color and such.
+   */
+  virtual bool ThemeWantsButtonInnerFocusRing(StyleAppearance aAppearance) {
+    return !ThemeDrawsFocusForWidget(aAppearance);
+  }
+
   /**
    * Should we insert a dropmarker inside of combobox button?
    */

layout/forms/nsButtonFrameRenderer.cpp

@@ -347,7 +347,7 @@ void nsDisplayButtonForeground::Paint(nsDisplayListBuilder* aBuilder,
   nsPresContext* presContext = mFrame->PresContext();
   const nsStyleDisplay* disp = mFrame->StyleDisplay();
   if (!mFrame->IsThemed(disp) ||
-      !presContext->Theme()->ThemeDrawsFocusForWidget(disp->mAppearance)) {
+      presContext->Theme()->ThemeWantsButtonInnerFocusRing(disp->mAppearance)) {
     nsRect r = nsRect(ToReferenceFrame(), mFrame->GetSize());
 
     // Draw the -moz-focus-inner border
@@ -369,7 +369,7 @@ bool nsDisplayButtonForeground::CreateWebRenderCommands(
   nsPresContext* presContext = mFrame->PresContext();
   const nsStyleDisplay* disp = mFrame->StyleDisplay();
   if (!mFrame->IsThemed(disp) ||
-      !presContext->Theme()->ThemeDrawsFocusForWidget(disp->mAppearance)) {
+      presContext->Theme()->ThemeWantsButtonInnerFocusRing(disp->mAppearance)) {
     nsRect r = nsRect(ToReferenceFrame(), mFrame->GetSize());
     br = mBFR->CreateInnerFocusBorderRenderer(aDisplayListBuilder, presContext,
                                               nullptr, GetPaintRect(), r,

layout/forms/nsComboboxControlFrame.cpp

@@ -1504,7 +1504,8 @@ void nsComboboxControlFrame::BuildDisplayList(nsDisplayListBuilder* aBuilder,
     if (window && window->ShouldShowFocusRing()) {
       nsPresContext* presContext = PresContext();
       const nsStyleDisplay* disp = StyleDisplay();
-      if ((!IsThemed(disp) || !presContext->Theme()->ThemeDrawsFocusForWidget(
+      if ((!IsThemed(disp) ||
+           presContext->Theme()->ThemeWantsButtonInnerFocusRing(
                disp->mAppearance)) &&
           mDisplayFrame && IsVisibleForPainting()) {
         aLists.Content()->AppendNewToTop<nsDisplayComboboxFocus>(aBuilder,

layout/forms/nsRangeFrame.cpp

@@ -261,8 +261,11 @@ void nsRangeFrame::BuildDisplayList(nsDisplayListBuilder* aBuilder,
     return;
   }
 
-  if (IsThemed(disp) &&
-      PresContext()->Theme()->ThemeDrawsFocusForWidget(disp->mAppearance)) {
+  // FIXME(emilio): This is using ThemeWantsButtonInnerFocusRing even though
+  // it's painting the ::-moz-focus-outer pseudo-class... But why is
+  // ::-moz-focus-outer useful, instead of outline?
+  if (IsThemed(disp) && !PresContext()->Theme()->ThemeWantsButtonInnerFocusRing(
+                            disp->mAppearance)) {
     return;  // the native theme displays its own visual indication of focus
   }
 

layout/generic/nsFrame.cpp

@@ -2625,13 +2625,19 @@ void nsFrame::DisplayOutlineUnconditional(nsDisplayListBuilder* aBuilder,
     return;
   }
 
-  // We don't display outline-style: auto on themed frames.
-  //
-  // TODO(emilio): Maybe we want a theme hook to say which frames can handle it
-  // themselves. Non-native theme probably will want this.
-  if (outline.mOutlineStyle.IsAuto() && IsThemed()) {
+  // We don't display outline-style: auto on themed frames that have their own
+  // focus indicators.
+  if (outline.mOutlineStyle.IsAuto()) {
+    auto* disp = StyleDisplay();
+    // FIXME(emilio): The range special-case is needed because <input
+    // type=range> displays its own outline with ::-moz-focus-outer, and this
+    // would show two outlines instead of one.
+    if (IsThemed(disp) &&
+        (PresContext()->Theme()->ThemeDrawsFocusForWidget(disp->mAppearance) ||
+         disp->mAppearance != StyleAppearance::Range)) {
       return;
     }
+  }
 
   aLists.Outlines()->AppendNewToTop<nsDisplayOutline>(aBuilder, this);
 }

layout/style/res/forms.css

@@ -470,10 +470,6 @@ input[type="image"]:disabled {
   cursor: unset;
 }
 
-input[type="image"]:-moz-focusring {
-  /* Don't specify the outline-color, we should always use initial value. */
-  outline: 1px dotted;
-}
 
 /* file selector */
 input[type="file"] {
@@ -562,15 +558,15 @@ input[type="checkbox"]:disabled:hover:active {
   cursor: unset;
 }
 
-% On Mac, the native theme takes care of this.
-% See nsNativeThemeCocoa::ThemeDrawsFocusForWidget.
-%ifndef XP_MACOSX
-input[type="checkbox"]:-moz-focusring,
-input[type="radio"]:-moz-focusring {
-  /* Don't specify the outline-color, we should always use initial value. */
-  outline: 1px dotted;
+input:not([type="file"]):not([type="image"]):-moz-focusring,
+select:-moz-focusring,
+button:-moz-focusring,
+textarea:-moz-focusring {
+  /* These elements can handle outline themselves when themed, so we use
+   * outline-style: auto and skip rendering the outline only when themed and
+   * the theme allows so */
+  outline-style: auto;
 }
-%endif
 
 input[type="search"] {
   box-sizing: border-box;

layout/style/res/html.css

@@ -726,16 +726,6 @@ html:-moz-focusring {
   outline-style: none;
 }
 
-input:not([type="file"]):-moz-focusring,
-button:-moz-focusring,
-select:-moz-focusring,
-button:-moz-focusring,
-textarea:-moz-focusring {
-  /* These elements can handle outline themselves when themed, so we use
-   * outline-style: auto and skip rendering the outline only when themed */
-  outline-style: auto;
-}
-
 /* hidden elements */
 base, basefont, datalist, head, meta, script, style, title,
 noembed, param, template {

widget/cocoa/nsNativeThemeCocoa.mm

@@ -4204,16 +4204,25 @@ bool nsNativeThemeCocoa::WidgetIsContainer(StyleAppearance aAppearance) {
 }
 
 bool nsNativeThemeCocoa::ThemeDrawsFocusForWidget(StyleAppearance aAppearance) {
-  if (aAppearance == StyleAppearance::MenulistButton || aAppearance == StyleAppearance::Button ||
-      aAppearance == StyleAppearance::MozMacHelpButton ||
-      aAppearance == StyleAppearance::MozMacDisclosureButtonOpen ||
-      aAppearance == StyleAppearance::MozMacDisclosureButtonClosed ||
-      aAppearance == StyleAppearance::Radio || aAppearance == StyleAppearance::Range ||
-      aAppearance == StyleAppearance::Checkbox)
+  switch (aAppearance) {
+    case StyleAppearance::Textarea:
+    case StyleAppearance::Textfield:
+    case StyleAppearance::Searchfield:
+    case StyleAppearance::NumberInput:
+    case StyleAppearance::Menulist:
+    case StyleAppearance::MenulistButton:
+    case StyleAppearance::Button:
+    case StyleAppearance::MozMacHelpButton:
+    case StyleAppearance::MozMacDisclosureButtonOpen:
+    case StyleAppearance::MozMacDisclosureButtonClosed:
+    case StyleAppearance::Radio:
+    case StyleAppearance::Range:
+    case StyleAppearance::Checkbox:
       return true;
-
+    default:
       return false;
   }
+}
 
 bool nsNativeThemeCocoa::ThemeNeedsComboboxDropmarker() { return false; }
 

widget/gtk/nsNativeThemeGTK.cpp

@@ -1972,14 +1972,20 @@ nsNativeThemeGTK::WidgetIsContainer(StyleAppearance aAppearance) {
 }
 
 bool nsNativeThemeGTK::ThemeDrawsFocusForWidget(StyleAppearance aAppearance) {
-  if (aAppearance == StyleAppearance::Menulist ||
-      aAppearance == StyleAppearance::MenulistButton ||
-      aAppearance == StyleAppearance::Button ||
-      aAppearance == StyleAppearance::Treeheadercell)
+  switch (aAppearance) {
+    case StyleAppearance::Button:
+    case StyleAppearance::Menulist:
+    case StyleAppearance::MenulistButton:
+    case StyleAppearance::MenulistTextfield:
+    case StyleAppearance::Textarea:
+    case StyleAppearance::Textfield:
+    case StyleAppearance::Treeheadercell:
+    case StyleAppearance::NumberInput:
       return true;
-
+    default:
       return false;
   }
+}
 
 bool nsNativeThemeGTK::ThemeNeedsComboboxDropmarker() { return false; }
 

widget/nsNativeBasicTheme.cpp

@@ -910,8 +910,16 @@ bool nsNativeBasicTheme::WidgetIsContainer(StyleAppearance aAppearance) {
 }
 
 bool nsNativeBasicTheme::ThemeDrawsFocusForWidget(StyleAppearance aAppearance) {
+  switch (aAppearance) {
+    // TODO(emilio): Checkbox / Radio don't have focus indicators when checked.
+    // If they did, we could just return true here unconditionally.
+    case StyleAppearance::Checkbox:
+    case StyleAppearance::Radio:
+      return false;
+    default:
       return true;
   }
+}
 
 bool nsNativeBasicTheme::ThemeNeedsComboboxDropmarker() { return true; }
 

widget/windows/nsNativeThemeWin.cpp

@@ -2601,8 +2601,18 @@ bool nsNativeThemeWin::WidgetIsContainer(StyleAppearance aAppearance) {
 }
 
 bool nsNativeThemeWin::ThemeDrawsFocusForWidget(StyleAppearance aAppearance) {
+  switch (aAppearance) {
+    case StyleAppearance::Menulist:
+    case StyleAppearance::MenulistButton:
+    case StyleAppearance::MenulistTextfield:
+    case StyleAppearance::Textarea:
+    case StyleAppearance::Textfield:
+    case StyleAppearance::NumberInput:
+      return true;
+    default:
       return false;
   }
+}
 
 bool nsNativeThemeWin::ThemeNeedsComboboxDropmarker() { return true; }
 

widget/windows/nsNativeThemeWin.h

@@ -68,6 +68,8 @@ class nsNativeThemeWin : private nsNativeTheme, public nsITheme {
 
   bool ThemeDrawsFocusForWidget(StyleAppearance aAppearance) override;
 
+  bool ThemeWantsButtonInnerFocusRing(StyleAppearance) override { return true; }
+
   bool ThemeNeedsComboboxDropmarker() override;
 
   virtual bool WidgetAppearanceDependsOnWindowFocus(