Bug 1638821 - Make the decision to whether paint focus rings depend on the theme, not the platform. r=spohl
authorEmilio Cobos Álvarez <emilio@crisal.io>
Tue, 19 May 2020 15:46:42 +0000
changeset 530881 b8259db1782b52d2ff53e131b3e531a90927852c
parent 530880 1d13a772cee19410c9d40204b175688552395da5
child 530882 26ed03b47566a439290598ca635c0d25e513062a
push id37433
push userdluca@mozilla.com
push dateWed, 20 May 2020 03:39:31 +0000
treeherdermozilla-central@855249e545c3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersspohl
bugs1638821
milestone78.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 1638821 - Make the decision to whether paint focus rings depend on the theme, not the platform. r=spohl The current code assumes that nsNativeTheme is in use which breaks with the non-native theme. Instead of hackily remove the FOCUS bit, make the theme check for the FOCUSRING bit, which is the right thing to check for anyway. Differential Revision: https://phabricator.services.mozilla.com/D75782
widget/cocoa/nsNativeThemeCocoa.mm
widget/nsNativeTheme.cpp
widget/windows/nsNativeThemeWin.cpp
--- a/widget/cocoa/nsNativeThemeCocoa.mm
+++ b/widget/cocoa/nsNativeThemeCocoa.mm
@@ -1215,23 +1215,40 @@ void nsNativeThemeCocoa::DrawMenuSeparat
   } else {
     menuState = aParams.selected ? kThemeMenuSelected : kThemeMenuActive;
   }
 
   HIThemeMenuItemDrawInfo midi = {0, kThemeMenuItemPlain, menuState};
   HIThemeDrawMenuSeparator(&inBoxRect, &inBoxRect, &midi, cgContext, HITHEME_ORIENTATION);
 }
 
