Fix magnifier jumpiness when arrowing through omnibox results

AX-Relnotes: Fixes a bug where the full screen and docked magnifiers would jump unexpectedly when using the arrow keys to move up and down through the omnibox results. Also fixes a bug where the full screen and docked magnifiers jump to the center of the omnibox entries, cutting off the left edge with the site icon and text. Now, the left edge of the selected omnibox entry is aligned with the left edge of the magnifier viewport, so that the site icon and text are visible.
Fixed: 1147350, 1147354
Change-Id: I8d2dc3515d9cd565e5cdff3ec84903a6f339eccb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2528613
Commit-Queue: Josiah Krutz <josiahk@google.com>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Reviewed-by: David Tseng <dtseng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#827159}

ash/magnifier/docked_magnifier_controller_impl.cc

@@ -5,6 +5,7 @@
 #include "ash/magnifier/docked_magnifier_controller_impl.h"
 
 #include <algorithm>
+#include <utility>
 
 #include "ash/accessibility/accessibility_controller_impl.h"
 #include "ash/host/ash_window_tree_host.h"
@@ -47,6 +48,13 @@ constexpr float kScrollScaleFactor = 0.0125f;
 constexpr char kDockedMagnifierViewportWindowName[] =
     "DockedMagnifierViewportWindow";
 
+// The duration of time to wait before moving the viewport to the caret.
+// Allows magnifier.js to preempt the viewport moving to the caret with
+// moveMagnifierToRect.
+// TODO(accessibility): Remove once caret updates handled in magnifier.js.
+constexpr base::TimeDelta kMoveMagnifierCaretDelay =
+    base::TimeDelta::FromMilliseconds(5);
+
 // Returns the current cursor location in screen coordinates.
 inline gfx::Point GetCursorScreenPoint() {
   return display::Screen::GetScreen()->GetCursorScreenPoint();
@@ -158,6 +166,27 @@ void DockedMagnifierControllerImpl::StepToNextScaleValue(int delta_index) {
       delta_index, GetScale(), kMinMagnifierScale, kMaxMagnifierScale));
 }
 
+void DockedMagnifierControllerImpl::MoveMagnifierToRect(
+    const gfx::Rect& rect_in_screen) {
+  DCHECK(GetEnabled());
+  last_move_magnifier_to_rect_ = base::TimeTicks::Now();
+
+  gfx::Point point_in_screen = rect_in_screen.CenterPoint();
+
+  // If rect is too wide to fit in viewport, include as much as we can, starting
+  // with the left edge.
+  const int scaled_viewport_width =
+      current_source_root_window_->bounds().width() / GetScale();
+  if (rect_in_screen.width() > scaled_viewport_width) {
+    point_in_screen.set_x(std::max(rect_in_screen.x() +
+                                       scaled_viewport_width / 2 -
+                                       magnifier_utils::kLeftEdgeContextPadding,
+                                   0));
+  }
+
+  CenterOnPoint(point_in_screen);
+}
+
 void DockedMagnifierControllerImpl::CenterOnPoint(
     const gfx::Point& point_in_screen) {
   if (!GetEnabled())
@@ -327,7 +356,13 @@ void DockedMagnifierControllerImpl::OnCaretBoundsChanged(
   if (!caret_screen_bounds.width() && !caret_screen_bounds.height())
     return;
 
-  CenterOnPoint(caret_screen_bounds.CenterPoint());
+  // Wait a few milliseconds to see if any move magnifier to rect activity is
+  // occurring.
+  // TODO(accessibility): Remove once we move caret following to magnifier.js.
+  last_caret_screen_point_ = caret_screen_bounds.CenterPoint();
+  move_magnifier_timer_.Start(
+      FROM_HERE, kMoveMagnifierCaretDelay, this,
+      &DockedMagnifierControllerImpl::OnMoveMagnifierTimer);
 }
 
 void DockedMagnifierControllerImpl::OnWidgetDestroying(views::Widget* widget) {
@@ -404,6 +439,11 @@ float DockedMagnifierControllerImpl::GetMinimumPointOfInterestHeightForTesting()
   return minimum_point_of_interest_height_;
 }
 
+gfx::Point DockedMagnifierControllerImpl::GetLastCaretScreenPointForTesting()
+    const {
+  return last_caret_screen_point_;
+}
+
 void DockedMagnifierControllerImpl::SwitchCurrentSourceRootWindowIfNeeded(
     aura::Window* new_root_window,
     bool update_old_root_workarea) {
@@ -731,4 +771,14 @@ void DockedMagnifierControllerImpl::ConfineMouseCursorOutsideViewport() {
       ->ConfineCursorToBoundsInRoot(confine_bounds);
 }
 
+void DockedMagnifierControllerImpl::OnMoveMagnifierTimer() {
+  // Ignore caret changes while move magnifier to rect activity is occurring.
+  if (base::TimeTicks::Now() - last_move_magnifier_to_rect_ <
+      magnifier_utils::kPauseCaretUpdateDuration) {
+    return;
+  }
+
+  CenterOnPoint(last_caret_screen_point_);
+}
+
 }  // namespace ash

ash/magnifier/docked_magnifier_controller_impl.h

@@ -78,6 +78,7 @@ class ASH_EXPORT DockedMagnifierControllerImpl
 
   // DockedMagnifierController:
   void CenterOnPoint(const gfx::Point& point_in_screen) override;
+  void MoveMagnifierToRect(const gfx::Rect& rect_in_screen) override;
   int GetMagnifierHeightForTesting() const override;
 
   // ash::SessionObserver:
@@ -130,6 +131,8 @@ class ASH_EXPORT DockedMagnifierControllerImpl
 
   float GetMinimumPointOfInterestHeightForTesting() const;
 
+  gfx::Point GetLastCaretScreenPointForTesting() const;
+
  private:
   // Switches the current source root window to |new_root_window| if it's
   // different than |current_source_root_window_|, destroys (if any) old
@@ -166,6 +169,10 @@ class ASH_EXPORT DockedMagnifierControllerImpl
   // viewport.
   void ConfineMouseCursorOutsideViewport();
 
+  // Invoked when |move_magnifier_timer_| fires to move the magnifier window to
+  // follow the caret.
+  void OnMoveMagnifierTimer();
+
   // The current root window of the source display from which we are reflecting
   // and magnifying into the viewport. It is set to |nullptr| when the magnifier
   // is disabled. The viewport is placed on the same display.
@@ -204,6 +211,16 @@ class ASH_EXPORT DockedMagnifierControllerImpl
 
   std::unique_ptr<PrefChangeRegistrar> pref_change_registrar_;
 
+  // Most recent caret position.
+  gfx::Point last_caret_screen_point_;
+
+  // Timer for moving magnifier window when it fires.
+  base::OneShotTimer move_magnifier_timer_;
+
+  // Last move magnifier to rect time - used for ignoring caret updates for a
+  // few milliseconds after the last move magnifier to rect call.
+  base::TimeTicks last_move_magnifier_to_rect_;
+
   DISALLOW_COPY_AND_ASSIGN(DockedMagnifierControllerImpl);
 };
 

