Simplified domain display: fix URL jumping around

This CL fixes horizontal and vertical jumping when simplified domain
omnibox field trials are enabled.

For vertical jumping, we were using the height and vertical position
returned from GetSubstringBounds(), but that doesn't always match the
existing height and vertical position of the URL in the omnibox. To
fix this, we now reuse the previous bounds' height and vertical
position when eliding/uneliding to the simplified domain.

Horizontal jumping was due to not taking the Textfield's border insets
into account when eliding/uneliding. To fix this, when eliding or
uneliding, we now set the display rect's x value to that of the
previous bounds' x value, instead of 0, so that the leading edge of
the display rect always stays in the same spot. When uneliding to the
full URL, we reuse the logic from Textfield::OnBoundsChanged() (which
has been factored out into its own method) to reset the Textfield's
display rect to its local bounds.

Bug: 1101674
Change-Id: Ia32420a2d4365910267acd05b86cd299204b7e95
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2283655
Commit-Queue: Emily Stark <estark@chromium.org>
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#786011}

chrome/browser/ui/views/omnibox/omnibox_view_views.cc

@@ -184,9 +184,16 @@ void OmniboxViewViews::ElideAnimation::Start(const gfx::Range& elide_to_bounds,
   elide_to_rect_ = gfx::Rect();
   for (const auto& rect : render_text_->GetSubstringBounds(elide_to_bounds))
     elide_to_rect_.Union(rect);
+  // The URL should never shift vertically while eliding to/from simplified
+  // domain.
+  elide_to_rect_.set_y(elide_from_rect_.y());
+  elide_to_rect_.set_height(elide_from_rect_.height());
 
   starting_display_offset_ = render_text_->GetUpdatedDisplayOffset().x();
-  ending_display_offset_ = starting_display_offset_ + -1 * (elide_to_rect_.x());
+  // Shift the text to where |elide_to_bounds| starts, relative to the current
+  // display rect.
+  ending_display_offset_ =
+      starting_display_offset_ - (elide_to_rect_.x() - elide_from_rect_.x());
 
   animation_->Start();
 }
