Backed out changeset f5546bfc9604 (bug 1636998) for reftest failures on 1174332-1.html . CLOSED TREE
authorNarcis Beleuzu <nbeleuzu@mozilla.com>
Wed, 13 May 2020 23:22:56 +0300
changeset 529698 6a0cd004fa71b854f321889b13c047e387851247
parent 529697 6a8277223f6d9e40b0edc84499870a9e64354d6d
child 529699 5b981d3376fe4707a6f93c70c1ef97cc075a355b
push id37414
push usernbeleuzu@mozilla.com
push dateThu, 14 May 2020 02:40:10 +0000
treeherdermozilla-central@045d696faa87 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs1636998, 1174332
milestone78.0a1
backs outf5546bfc96041192518676f44d328ac533b595b2
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
Backed out changeset f5546bfc9604 (bug 1636998) for reftest failures on 1174332-1.html . CLOSED TREE
gfx/src/nsITheme.h
layout/forms/nsButtonFrameRenderer.cpp
layout/forms/nsComboboxControlFrame.cpp
layout/forms/nsRangeFrame.cpp
layout/generic/nsFrame.cpp
layout/style/res/forms.css
layout/style/res/html.css
widget/cocoa/nsNativeThemeCocoa.mm
widget/gtk/nsNativeThemeGTK.cpp
widget/nsNativeBasicTheme.cpp
widget/windows/nsNativeThemeWin.cpp
widget/windows/nsNativeThemeWin.h
--- a/gfx/src/nsITheme.h
+++ b/gfx/src/nsITheme.h
@@ -209,27 +209,16 @@ class nsITheme : public nsISupports {
   virtual bool WidgetIsContainer(StyleAppearance aWidgetType) = 0;
 
   /**
    * Does the nsITheme implementation draw its own focus ring for this widget?
    */
   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?
    */
   virtual bool ThemeNeedsComboboxDropmarker() = 0;
 };
 
 NS_DEFINE_STATIC_IID_ACCESSOR(nsITheme, NS_ITHEME_IID)
 
 // Singleton accessor functions, these should never return null.