+static bool ShouldUnconditionallyDrawFocusRingIfFocused(nsIFrame* aFrame) {
+  // Mac always draws focus rings for textboxes and lists.
+  switch (aFrame->StyleDisplay()->mAppearance) {
+    case StyleAppearance::MenulistTextfield:
+    case StyleAppearance::NumberInput:
+    case StyleAppearance::Textfield:
+    case StyleAppearance::Textarea:
+    case StyleAppearance::Searchfield:
+    case StyleAppearance::Listbox:
+      return true;
+    default:
+      return false;
+  }
+}
+
 nsNativeThemeCocoa::ControlParams nsNativeThemeCocoa::ComputeControlParams(
     nsIFrame* aFrame, EventStates aEventState) {
   ControlParams params;
   params.disabled = IsDisabled(aFrame, aEventState);
   params.insideActiveWindow = FrameIsInActiveWindow(aFrame);
   params.pressed = aEventState.HasAllStates(NS_EVENT_STATE_ACTIVE | NS_EVENT_STATE_HOVER);
-  params.focused = aEventState.HasState(NS_EVENT_STATE_FOCUS);
+  params.focused = aEventState.HasState(NS_EVENT_STATE_FOCUS) &&
+                   (aEventState.HasState(NS_EVENT_STATE_FOCUSRING) ||
+                    ShouldUnconditionallyDrawFocusRingIfFocused(aFrame));
   params.rtl = IsFrameRTL(aFrame);
   return params;
 }
 
 static const NSSize kHelpButtonSize = NSMakeSize(20, 20);
 static const NSSize kDisclosureButtonSize = NSMakeSize(21, 21);
 
 static const CellRenderSettings pushButtonSettings = {{
@@ -2030,17 +2047,17 @@ nsNativeThemeCocoa::ScaleParams nsNative
     params.max = 100;
   }
 
   params.reverse =
       aFrame->GetContent()->IsElement() &&
       aFrame->GetContent()->AsElement()->AttrValueIs(kNameSpaceID_None, nsGkAtoms::dir,
                                                      NS_LITERAL_STRING("reverse"), eCaseMatters);
   params.insideActiveWindow = FrameIsInActiveWindow(aFrame);
-  params.focused = aEventState.HasState(NS_EVENT_STATE_FOCUS);
+  params.focused = aEventState.HasState(NS_EVENT_STATE_FOCUSRING);
   params.disabled = IsDisabled(aFrame, aEventState);
   params.horizontal = aIsHorizontal;
   return params;
 }
 
 Maybe<nsNativeThemeCocoa::ScaleParams> nsNativeThemeCocoa::ComputeHTMLScaleParams(
     nsIFrame* aFrame, EventStates aEventState) {
   nsRangeFrame* rangeFrame = do_QueryFrame(aFrame);
@@ -2053,17 +2070,17 @@ Maybe<nsNativeThemeCocoa::ScaleParams> n
   // ScaleParams requires integer min, max and value. This is purely for
   // drawing, so we normalize to a range 0-1000 here.
   ScaleParams params;
   params.value = int32_t(rangeFrame->GetValueAsFractionOfRange() * 1000);
   params.min = 0;
   params.max = 1000;
   params.reverse = !isHorizontal || rangeFrame->IsRightToLeft();
   params.insideActiveWindow = FrameIsInActiveWindow(aFrame);
-  params.focused = aEventState.HasState(NS_EVENT_STATE_FOCUS);
+  params.focused = aEventState.HasState(NS_EVENT_STATE_FOCUSRING);
   params.disabled = IsDisabled(aFrame, aEventState);
   params.horizontal = isHorizontal;
   return Some(params);
 }
 
 void nsNativeThemeCocoa::DrawScale(CGContextRef cgContext, const HIRect& inBoxRect,
                                    const ScaleParams& aParams) {
   NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
@@ -2152,17 +2169,17 @@ static const SegmentedControlRenderSetti
 
 nsNativeThemeCocoa::SegmentParams nsNativeThemeCocoa::ComputeSegmentParams(
     nsIFrame* aFrame, EventStates aEventState, SegmentType aSegmentType) {
   SegmentParams params;
   params.segmentType = aSegmentType;
   params.insideActiveWindow = FrameIsInActiveWindow(aFrame);
   params.pressed = IsPressedButton(aFrame);
   params.selected = IsSelectedButton(aFrame);
-  params.focused = aEventState.HasState(NS_EVENT_STATE_FOCUS);
+  params.focused = aEventState.HasState(NS_EVENT_STATE_FOCUSRING);
   bool isRTL = IsFrameRTL(aFrame);
   nsIFrame* left = GetAdjacentSiblingFrameWithSameAppearance(aFrame, isRTL);
   nsIFrame* right = GetAdjacentSiblingFrameWithSameAppearance(aFrame, !isRTL);
   params.atLeftEnd = !left;
   params.atRightEnd = !right;
   params.drawsLeftSeparator = SeparatorResponsibility(left, aFrame) == aFrame;
   params.drawsRightSeparator = SeparatorResponsibility(aFrame, right) == aFrame;
   params.rtl = isRTL;
@@ -2915,16 +2932,17 @@ Maybe<nsNativeThemeCocoa::WidgetInfo> ns
           ButtonParams{ComputeControlParams(aFrame, eventState), ButtonType::eArrowButton}));
 
     case StyleAppearance::Groupbox:
       return Some(WidgetInfo::GroupBox());
 
     case StyleAppearance::MenulistTextfield:
     case StyleAppearance::Textfield:
     case StyleAppearance::NumberInput: {
+      // See ShouldUnconditionallyDrawFocusRingIfFocused.
       bool isFocused = eventState.HasState(NS_EVENT_STATE_FOCUS);
       // XUL textboxes set the native appearance on the containing box, while
       // concrete focus is set on the html:input element within it. We can
       // though, check the focused attribute of xul textboxes in this case.
       // On Mac, focus rings are always shown for textboxes, so we do not need
       // to check the window's focus ring state here
       if (aFrame->GetContent()->IsXULElement() && IsFocused(aFrame)) {
         isFocused = true;
--- a/widget/nsNativeTheme.cpp
+++ b/widget/nsNativeTheme.cpp
@@ -57,50 +57,26 @@ EventStates nsNativeTheme::GetContentSta
         nsNumberControlFrame::GetNumberControlFrameForSpinButton(aFrame);
     if (numberControlFrame &&
         numberControlFrame->GetContent()->AsElement()->State().HasState(
             NS_EVENT_STATE_DISABLED)) {
       flags |= NS_EVENT_STATE_DISABLED;
     }
   }
 
-  if (isXULCheckboxRadio && aAppearance == StyleAppearance::Radio) {
-    if (IsFocused(aFrame)) {
-      flags |= NS_EVENT_STATE_FOCUS;
-      nsPIDOMWindowOuter* window =
-          aFrame->GetContent()->OwnerDoc()->GetWindow();
-      if (window && window->ShouldShowFocusRing()) {
-        flags |= NS_EVENT_STATE_FOCUSRING;
-      }
+  if (isXULCheckboxRadio && aAppearance == StyleAppearance::Radio &&
+      IsFocused(aFrame)) {
+    flags |= NS_EVENT_STATE_FOCUS;
+    nsPIDOMWindowOuter* window =
+        aFrame->GetContent()->OwnerDoc()->GetWindow();
+    if (window && window->ShouldShowFocusRing()) {
+      flags |= NS_EVENT_STATE_FOCUSRING;
     }
   }
 
-  // On Windows and Mac, only draw focus rings if they should be shown. This
-  // means that focus rings are only shown once the keyboard has been used to
-  // focus something in the window.
-#if defined(XP_MACOSX)
-  // Mac always draws focus rings for textboxes and lists.
-  if (aAppearance == StyleAppearance::MenulistTextfield ||
-      aAppearance == StyleAppearance::NumberInput ||
-      aAppearance == StyleAppearance::Textfield ||
-      aAppearance == StyleAppearance::Textarea ||
-      aAppearance == StyleAppearance::Searchfield ||
-      aAppearance == StyleAppearance::Listbox) {
-    return flags;
-  }
-#endif
-#if defined(XP_WIN)
-  // On Windows, focused buttons are always drawn as such by the native theme.
-  if (aAppearance == StyleAppearance::Button) return flags;
-#endif
-#if defined(XP_MACOSX) || defined(XP_WIN)
-  if (!flags.HasState(NS_EVENT_STATE_FOCUSRING)) {
-    flags &= ~NS_EVENT_STATE_FOCUS;
-  }
-#endif
   return flags;
 }
 
 /* static */
 bool nsNativeTheme::CheckBooleanAttr(nsIFrame* aFrame, nsAtom* aAtom) {
   if (!aFrame) return false;
 
   nsIContent* content = aFrame->GetContent();
--- a/widget/windows/nsNativeThemeWin.cpp
+++ b/widget/windows/nsNativeThemeWin.cpp
@@ -798,21 +798,34 @@ nsNativeThemeWin::GetTheme(StyleAppearan
   }
   return nsUXThemeData::GetTheme(themeClass.value());
 }
 
 int32_t nsNativeThemeWin::StandardGetState(nsIFrame* aFrame,
                                            StyleAppearance aAppearance,
                                            bool wantFocused) {
   EventStates eventState = GetContentState(aFrame, aAppearance);
-  if (eventState.HasAllStates(NS_EVENT_STATE_HOVER | NS_EVENT_STATE_ACTIVE))
+  if (eventState.HasAllStates(NS_EVENT_STATE_HOVER | NS_EVENT_STATE_ACTIVE)) {
     return TS_ACTIVE;
-  if (eventState.HasState(NS_EVENT_STATE_HOVER)) return TS_HOVER;
-  if (wantFocused && eventState.HasState(NS_EVENT_STATE_FOCUS))
-    return TS_FOCUSED;
+  }
+  if (eventState.HasState(NS_EVENT_STATE_HOVER)) {
+    return TS_HOVER;
+  }
+  if (wantFocused) {
+    if (eventState.HasState(NS_EVENT_STATE_FOCUSRING)) {
+      return TS_FOCUSED;
+    }
+    // On Windows, focused buttons are always drawn as such by the native
+    // theme, that's why we check NS_EVENT_STATE_FOCUS instead of
+    // NS_EVENT_STATE_FOCUSRING.
+    if (aAppearance == StyleAppearance::Button &&
+        eventState.HasState(NS_EVENT_STATE_FOCUS)) {
+      return TS_FOCUSED;
+    }
+  }
 
   return TS_NORMAL;
 }
 
 bool nsNativeThemeWin::IsMenuActive(nsIFrame* aFrame,
                                     StyleAppearance aAppearance) {
   nsIContent* content = aFrame->GetContent();
   if (content->IsXULElement() &&
@@ -919,17 +932,17 @@ nsresult nsNativeThemeWin::GetThemePartA
         nsIContent* content = aFrame->GetContent();
 
         /* XUL textboxes don't get focused themselves, because they have child
          * html:input.. but we can check the XUL focused attributes on them
          */
         if (content && content->IsXULElement() && IsFocused(aFrame))
           aState = TFS_EDITBORDER_FOCUSED;
         else if (eventState.HasAtLeastOneOfStates(NS_EVENT_STATE_ACTIVE |
-                                                  NS_EVENT_STATE_FOCUS))
+                                                  NS_EVENT_STATE_FOCUSRING))
           aState = TFS_EDITBORDER_FOCUSED;
         else if (eventState.HasState(NS_EVENT_STATE_HOVER))
           aState = TFS_EDITBORDER_HOVER;
         else
           aState = TFS_EDITBORDER_NORMAL;
       }
 
       return NS_OK;
@@ -1101,17 +1114,17 @@ nsresult nsNativeThemeWin::GetThemePartA
         aState = TKP_DISABLED;
       } else {
         if (eventState.HasState(
                 NS_EVENT_STATE_ACTIVE))  // Hover is not also a requirement for
                                          // the thumb, since the drag is not
                                          // canceled when you move outside the
                                          // thumb.
           aState = TS_ACTIVE;
-        else if (eventState.HasState(NS_EVENT_STATE_FOCUS))
+        else if (eventState.HasState(NS_EVENT_STATE_FOCUSRING))
           aState = TKP_FOCUSED;
         else if (eventState.HasState(NS_EVENT_STATE_HOVER))
           aState = TS_HOVER;
         else
           aState = TS_NORMAL;
       }
       return NS_OK;
     }