ash/magnifier/docked_magnifier_controller_impl_unittest.cc

@@ -808,14 +808,18 @@ TEST_F(DockedMagnifierTest, TextInputFieldEvents) {
   // goes through the magnifier layer transform, it should end up being in the
   // center of the viewport.
   gfx::Point caret_center(text_input_helper.GetCaretBounds().CenterPoint());
-  TestMagnifierLayerTransform(caret_center, root_windows[0]);
+  gfx::Point caret_screen_point =
+      controller()->GetLastCaretScreenPointForTesting();
+  ASSERT_EQ(caret_center, caret_screen_point);
 
   // Simulate typing by pressing some keys while focus is in the text field. The
   // transformed caret center should always go to the viewport center.
   GetEventGenerator()->PressKey(ui::VKEY_A, 0);
   GetEventGenerator()->ReleaseKey(ui::VKEY_A, 0);
   gfx::Point new_caret_center(text_input_helper.GetCaretBounds().CenterPoint());
-  TestMagnifierLayerTransform(new_caret_center, root_windows[0]);
+  gfx::Point new_caret_screen_point =
+      controller()->GetLastCaretScreenPointForTesting();
+  ASSERT_EQ(new_caret_center, new_caret_screen_point);
 }
 
 // Tests that there are no crashes observed when the docked magnifier switches