--- a/layout/forms/nsButtonFrameRenderer.cpp
+++ b/layout/forms/nsButtonFrameRenderer.cpp
@@ -342,17 +342,17 @@ void nsDisplayButtonForeground::ComputeI
   nsDisplayItem::ComputeInvalidationRegion(aBuilder, aGeometry, aInvalidRegion);
 }
 
 void nsDisplayButtonForeground::Paint(nsDisplayListBuilder* aBuilder,
                                       gfxContext* aCtx) {
   nsPresContext* presContext = mFrame->PresContext();
   const nsStyleDisplay* disp = mFrame->StyleDisplay();
   if (!mFrame->IsThemed(disp) ||
-      presContext->Theme()->ThemeWantsButtonInnerFocusRing(disp->mAppearance)) {
+      !presContext->Theme()->ThemeDrawsFocusForWidget(disp->mAppearance)) {
     nsRect r = nsRect(ToReferenceFrame(), mFrame->GetSize());
 
     // Draw the -moz-focus-inner border
     ImgDrawResult result = mBFR->PaintInnerFocusBorder(
         aBuilder, presContext, *aCtx, GetPaintRect(), r);
 
     nsDisplayItemGenericImageGeometry::UpdateDrawResult(this, result);
   }
@@ -364,17 +364,17 @@ bool nsDisplayButtonForeground::CreateWe
     const StackingContextHelper& aSc,
     mozilla::layers::RenderRootStateManager* aManager,
     nsDisplayListBuilder* aDisplayListBuilder) {
   Maybe<nsCSSBorderRenderer> br;
   bool borderIsEmpty = false;
   nsPresContext* presContext = mFrame->PresContext();
   const nsStyleDisplay* disp = mFrame->StyleDisplay();
   if (!mFrame->IsThemed(disp) ||
-      presContext->Theme()->ThemeWantsButtonInnerFocusRing(disp->mAppearance)) {
+      !presContext->Theme()->ThemeDrawsFocusForWidget(disp->mAppearance)) {
     nsRect r = nsRect(ToReferenceFrame(), mFrame->GetSize());
     br = mBFR->CreateInnerFocusBorderRenderer(aDisplayListBuilder, presContext,
                                               nullptr, GetPaintRect(), r,
                                               &borderIsEmpty);
   }
 
   if (!br) {
     return borderIsEmpty;
--- a/layout/forms/nsComboboxControlFrame.cpp
+++ b/layout/forms/nsComboboxControlFrame.cpp
@@ -1499,19 +1499,18 @@ void nsComboboxControlFrame::BuildDispla
   }
 
   // draw a focus indicator only when focus rings should be drawn
   if (Document* doc = mContent->GetComposedDoc()) {
     nsPIDOMWindowOuter* window = doc->GetWindow();
     if (window && window->ShouldShowFocusRing()) {
       nsPresContext* presContext = PresContext();
       const nsStyleDisplay* disp = StyleDisplay();
-      if ((!IsThemed(disp) ||
-           presContext->Theme()->ThemeWantsButtonInnerFocusRing(
-               disp->mAppearance)) &&
+      if ((!IsThemed(disp) || !presContext->Theme()->ThemeDrawsFocusForWidget(
+                                  disp->mAppearance)) &&
           mDisplayFrame && IsVisibleForPainting()) {
         aLists.Content()->AppendNewToTop<nsDisplayComboboxFocus>(aBuilder,
                                                                  this);
       }
     }
   }
 
   DisplaySelectionOverlay(aBuilder, aLists.Content());
--- a/layout/forms/nsRangeFrame.cpp
+++ b/layout/forms/nsRangeFrame.cpp
@@ -256,21 +256,18 @@ void nsRangeFrame::BuildDisplayList(nsDi
   }
 
   if (!mOuterFocusStyle || !mOuterFocusStyle->StyleBorder()->HasBorder()) {
     // no ::-moz-focus-outer specified border (how style specifies a focus ring
     // for range)
     return;
   }
 
-  // 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)) {
+  if (IsThemed(disp) &&
+      PresContext()->Theme()->ThemeDrawsFocusForWidget(disp->mAppearance)) {
     return;  // the native theme displays its own visual indication of focus
   }
 
   aLists.Content()->AppendNewToTop<nsDisplayRangeFocusRing>(aBuilder, this);
 }
 
 void nsRangeFrame::Reflow(nsPresContext* aPresContext,
                           ReflowOutput& aDesiredSize,
--- a/layout/generic/nsFrame.cpp
+++ b/layout/generic/nsFrame.cpp
@@ -2620,28 +2620,22 @@ void nsFrame::DisplayOutlineUnconditiona
   if (HasAnyStateBits(NS_FRAME_PART_OF_IBSPLIT) &&
       GetScrollableOverflowRect().IsEmpty()) {
     // Skip parts of IB-splits with an empty overflow rect, see bug 434301.
     // We may still want to fix some of the overflow area calculations over in
     // that bug.
     return;
   }
 
-  // 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;
-    }
+  // 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()) {
+    return;
   }
 
   aLists.Outlines()->AppendNewToTop<nsDisplayOutline>(aBuilder, this);
 }
 
 void nsFrame::DisplayOutline(nsDisplayListBuilder* aBuilder,
                              const nsDisplayListSet& aLists) {
   if (!IsVisibleForPainting()) return;
--- a/layout/style/res/forms.css
+++ b/layout/style/res/forms.css
@@ -465,16 +465,20 @@ input[type="image"] {
   font-size: small;
   cursor: pointer;
 }
 
 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"] {
   white-space: nowrap !important;
   overflow: hidden !important;
   overflow-clip-box: padding-box;
   color: unset;
 
@@ -553,25 +557,25 @@ input[type="radio"]:disabled:hover,
 input[type="radio"]:disabled:hover:active,
 input[type="checkbox"]:disabled,
 input[type="checkbox"]:disabled:active,
 input[type="checkbox"]:disabled:hover,
 input[type="checkbox"]:disabled:hover:active {
   cursor: unset;
 }
 
-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;
+% 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;
 }