@@ -1247,18 +1260,18 @@ nsresult nsNativeThemeWin::GetThemePartA
 
       if (IsDisabled(aFrame, eventState)) {
         aState = TS_DISABLED;
       } else if (IsReadOnly(aFrame)) {
         aState = TS_NORMAL;
       } else if (IsOpenButton(aFrame)) {
         aState = TS_ACTIVE;
       } else {
-        if (useDropBorder &&
-            (eventState.HasState(NS_EVENT_STATE_FOCUS) || IsFocused(aFrame)))
+        if (useDropBorder && (eventState.HasState(NS_EVENT_STATE_FOCUSRING) ||
+                              IsFocused(aFrame)))
           aState = TS_ACTIVE;
         else if (eventState.HasAllStates(NS_EVENT_STATE_HOVER |
                                          NS_EVENT_STATE_ACTIVE))
           aState = TS_ACTIVE;
         else if (eventState.HasState(NS_EVENT_STATE_HOVER))
           aState = TS_HOVER;
         else
           aState = TS_NORMAL;
@@ -1877,17 +1890,17 @@ RENDER_AGAIN:
 
   // Draw focus rectangles for range and scale elements
   // XXX it'd be nice to draw these outside of the frame
   if (aAppearance == StyleAppearance::Range ||
       aAppearance == StyleAppearance::ScaleHorizontal ||
       aAppearance == StyleAppearance::ScaleVertical) {
     EventStates contentState = GetContentState(aFrame, aAppearance);
 
-    if (contentState.HasState(NS_EVENT_STATE_FOCUS)) {
+    if (contentState.HasState(NS_EVENT_STATE_FOCUSRING)) {
       POINT vpOrg;
       HPEN hPen = nullptr;
 
       uint8_t id = SaveDC(hdc);
 
       ::SelectClipRgn(hdc, nullptr);
       ::GetViewportOrgEx(hdc, &vpOrg);
       ::SetBrushOrgEx(hdc, vpOrg.x + widgetRect.left, vpOrg.y + widgetRect.top,
@@ -3114,16 +3127,19 @@ nsresult nsNativeThemeWin::ClassicGetThe
           const nsStyleUI* uiData = aFrame->StyleUI();
           // The down state is flat if the button is focusable
           if (uiData->mUserFocus == StyleUserFocus::Normal) {
             if (!aFrame->GetContent()->IsHTMLElement()) aState |= DFCS_FLAT;
 
             aFocused = true;
           }
         }
+        // On Windows, focused buttons are always drawn as such by the native
+        // theme, that's why we check NS_EVENT_STATE_FOCUS instead of
+        // NS_EVENT_STATE_FOCUSRING.
         if (contentState.HasState(NS_EVENT_STATE_FOCUS) ||
             (aState == DFCS_BUTTONPUSH && IsDefaultButton(aFrame))) {
           aFocused = true;
         }
       }
 
       return NS_OK;
     }
@@ -3150,17 +3166,17 @@ nsresult nsNativeThemeWin::ClassicGetThe
         aState = DFCS_BUTTONRADIO;
       }
       if (isChecked) {
         aState |= DFCS_CHECKED;
       }
 
       contentState = GetContentState(aFrame, aAppearance);
       if (!content->IsXULElement() &&
-          contentState.HasState(NS_EVENT_STATE_FOCUS)) {
+          contentState.HasState(NS_EVENT_STATE_FOCUSRING)) {
         aFocused = true;
       }
 
       if (IsDisabled(aFrame, contentState)) {
         aState |= DFCS_INACTIVE;
       } else if (contentState.HasAllStates(NS_EVENT_STATE_ACTIVE |
                                            NS_EVENT_STATE_HOVER)) {
         aState |= DFCS_PUSHED;