@@ -843,7 +847,9 @@ TEST_F(DockedMagnifierTest, NoCrashDueToRecursion) {
   // Focus on the text input field.
   text_input_helper.FocusOnTextInputView();
   gfx::Point caret_center(text_input_helper.GetCaretBounds().CenterPoint());
-  TestMagnifierLayerTransform(caret_center, roots[0]);
+  gfx::Point caret_screen_point =
+      controller()->GetLastCaretScreenPointForTesting();
+  ASSERT_EQ(caret_center, caret_screen_point);
 
   // Move the mouse to the second display and expect no crashes.
   GetEventGenerator()->MoveMouseTo(1000, 300);

ash/magnifier/magnification_controller.cc

@@ -378,7 +378,6 @@ void MagnificationController::OnCaretBoundsChanged(
   // being moved back and forth with these two OnCaretBoundsChanged events, we
   // defer moving magnifier window until the |move_magnifier_timer_| fires,
   // when the caret settles eventually.
-  move_magnifier_timer_.Stop();
   move_magnifier_timer_.Start(
       FROM_HERE,
       base::TimeDelta::FromMilliseconds(
@@ -982,6 +981,7 @@ void MagnificationController::MoveMagnifierWindowCenterPoint(
 void MagnificationController::MoveMagnifierWindowFollowRect(
     const gfx::Rect& rect) {
   DCHECK(root_window_);
+  last_move_magnifier_to_rect_ = base::TimeTicks::Now();
   bool should_pan = false;
 
   const gfx::Rect viewport_rect = GetViewportRect();
@@ -1006,6 +1006,11 @@ void MagnificationController::MoveMagnifierWindowFollowRect(
     should_pan = true;
   }
 
+  // If rect is too wide to fit in viewport, include as much as we can, starting
+  // with the left edge.
+  if (rect.width() > viewport_rect.width())
+    x = rect.x() - magnifier_utils::kLeftEdgeContextPadding;
+
   if (should_pan) {
     if (is_on_animation_) {
       root_window_->layer()->GetAnimator()->StopAnimating();
@@ -1018,6 +1023,12 @@ void MagnificationController::MoveMagnifierWindowFollowRect(
 }
 
 void MagnificationController::OnMoveMagnifierTimer() {
+  // Ignore caret changes while move magnifier to rect activity is occurring.
+  if (base::TimeTicks::Now() - last_move_magnifier_to_rect_ <
+      magnifier_utils::kPauseCaretUpdateDuration) {
+    return;
+  }
+
   MoveMagnifierWindowCenterPoint(caret_point_);
 }
 

ash/magnifier/magnification_controller.h

@@ -322,6 +322,10 @@ class ASH_EXPORT MagnificationController : public ui::EventHandler,
   // mode.
   bool disable_move_magnifier_delay_ = false;
 
+  // Last move magnifier to rect time - used for ignoring caret updates for a
+  // few milliseconds after the last move magnifier to rect call.
+  base::TimeTicks last_move_magnifier_to_rect_;
+
   DISALLOW_COPY_AND_ASSIGN(MagnificationController);
 };
 

ash/magnifier/magnifier_utils.h

@@ -6,6 +6,7 @@
 #define ASH_MAGNIFIER_MAGNIFIER_UTILS_H_
 
 #include "ash/ash_export.h"
+#include "base/time/time.h"
 
 namespace aura {
 class Window;
@@ -23,6 +24,28 @@ namespace magnifier_utils {
 // Note: this value is 2.0 ^ (1 / 4).
 constexpr float kMagnificationScaleFactor = 1.18920712f;
 
+// When magnifier wants to make visible a rect that's wider than the viewport,
+// we want to align the left edge of the rect to the left edge of the viewport.
+// In a right-to-left language, such as Hebrew, we'll want to align the right
+// edge of the rect with the right edge of the viewport. This way, the user can
+// see the maximum amount of useful information in the rect, assuming the
+// information begins at that edge (e.g. an omnibox entry). We also want to
+// include a bit of padding beyond that edge of the rect, to provide more
+// context about what's around the rect to the user. |kLeftEdgeContextPadding|
+// defines how much padding to include in the viewport.
+// TODO(accessibility): Add support for right-to-left languages.
+constexpr int kLeftEdgeContextPadding = 32;
+
+// The duration of time to ignore caret update after the last move magnifier to
+// rect call. Prevents jumping magnifier viewport to caret when user is
+// navigating through other things, but which happen to cause text/caret
+// updates (e.g. the omnibox results). Try keep under one frame buffer length
+// (~16ms assuming 60hz screen updates), however most importantly keep it short,
+// so e.g. when user focuses an element, and then starts typing, the viewport
+// quickly moves to the caret position.
+constexpr base::TimeDelta kPauseCaretUpdateDuration =
+    base::TimeDelta::FromMilliseconds(15);
+
 // Calculates the new scale if it were to be adjusted exponentially by the
 // given |linear_offset|. This allows linear changes in scroll offset
 // to have exponential changes on the scale, so that as the user zooms in,

ash/public/cpp/docked_magnifier_controller.h

@@ -9,6 +9,7 @@
 
 namespace gfx {
 class Point;
+class Rect;
 }
 
 namespace ash {
@@ -26,6 +27,11 @@ class ASH_EXPORT DockedMagnifierController {
   // by itself.
   virtual void CenterOnPoint(const gfx::Point& point_in_screen) = 0;
 
+  // Requests that the Docked Magnifier centers its viewport around the center
+  // of this rect OR aligns the left edge of the viewport with the left edge
+  // of the rect, if the rect is wider than the viewport.
+  virtual void MoveMagnifierToRect(const gfx::Rect& rect_in_screen) = 0;
+
   // Returns docked magnifier height.
   virtual int GetMagnifierHeightForTesting() const = 0;
 

chrome/browser/chromeos/accessibility/magnification_manager.cc

@@ -125,7 +125,7 @@ void MagnificationManager::HandleMoveMagnifierToRectIfEnabled(
     return;
   }
   if (IsDockedMagnifierEnabled()) {
-    ash::DockedMagnifierController::Get()->CenterOnPoint(rect.CenterPoint());
+    ash::DockedMagnifierController::Get()->MoveMagnifierToRect(rect);
   }
 }