Making the temporary focus highlight match UX mocks. See bug for screenshots.

AX-Relnotes: N/A
Bug: 1021939
Change-Id: I051ee8ff4deb08977a5319783bae5aced602d54e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2256942
Commit-Queue: Alice Boxhall <aboxhall@chromium.org>
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Reviewed-by: Chris Hall <chrishall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#781731}

chrome/browser/ui/views/accessibility/accessibility_focus_highlight.cc

@@ -25,42 +25,58 @@ class Compositor;
 
 namespace {
 
+// The number of pixels of padding between the outer edge of the focused
+// element's bounding box and the inner edge of the inner focus ring.
+constexpr int kPaddingDIPs = 8;
+
+// The size of the border radius of the innermost focus highlight ring.
+constexpr int kBorderRadiusDIPs = 4;
+
+// The stroke width, in DIPs, of the innermost focus ring, and each line drawn
+// as part of the focus ring gradient effect.
+constexpr int kStrokeWidthDIPs = 2;
+
+// The thickness, in DIPs, of the outer focus ring gradient.
+constexpr int kGradientWidthDIPs = 9;
+
 // The padding between the bounds of the layer and the bounds of the
 // drawn focus ring, in DIPs. If it's zero the focus ring might be
 // clipped.
 constexpr int kLayerPaddingDIPs = 2;
 
-// The number of pixels the focus ring is outset from the object it outlines,
-// which also determines the border radius of the rounded corners.
-constexpr int kAccessibilityFocusHighlightMarginDIPs = 7;
-
-// The stroke width, in DIPs, of each line drawn as part of the focus ring
-// gradient effect.
-constexpr int kStrokeWidthDIPs = 2;
-
-// The thickness, in DIPs, of the focus ring gradient.
-constexpr int kGradientWidthDIPs = 3;
+// Total DIPs between the edge of the node and the edge of the layer.
+constexpr int kTotalLayerPaddingDIPs =
+    kPaddingDIPs + kStrokeWidthDIPs + kGradientWidthDIPs + kLayerPaddingDIPs;
 
 // The amount of time it should take for the highlight to fade in.
 constexpr int kFadeInTimeMilliseconds = 100;
 
+// The amount of time the highlight should persist before beginning to fade.
+constexpr int kHighlightPersistTimeMilliseconds = 1000;
+
 // The amount of time it should take for the highlight to fade out.
-constexpr int kFadeOutTimeMilliseconds = 1600;
+constexpr int kFadeOutTimeMilliseconds = 600;
 
 }  // namespace
 
 // static
-SkColor AccessibilityFocusHighlight::color_;
+SkColor AccessibilityFocusHighlight::default_color_;
 
 // static
 base::TimeDelta AccessibilityFocusHighlight::fade_in_time_;
 
+// static
+base::TimeDelta AccessibilityFocusHighlight::persist_time_;
+
 // static
 base::TimeDelta AccessibilityFocusHighlight::fade_out_time_;
 
 // static
 bool AccessibilityFocusHighlight::skip_activation_check_for_testing_ = false;
 
+// static
+bool AccessibilityFocusHighlight::use_default_color_for_testing_ = false;
+
 AccessibilityFocusHighlight::AccessibilityFocusHighlight(
     BrowserView* browser_view)
     : browser_view_(browser_view),
@@ -82,9 +98,11 @@ AccessibilityFocusHighlight::AccessibilityFocusHighlight(
   // One-time initialization of statics the first time an instance is created.
   if (fade_in_time_.is_zero()) {
     fade_in_time_ = base::TimeDelta::FromMilliseconds(kFadeInTimeMilliseconds);
+    persist_time_ =
+        base::TimeDelta::FromMilliseconds(kHighlightPersistTimeMilliseconds);
     fade_out_time_ =
         base::TimeDelta::FromMilliseconds(kFadeOutTimeMilliseconds);
-    color_ = SkColorSetRGB(247, 152, 58);
+    default_color_ = SkColorSetRGB(16, 16, 16);  // #101010
   }
 }
 