@@ -216,12 +223,18 @@ void OmniboxViewViews::ElideAnimation::AnimationProgressed(
     return;
 
   // |bounds| contains the interpolated substring to show for this frame. Shift
-  // it to x=0 position because the animation should gradually bring the desired
-  // string into view at the leftmost position.
+  // it to line up with the x position of the previous frame (|old_bounds|),
+  // because the animation should gradually bring the desired string into view
+  // at the leading edge. The y/height values shouldn't change because
+  // |elide_to_rect_| is set to have the same y and height values as
+  // |elide_to_rect_|.
+  gfx::Rect old_bounds = render_text_->display_rect();
   gfx::Rect bounds = gfx::Tween::RectValueBetween(
       animation->GetCurrentValue(), elide_from_rect_, elide_to_rect_);
-  gfx::Rect shifted_bounds(/*x=*/0, bounds.y(), bounds.width(),
-                           bounds.height());
+  DCHECK_EQ(bounds.y(), old_bounds.y());
+  DCHECK_EQ(bounds.height(), old_bounds.height());
+  gfx::Rect shifted_bounds(old_bounds.x(), old_bounds.y(), bounds.width(),
+                           old_bounds.height());
   render_text_->SetDisplayRect(shifted_bounds);
 
   render_text_->SetDisplayOffset(gfx::Tween::IntValueBetween(
@@ -430,7 +443,7 @@ void OmniboxViewViews::EmphasizeURLComponents() {
     } else {
       // If the text isn't eligible to be elided to a simplified domain, then
       // ensure that as much of it is visible as will fit.
-      GetRenderText()->SetDisplayRect(GetLocalBounds());
+      FitToLocalBounds();
     }
   }
 }
@@ -1675,9 +1688,9 @@ void OmniboxViewViews::OnBlur() {
       if (IsURLEligibleForSimplifiedDomainEliding()) {
         ElideToSimplifiedDomain();
       } else {
-        // If the text isn't a URL that can be elided to a simplified domain,
-        // then ensure that as much of it is visible as possible.
-        GetRenderText()->SetDisplayRect(GetLocalBounds());
+        // If the text isn't eligible to be elided to a simplified domain, then
+        // ensure that as much of it is visible as will fit.
+        FitToLocalBounds();
       }
     } else if (OmniboxFieldTrial::ShouldHidePathQueryRefOnInteraction()) {
       // When hide-on-interaction is enabled, this method ensures that, once the
@@ -2354,17 +2367,22 @@ void OmniboxViewViews::ElideToSimplifiedDomain() {
   }
 
   // |simplified_domain_rect| gives us the current bounds of the simplified
-  // domain substring. We shift it to the leftmost edge of the omnibox, and then
-  // scroll to where the simplified domain begins, so that the simplified domain
+  // domain substring. We shift it to the leftmost edge of the omnibox (as
+  // determined by the x position of the current display rect), and then scroll
+  // to where the simplified domain begins, so that the simplified domain
   // appears at the leftmost edge.
-  gfx::Rect shifted_simplified_domain_rect(/*x=*/0, simplified_domain_rect.y(),
+  gfx::Rect old_bounds = GetRenderText()->display_rect();
+  // Use |old_bounds| for y and height values because the URL should never shift
+  // vertically while eliding to/from simplified domain.
+  gfx::Rect shifted_simplified_domain_rect(old_bounds.x(), old_bounds.y(),
                                            simplified_domain_rect.width(),
-                                           simplified_domain_rect.height());
-
+                                           old_bounds.height());
   GetRenderText()->SetDisplayRect(shifted_simplified_domain_rect);
+  // Scroll the text to where the simplified domain begins, relative to the
+  // leftmost edge of the current display rect.
   GetRenderText()->SetDisplayOffset(
       GetRenderText()->GetUpdatedDisplayOffset().x() -
-      simplified_domain_rect.x());
+      (simplified_domain_rect.x() - old_bounds.x()));
 }
 
 void OmniboxViewViews::UnelideFromSimplifiedDomain() {
@@ -2378,9 +2396,8 @@ void OmniboxViewViews::UnelideFromSimplifiedDomain() {
   if (elide_after_interaction_animation_)
     elide_after_interaction_animation_->Stop();
   ApplyCaretVisibility();
+  FitToLocalBounds();
   GetRenderText()->SetElideBehavior(gfx::ELIDE_TAIL);
-  GetRenderText()->SetDisplayRect(GetLocalBounds());
-  GetRenderText()->SetDisplayOffset(0);
 }
 
 OmniboxViewViews::ElideAnimation*

chrome/browser/ui/views/omnibox/omnibox_view_views.h

@@ -180,6 +180,9 @@ class OmniboxViewViews : public OmniboxView,
   FRIEND_TEST_ALL_PREFIXES(
       OmniboxViewViewsHideOnInteractionAndRevealOnHoverTest,
       PathChangeDuringAnimation);
+  FRIEND_TEST_ALL_PREFIXES(
+      OmniboxViewViewsHideOnInteractionAndRevealOnHoverTest,
+      VerticalAndHorizontalPosition);
   FRIEND_TEST_ALL_PREFIXES(OmniboxViewViewsHideOnInteractionTest,
                            SameDocNavigations);
   FRIEND_TEST_ALL_PREFIXES(OmniboxViewViewsHideOnInteractionTest,

chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc

@@ -1823,6 +1823,67 @@ TEST_P(OmniboxViewViewsHideOnInteractionAndRevealOnHoverTest,
   EXPECT_FALSE(elide_animation->IsAnimating());
 }
 
+// Tests that vertical and horizontal positioning doesn't change when eliding
+// to/from simplified domain. Regression test for https://crbug.com/1101674.
+TEST_P(OmniboxViewViewsHideOnInteractionAndRevealOnHoverTest,
+       VerticalAndHorizontalPosition) {
+  const base::string16 kDisplayUrl = base::ASCIIToUTF16("foo.example.test/bar");
+  location_bar_model()->set_url(GURL("https://foo.example.test/bar"));
+  location_bar_model()->set_url_for_display(kDisplayUrl);
+  omnibox_view()->model()->ResetDisplayTexts();
+  omnibox_view()->RevertAll();
+  gfx::RenderText* render_text = omnibox_view()->GetRenderText();
+
+  // Call OnThemeChanged() to create the animations.
+  omnibox_view()->OnThemeChanged();
+
+  const gfx::Rect& original_display_rect = render_text->display_rect();
+
+  content::MockNavigationHandle navigation;
+  navigation.set_is_same_document(false);
+  omnibox_view()->DidFinishNavigation(&navigation);
+  ASSERT_NO_FATAL_FAILURE(ExpectUnelidedFromSimplifiedDomain(
+      omnibox_view()->GetRenderText(), kDisplayUrl));
+
+  // After a navigation, the URL should not be elided to the simplified domain,
+  // and the display rect (including vertical and horizontal position) should be
+  // unchanged.
+  ASSERT_NO_FATAL_FAILURE(ExpectUnelidedFromSimplifiedDomain(
+      omnibox_view()->GetRenderText(), kDisplayUrl));
+  EXPECT_EQ(original_display_rect, render_text->display_rect());
+
+  // Simulate a user interaction to elide to simplified domain and advance
+  // through the animation; the vertical position should still be unchanged, and
+  // the text should still start at the some position (the same x value).
+  omnibox_view()->DidGetUserInteraction(
+      blink::WebInputEvent::Type::kGestureScrollBegin);
+  OmniboxViewViews::ElideAnimation* elide_animation =
+      omnibox_view()->GetElideAfterInteractionAnimationForTesting();
+  gfx::AnimationContainerElement* elide_as_element =
+      elide_animation->GetAnimationForTesting();
+  elide_as_element->SetStartTime(base::TimeTicks());
+  elide_as_element->Step(base::TimeTicks() + base::TimeDelta::FromSeconds(1));
+  const gfx::Rect& elided_display_rect = render_text->display_rect();
+  EXPECT_EQ(original_display_rect.y(), elided_display_rect.y());
+  EXPECT_EQ(original_display_rect.height(), elided_display_rect.height());
+  EXPECT_EQ(original_display_rect.x(), elided_display_rect.x());
+
+  // Now hover over the omnibox to trigger an unelide and check that the display
+  // rect (including vertical and horizontal position) is back to what it was
+  // originally.
+  omnibox_view()->OnMouseMoved(CreateMouseEvent(ui::ET_MOUSE_MOVED, {0, 0}));
+  OmniboxViewViews::ElideAnimation* unelide_animation =
+      omnibox_view()->GetHoverElideOrUnelideAnimationForTesting();
+  ASSERT_TRUE(unelide_animation);
+  gfx::AnimationContainerElement* unelide_as_element =
+      static_cast<gfx::AnimationContainerElement*>(
+          unelide_animation->GetAnimationForTesting());
+  unelide_as_element->SetStartTime(base::TimeTicks());
+  unelide_as_element->Step(base::TimeTicks() + base::TimeDelta::FromSeconds(1));
+  const gfx::Rect& unelided_display_rect = render_text->display_rect();
+  EXPECT_EQ(original_display_rect, unelided_display_rect);
+}
+
 // Tests that in the hide-on-interaction field trial, the URL is simplified on
 // cross-document main-frame navigations, but not on same-document navigations.
 TEST_P(OmniboxViewViewsHideOnInteractionTest, SameDocNavigations) {

ui/views/controls/textfield/textfield.cc

@@ -656,6 +656,24 @@ void Textfield::SetExtraInsets(const gfx::Insets& insets) {
   UpdateBorder();
 }
 
+void Textfield::FitToLocalBounds() {
+  // Textfield insets include a reasonable amount of whitespace on all sides of
+  // the default font list. Fallback fonts with larger heights may paint over
+  // the vertical whitespace as needed. Alternate solutions involve undesirable
+  // behavior like changing the default font size, shrinking some fallback fonts
+  // beyond their legibility, or enlarging controls dynamically with content.
+  gfx::Rect bounds = GetLocalBounds();
+  const gfx::Insets insets = GetInsets();
+  // The text will draw with the correct vertical alignment if we don't apply
+  // the vertical insets.
+  bounds.Inset(insets.left(), 0, insets.right(), 0);
+  bounds.set_x(GetMirroredXForRect(bounds));
+  GetRenderText()->SetDisplayRect(bounds);
+  OnCaretBoundsChanged();
+  UpdateCursorViewPosition();
+  UpdateCursorVisibility();
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // Textfield, View overrides:
 
@@ -1091,21 +1109,7 @@ bool Textfield::HandleAccessibleAction(const ui::AXActionData& action_data) {
 }
 
 void Textfield::OnBoundsChanged(const gfx::Rect& previous_bounds) {
-  // Textfield insets include a reasonable amount of whitespace on all sides of
-  // the default font list. Fallback fonts with larger heights may paint over
-  // the vertical whitespace as needed. Alternate solutions involve undesirable
-  // behavior like changing the default font size, shrinking some fallback fonts
-  // beyond their legibility, or enlarging controls dynamically with content.
-  gfx::Rect bounds = GetLocalBounds();
-  const gfx::Insets insets = GetInsets();
-  // The text will draw with the correct vertical alignment if we don't apply
-  // the vertical insets.
-  bounds.Inset(insets.left(), 0, insets.right(), 0);
-  bounds.set_x(GetMirroredXForRect(bounds));
-  GetRenderText()->SetDisplayRect(bounds);
-  OnCaretBoundsChanged();
-  UpdateCursorViewPosition();
-  UpdateCursorVisibility();
+  FitToLocalBounds();
 }
 
 bool Textfield::GetNeedsNotificationWhenVisibleBoundsChange() const {

ui/views/controls/textfield/textfield.h

@@ -261,6 +261,10 @@ class VIEWS_EXPORT Textfield : public View,
 
   void SetExtraInsets(const gfx::Insets& insets);
 
+  // Fits the textfield to the local bounds, applying internal padding and
+  // updating the cursor position and visibility.
+  void FitToLocalBounds();
+
   // View overrides:
   int GetBaseline() const override;
   gfx::Size CalculatePreferredSize() const override;

ui/views/controls/textfield/textfield_unittest.cc

@@ -3410,6 +3410,22 @@ TEST_F(TextfieldTest, CursorVisibility) {
   EXPECT_TRUE(test_api_->IsCursorVisible());
 }
 
+// Tests that Textfield::FitToLocalBounds() sets the RenderText's display rect
+// to the view's bounds, taking the border into account.
+TEST_F(TextfieldTest, FitToLocalBounds) {
+  const int kDisplayRectWidth = 100;
+  const int kBorderWidth = 5;
+  InitTextfield();
+  textfield_->SetBounds(0, 0, kDisplayRectWidth, 100);
+  textfield_->SetBorder(views::CreateEmptyBorder(
+      gfx::Insets(kBorderWidth, kBorderWidth, kBorderWidth, kBorderWidth)));
+  test_api_->GetRenderText()->SetDisplayRect(gfx::Rect(0, 0, 20, 20));
+  ASSERT_EQ(20, test_api_->GetRenderText()->display_rect().width());
+  textfield_->FitToLocalBounds();
+  EXPECT_EQ(kDisplayRectWidth - 2 * kBorderWidth,
+            test_api_->GetRenderText()->display_rect().width());
+}
+
 // Verify that cursor view height does not exceed the textfield height.
 TEST_F(TextfieldTest, CursorViewHeight) {
   InitTextfield();