+%endif
 
 input[type="search"] {
   box-sizing: border-box;
 }
 
 /* buttons */
 
 /* Note: Values in nsNativeTheme IsWidgetStyled function
--- a/layout/style/res/html.css
+++ b/layout/style/res/html.css
@@ -721,16 +721,26 @@ canvas {
 iframe:-moz-focusring,
 body:-moz-focusring,
 html:-moz-focusring {
   /* These elements historically don't show outlines when focused by default.
    * We could consider changing that if needed. */
   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 {
    display: none;
 }
 
 area {
   /* Don't give it frames other than its imageframe */
--- a/widget/cocoa/nsNativeThemeCocoa.mm
+++ b/widget/cocoa/nsNativeThemeCocoa.mm
@@ -4199,34 +4199,25 @@ bool nsNativeThemeCocoa::WidgetIsContain
       return false;
     default:
       break;
   }
   return true;
 }
 
 bool nsNativeThemeCocoa::ThemeDrawsFocusForWidget(StyleAppearance aAppearance) {
-  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;
-  }
+  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)
+    return true;
+
+  return false;
 }
 
 bool nsNativeThemeCocoa::ThemeNeedsComboboxDropmarker() { return false; }
 
 bool nsNativeThemeCocoa::WidgetAppearanceDependsOnWindowFocus(StyleAppearance aAppearance) {
   switch (aAppearance) {
     case StyleAppearance::Dialog:
     case StyleAppearance::Groupbox:
--- a/widget/gtk/nsNativeThemeGTK.cpp
+++ b/widget/gtk/nsNativeThemeGTK.cpp
@@ -1967,29 +1967,23 @@ nsNativeThemeGTK::WidgetIsContainer(Styl
       aAppearance == StyleAppearance::ButtonArrowDown ||
       aAppearance == StyleAppearance::ButtonArrowNext ||
       aAppearance == StyleAppearance::ButtonArrowPrevious)
     return false;
   return true;
 }
 
 bool nsNativeThemeGTK::ThemeDrawsFocusForWidget(StyleAppearance aAppearance) {
-  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;
-  }
+  if (aAppearance == StyleAppearance::Menulist ||
+      aAppearance == StyleAppearance::MenulistButton ||
+      aAppearance == StyleAppearance::Button ||
+      aAppearance == StyleAppearance::Treeheadercell)
+    return true;
+
+  return false;
 }
 
 bool nsNativeThemeGTK::ThemeNeedsComboboxDropmarker() { return false; }
 
 nsITheme::Transparency nsNativeThemeGTK::GetWidgetTransparency(
     nsIFrame* aFrame, StyleAppearance aAppearance) {
   switch (aAppearance) {
     // These widgets always draw a default background.
--- a/widget/nsNativeBasicTheme.cpp
+++ b/widget/nsNativeBasicTheme.cpp
@@ -905,25 +905,17 @@ bool nsNativeBasicTheme::WidgetIsContain
     case StyleAppearance::Checkbox:
       return false;
     default:
       return true;
   }
 }
 
 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;
-  }
+  return true;
 }
 
 bool nsNativeBasicTheme::ThemeNeedsComboboxDropmarker() { return true; }
 
 already_AddRefed<nsITheme> do_GetBasicNativeThemeDoNotUseDirectly() {
   static StaticRefPtr<nsITheme> gInstance;
   if (MOZ_UNLIKELY(!gInstance)) {
     gInstance = new nsNativeBasicTheme();
--- a/widget/windows/nsNativeThemeWin.cpp
+++ b/widget/windows/nsNativeThemeWin.cpp
@@ -2596,27 +2596,17 @@ bool nsNativeThemeWin::WidgetIsContainer
   if (aAppearance == StyleAppearance::MozMenulistArrowButton ||
       aAppearance == StyleAppearance::Radio ||
       aAppearance == StyleAppearance::Checkbox)
     return false;
   return true;
 }
 
 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;
-  }
+  return false;
 }
 
 bool nsNativeThemeWin::ThemeNeedsComboboxDropmarker() { return true; }
 
 bool nsNativeThemeWin::WidgetAppearanceDependsOnWindowFocus(
     StyleAppearance aAppearance) {
   switch (aAppearance) {
     case StyleAppearance::MozWindowTitlebar:
--- a/widget/windows/nsNativeThemeWin.h
+++ b/widget/windows/nsNativeThemeWin.h
@@ -63,18 +63,16 @@ class nsNativeThemeWin : private nsNativ
 
   bool ThemeSupportsWidget(nsPresContext* aPresContext, nsIFrame* aFrame,
                            StyleAppearance aAppearance) override;
 
   bool WidgetIsContainer(StyleAppearance aAppearance) override;
 
   bool ThemeDrawsFocusForWidget(StyleAppearance aAppearance) override;
 
-  bool ThemeWantsButtonInnerFocusRing(StyleAppearance) override { return true; }
-
   bool ThemeNeedsComboboxDropmarker() override;
 
   virtual bool WidgetAppearanceDependsOnWindowFocus(
       StyleAppearance aAppearance) override;
 
   enum { eThemeGeometryTypeWindowButtons = eThemeGeometryTypeUnknown + 1 };
   virtual ThemeGeometryType ThemeGeometryTypeForWidget(
       nsIFrame* aFrame, StyleAppearance aAppearance) override;