@@ -96,7 +114,8 @@ AccessibilityFocusHighlight::~AccessibilityFocusHighlight() {
 // static
 void AccessibilityFocusHighlight::SetNoFadeForTesting() {
   fade_in_time_ = base::TimeDelta();
-  fade_out_time_ = base::TimeDelta::FromHours(1);
+  persist_time_ = base::TimeDelta::FromHours(1);
+  fade_out_time_ = base::TimeDelta();
 }
 
 // static
@@ -105,11 +124,21 @@ void AccessibilityFocusHighlight::SkipActivationCheckForTesting() {
 }
 
 // static
-SkColor AccessibilityFocusHighlight::GetHighlightColorForTesting() {
-  return color_;
+void AccessibilityFocusHighlight::UseDefaultColorForTesting() {
+  use_default_color_for_testing_ = true;
+}
+
+SkColor AccessibilityFocusHighlight::GetHighlightColor() {
+  SkColor theme_color = browser_view_->GetNativeTheme()->GetSystemColor(
+      ui::NativeTheme::kColorId_FocusedBorderColor);
+
+  if (theme_color == SK_ColorTRANSPARENT || use_default_color_for_testing_)
+    return default_color_;
+
+  return theme_color;
 }
 
-void AccessibilityFocusHighlight::CreateOrUpdateLayer() {
+void AccessibilityFocusHighlight::CreateOrUpdateLayer(gfx::Rect node_bounds) {
   // Find the layer of our owning BrowserView.
   views::Widget* widget = browser_view_->GetWidget();
   DCHECK(widget);
@@ -132,18 +161,30 @@ void AccessibilityFocusHighlight::CreateOrUpdateLayer() {
   layer_->parent()->StackAtTop(layer_.get());
 
   // Update the bounds.
-  layer_->SetBounds(bounds_);
+  // Outset the bounds of the layer by the total width of the focus highlight,
+  // plus the extra padding to ensure the highlight isn't clipped.
+  gfx::Rect layer_bounds = node_bounds;
+  int padding = kTotalLayerPaddingDIPs * device_scale_factor_;
+  layer_bounds.Inset(-padding, -padding);
+
+  layer_->SetBounds(layer_bounds);
+
+  // Set node_bounds_ and make their position relative to the layer, instead of
+  // the page.
+  node_bounds_ = node_bounds;
+  node_bounds_.set_x(padding);
+  node_bounds_.set_y(padding);
 
   // Update the timestamp of the last time the layer changed.
   focus_last_changed_time_ = base::TimeTicks::Now();
 
   // Ensure it's repainted.
-  gfx::Rect layer_bounds(0, 0, bounds_.width(), bounds_.height());
-  layer_->SchedulePaint(layer_bounds);
+  gfx::Rect bounds(0, 0, layer_bounds.width(), layer_bounds.height());
+  layer_->SchedulePaint(bounds);
 
   // Schedule the animation observer, or update it if needed.
   display::Display display =
-      display::Screen::GetScreen()->GetDisplayMatching(bounds_);
+      display::Screen::GetScreen()->GetDisplayMatching(layer_bounds);
   ui::Compositor* compositor = root_layer->GetCompositor();
   if (compositor != compositor_) {
     if (compositor_ && compositor_->HasAnimationObserver(this))
@@ -199,58 +240,74 @@ void AccessibilityFocusHighlight::Observe(
   // given to us in screen DIPs.
   content::FocusedNodeDetails* node_details =
       content::Details<content::FocusedNodeDetails>(details).ptr();
-  bounds_ = node_details->node_bounds_in_screen;
+  gfx::Rect node_bounds = node_details->node_bounds_in_screen;
 
   // Convert it to the local coordinates of this BrowserView's widget.
-  bounds_.Offset(-gfx::ToFlooredVector2d(browser_view_->GetWidget()
-                                             ->GetClientAreaBoundsInScreen()
-                                             .OffsetFromOrigin()));
-
-  // Outset the bounds by the margin of the focus ring plus the layer padding.
-  int outset =
-      int{(kAccessibilityFocusHighlightMarginDIPs + kLayerPaddingDIPs) *
-          device_scale_factor_};
-  bounds_.Inset(-outset, -outset);
+  node_bounds.Offset(-gfx::ToFlooredVector2d(browser_view_->GetWidget()
+                                                 ->GetClientAreaBoundsInScreen()
+                                                 .OffsetFromOrigin()));
 
   // Create the layer if needed, and move/resize it.
-  CreateOrUpdateLayer();
+  CreateOrUpdateLayer(node_bounds);
 }
 
 void AccessibilityFocusHighlight::OnPaintLayer(
     const ui::PaintContext& context) {
   ui::PaintRecorder recorder(context, layer_->size());
-
+  SkColor highlight_color = GetHighlightColor();
   cc::PaintFlags flags;
   flags.setAntiAlias(true);
   flags.setStyle(cc::PaintFlags::kStroke_Style);
+  flags.setColor(highlight_color);
 
-  float dsf = device_scale_factor_;
+  gfx::RectF bounds(node_bounds_);
 
-  flags.setStrokeWidth(kStrokeWidthDIPs * dsf);
+  // Draw gradient first, so other lines will be drawn over the top.
+  // Apply padding
+  int padding = kPaddingDIPs * device_scale_factor_;
+  bounds.Inset(-padding, -padding);
 
-  int padding = int{kLayerPaddingDIPs * dsf};
-  int width = bounds_.width() - 2 * padding;
-  int height = bounds_.height() - 2 * padding;
-  int margin = int{kAccessibilityFocusHighlightMarginDIPs * dsf};
-
-  // Translate the coordinate space so that we can draw the rounded
-  // rect at (0, 0) and not need to take the padding into account.
-  recorder.canvas()->Translate(gfx::Vector2d(padding, padding));
+  gfx::RectF gradient_bounds(bounds);
+  int border_radius = kBorderRadiusDIPs * device_scale_factor_;
+  int gradient_border_radius = border_radius;
 
   // Create a gradient effect by drawing the path outline multiple
   // times with increasing insets from 0 to kGradientWidthDIPs, and
   // with increasing transparency.
-  int w = int{kGradientWidthDIPs * dsf};
-  for (int i = 0; i < w; ++i) {
-    // Distance remaining within border gradient.
-    int dist = w - i;
+  int gradient_width = kGradientWidthDIPs * device_scale_factor_;
+  int stroke_width = kStrokeWidthDIPs * device_scale_factor_;
+  flags.setStrokeWidth(stroke_width);
+  int original_alpha = std::min(SkColorGetA(highlight_color), 192u);
+  for (int remaining = gradient_width; remaining > 0; remaining -= 1) {
     // Decrease alpha as distance remaining decreases.
-    int alpha = 255 * dist * dist / (w * w);
-    flags.setColor(SkColorSetA(color_, alpha));
+    int alpha = (original_alpha * remaining * remaining) /
+                (gradient_width * gradient_width);
+    flags.setAlpha(alpha);
+
+    recorder.canvas()->DrawRoundRect(gradient_bounds, gradient_border_radius,
+                                     flags);
 
-    gfx::RectF rect(i, i, width - i, height - i);
-    recorder.canvas()->DrawRoundRect(rect, margin - i, flags);
+    gradient_bounds.Inset(-1, -1);
+    gradient_border_radius += 1;
   }
+
+  // Draw the white ring before the inner ring, so that the inner ring is
+  // partially over the top, rather than drawing a 1px white ring. A 1px ring
+  // would be antialiased to look semi-transparent, which is not what we want.
+
+  // Resize bounds and border radius around inner ring
+  gfx::RectF white_ring_bounds(bounds);
+  white_ring_bounds.Inset(-(stroke_width / 2), -(stroke_width / 2));
+  int white_ring_border_radius = border_radius + (stroke_width / 2);
+
+  flags.setColor(SK_ColorWHITE);
+  flags.setStrokeWidth(stroke_width);
+  recorder.canvas()->DrawRoundRect(white_ring_bounds, white_ring_border_radius,
+                                   flags);
+
+  // Draw the innermost solid ring
+  flags.setColor(highlight_color);
+  recorder.canvas()->DrawRoundRect(bounds, border_radius, flags);
 }
 
 void AccessibilityFocusHighlight::OnDeviceScaleFactorChanged(
@@ -283,26 +340,29 @@ void AccessibilityFocusHighlight::OnAnimationStep(base::TimeTicks timestamp) {
 
   // If the fade out has completed, remove the layer and remove the
   // animation observer.
-  if (time_since_focus_move > fade_out_time_) {
+  if (time_since_focus_move > persist_time_ + fade_out_time_) {
     RemoveLayer();
     return;
   }
 
   // Compute the opacity based on the fade in and fade out times.
-  float opacity;
+  // TODO(aboxhall): figure out how to use cubic beziers
+  float opacity = 1.0f;
   if (time_since_layer_create < fade_in_time_) {
     // We're fading in.
     opacity = time_since_layer_create.InSecondsF() / fade_in_time_.InSecondsF();
-  } else {
-    // Fading out. Add fade_in_time_ and fade_out_time_ because we don't
-    // want to start the fade out until after the fade in has finished.
-    opacity = 1.0f - (time_since_focus_move.InSecondsF() /
-                      fade_out_time_.InSecondsF());
+  } else if (time_since_focus_move > persist_time_) {
+    // Fading out.
+    float time_since_began_fading =
+        time_since_focus_move.InSecondsF() -
+        (fade_in_time_.InSecondsF() + persist_time_.InSecondsF());
+    float fade_out_time_float = fade_out_time_.InSecondsF();
+
+    opacity = 1.0f - (time_since_began_fading / fade_out_time_float);
   }
 
   // Layer::SetOpacity will throw an error if we're not within 0...1.
   opacity = base::ClampToRange(opacity, 0.0f, 1.0f);
-
   layer_->SetOpacity(opacity);
 }
 

chrome/browser/ui/views/accessibility/accessibility_focus_highlight.h

@@ -45,10 +45,10 @@ class AccessibilityFocusHighlight : public ui::LayerDelegate,
   // For testing.
   static void SetNoFadeForTesting();
   static void SkipActivationCheckForTesting();
-  static SkColor GetHighlightColorForTesting();
+  static void UseDefaultColorForTesting();
 
-  // Create the layer if needed, and update its bounds to match |bounds_|.
-  void CreateOrUpdateLayer();
+  // Create the layer if needed, and set node_bounds_
+  void CreateOrUpdateLayer(gfx::Rect node_bounds);
 
   // Get rid of the layer and stop animation.
   void RemoveLayer();
@@ -70,15 +70,17 @@ class AccessibilityFocusHighlight : public ui::LayerDelegate,
   void OnAnimationStep(base::TimeTicks timestamp) override;
   void OnCompositingShuttingDown(ui::Compositor* compositor) override;
 
+  // Compute the highlight color based on theme colors and defaults.
+  SkColor GetHighlightColor();
+
   // The layer, if visible.
   std::unique_ptr<ui::Layer> layer_;
 
   // The compositor associated with this layer.
   ui::Compositor* compositor_ = nullptr;
 
-  // The bounding rectangle of the focused object, in the coordinate system
-  // of our owner BrowserView's layer.
-  gfx::Rect bounds_;
+  // The bounding rectangle of the focused object, relative to the layer.
+  gfx::Rect node_bounds_;
 
   // Owns this.
   BrowserView* browser_view_;
@@ -92,18 +94,25 @@ class AccessibilityFocusHighlight : public ui::LayerDelegate,
   // The current scale factor between DIPs and pixels.
   float device_scale_factor_;
 
-  // The color used for the highlight.
-  static SkColor color_;
+  // The default color used for the highlight.
+  static SkColor default_color_;
 
   // The amount of time it should take for the highlight to fade in.
   static base::TimeDelta fade_in_time_;
 
+  // The amount of time the highlight should persist between fading in and
+  // fading out.
+  static base::TimeDelta persist_time_;
+
   // The amount of time it should take for the highlight to fade out.
   static base::TimeDelta fade_out_time_;
 
   // If set, draws the highlight even if the widget is not active.
   static bool skip_activation_check_for_testing_;
 
+  // If set, don't check the system theme color.
+  static bool use_default_color_for_testing_;
+
   // For observing browser preference notifications.
   PrefChangeRegistrar profile_pref_registrar_;
 

chrome/browser/ui/views/accessibility/accessibility_focus_highlight_browsertest.cc

@@ -127,11 +127,12 @@ IN_PROC_BROWSER_TEST_F(AccessibilityFocusHighlightBrowserTest,
                        MAYBE_DrawsHighlight) {
   ui_test_utils::NavigateToURL(
       browser(), GURL("data:text/html,"
-                      "<body style='background-color: rgb(204, 255, 255)'>"
-                      "<input id='textfield' style='width: 100%'>"));
+                      "<body style='background-color: rgb(204, 255, 255);'>"
+                      "<div tabindex=0 id='div'>Focusable div</div>"));
 
   AccessibilityFocusHighlight::SetNoFadeForTesting();
   AccessibilityFocusHighlight::SkipActivationCheckForTesting();
+  AccessibilityFocusHighlight::UseDefaultColorForTesting();
 
   browser()->profile()->GetPrefs()->SetBoolean(
       prefs::kAccessibilityFocusHighlightEnabled, true);
@@ -145,16 +146,16 @@ IN_PROC_BROWSER_TEST_F(AccessibilityFocusHighlightBrowserTest,
   } while (CountPercentPixelsWithColor(image, SkColorSetRGB(204, 255, 255)) <
            90.0f);
 
-  // Initially less than 0.01% of the image should be the focus ring's highlight
+  SkColor highlight_color = AccessibilityFocusHighlight::default_color_;
+
+  // Initially less than 0.05% of the image should be the focus ring's highlight
   // color.
-  SkColor highlight_color =
-      AccessibilityFocusHighlight::GetHighlightColorForTesting();
-  ASSERT_LT(CountPercentPixelsWithColor(image, highlight_color), 0.01f);
+  ASSERT_LT(CountPercentPixelsWithColor(image, highlight_color), 0.05f);
 
   // Focus something.
   content::WebContents* web_contents =
       browser()->tab_strip_model()->GetActiveWebContents();
-  std::string script("document.getElementById('textfield').focus();");
+  std::string script("document.getElementById('div').focus();");
   EXPECT_TRUE(content::ExecuteScript(web_contents, script));
 
   // Now wait until at least 0.1% of the image has the focus ring's highlight