Fix elision for bidirectional URLs

URLs where the TLD is separate from the host, which can happen for URLs
of the form http://LTR.RTL/RTL, were displayed with a blank spot when
elision was enabled. This CL changes this so the in between section,
which is not elided in this case, is also not colored transparent.

Bug: 1117631
Change-Id: I9bba2d2c0d494a4021b8c8edaacc87084e040964
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2368458Reviewed-by: Tommy Li <tommycli@chromium.org>
Reviewed-by: Emily Stark <estark@chromium.org>
Commit-Queue: Carlos IL <carlosil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#801559}

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

@@ -188,9 +188,6 @@ OmniboxViewViews::ElideAnimation::ElideAnimation(OmniboxViewViews* view,
 
 OmniboxViewViews::ElideAnimation::~ElideAnimation() = default;
 
-// TODO(estark): this code doesn't work for URLs with RTL components. Will need
-// to figure out another animation or just skip the animation entirely on URLs
-// with RTL components.
 void OmniboxViewViews::ElideAnimation::Start(
     const gfx::Range& elide_to_bounds,
     uint32_t delay_ms,
@@ -199,25 +196,39 @@ void OmniboxViewViews::ElideAnimation::Start(
     SkColor ending_color) {
   DCHECK(ranges_surrounding_simplified_domain.size() == 1 ||
          ranges_surrounding_simplified_domain.size() == 2);
-  ranges_surrounding_simplified_domain_ = ranges_surrounding_simplified_domain;
+
   starting_color_ = starting_color;
   ending_color_ = ending_color;
 
   // simplified_domain_bounds_ will be set to a rectangle surrounding the part
   // of the URL that is never elided, on its original position before any
-  // animation runs. If ranges_surrounding_simplified_domain_ only contains one
+  // animation runs. If ranges_surrounding_simplified_domain only contains one
   // range it means we are not eliding on the right side, so we use the right
   // side of elide_to_bounds as the range as it will always be the right limit
   // of the simplified section.
   gfx::Range simplified_domain_range(
-      ranges_surrounding_simplified_domain_[0].end(),
-      ranges_surrounding_simplified_domain_.size() == 2
-          ? ranges_surrounding_simplified_domain_[1].start()
+      ranges_surrounding_simplified_domain[0].end(),
+      ranges_surrounding_simplified_domain.size() == 2
+          ? ranges_surrounding_simplified_domain[1].start()
           : elide_to_bounds.end());
   for (auto rect : render_text_->GetSubstringBounds(simplified_domain_range)) {
     simplified_domain_bounds_.Union(rect - render_text_->GetLineOffset(0));
   }
 
+  // Keep track of which ranges need to be colored. Any range that when
+  // displayed is in the middle of the unelided section (which can happen for
+  // certain bidirectional URLs) won't be elided, and should not be colored.
+  for (auto range : ranges_surrounding_simplified_domain) {
+    if (range.length() > 0) {
+      gfx::Rect range_bounds;
+      for (auto rect : render_text_->GetSubstringBounds(range))
+        range_bounds.Union(rect - render_text_->GetLineOffset(0));
+      if (!simplified_domain_bounds_.Contains(range_bounds)) {
+        ranges_to_color_.push_back(range);
+      }
+    }
+  }
+
   // After computing |elide_to_rect_| below, |elide_to_bounds| aren't actually
   // need anymore for the animation. However, the bounds provide a convenient
   // way for the animation consumer to check if an animation is currently in
@@ -322,7 +333,7 @@ void OmniboxViewViews::ElideAnimation::AnimationProgressed(
                                                 ending_display_offset_);
   render_text_->SetDisplayOffset(current_offset_);
 
-  for (const auto& range : ranges_surrounding_simplified_domain_) {
+  for (const auto& range : ranges_to_color_) {
     view_->ApplyColor(GetCurrentColor(), range);
   }
 
@@ -2695,9 +2706,19 @@ void OmniboxViewViews::ElideURL() {
 
   // GetSubstringBounds() rounds outward internally, so there may be small
   // portions of text still showing. Set the ranges surrounding the simplified
-  // domain to transparent so that these artifacts don't show.
-  for (const auto& range : ranges_surrounding_simplified_domain)
+  // domain to transparent so that these artifacts don't show. Skip this if
+  // the range is inside the simplified domain bounds (which can happen for
+  // some bidirectional URLs), since it won't be elided in that case.
+  for (const auto& range : ranges_surrounding_simplified_domain) {
+    if (range.length() > 0) {
+      gfx::Rect range_bounds;
+      for (auto rect : GetRenderText()->GetSubstringBounds(range))
+        range_bounds.Union(rect);
+      if (!shifted_simplified_domain_rect.Contains(range_bounds)) {
         ApplyColor(SK_ColorTRANSPARENT, range);
+      }
+    }
+  }
 }
 
 void OmniboxViewViews::ShowFullURL() {

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

@@ -256,9 +256,11 @@ class OmniboxViewViews : public OmniboxView,
     // The current offset, exposed for testing.
     int current_offset_;
 
-    // Holds the ranges surrounding the simplified domain part. As the animation
-    // runs, each range fades from |starting_color_| to |ending_color_|.
-    std::vector<gfx::Range> ranges_surrounding_simplified_domain_;
+    // As the animation runs, each range fades from |starting_color_| to
+    // |ending_color_|. This is normally the ranges surrounding the simplified
+    // domain, but can omit one of them if the simplified domain is not
+    // displayed contiguously (which can happen for some bidirectional URLs).
+    std::vector<gfx::Range> ranges_to_color_;
     SkColor starting_color_;
     SkColor ending_color_;
 
@@ -282,6 +284,8 @@ class OmniboxViewViews : public OmniboxView,
  private:
   FRIEND_TEST_ALL_PREFIXES(OmniboxViewViewsRevealOnHoverTest, HoverAndExit);
   FRIEND_TEST_ALL_PREFIXES(OmniboxViewViewsRevealOnHoverTest, HoverAndExitIDN);
+  FRIEND_TEST_ALL_PREFIXES(OmniboxViewViewsRevealOnHoverTest,
+                           PathNotTransparentSplitURL);
   FRIEND_TEST_ALL_PREFIXES(OmniboxViewViewsRevealOnHoverTest, PrivateRegistry);
   FRIEND_TEST_ALL_PREFIXES(
       OmniboxViewViewsHideOnInteractionAndRevealOnHoverTest,

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

@@ -1897,6 +1897,50 @@ TEST_P(OmniboxViewViewsRevealOnHoverTest, HoverAndExitIDN) {
       kSimplifiedDomainDisplayIDNUrlPath, ShouldElideToRegistrableDomain()));
 }
 
+// Tests the path doesn't disappear for a URL where it appears in between of the
+// unelided section (and thus is not elided).
+TEST_P(OmniboxViewViewsRevealOnHoverTest, PathNotTransparentSplitURL) {
+  // A bidirectional URL with this format causes the tld (مثال) to appear
+  // separate from the host (test), and with the path (إختبار) displayed in
+  // between.
+  const base::string16 kSimplifiedDomainDisplaySplitUrl =
+      base::UTF8ToUTF16("https://test.مثال/إختبار");
+  const base::string16 kSimplifiedDomainDisplaySplitUrlHostnameAndScheme =
+      base::UTF8ToUTF16("https://test.مثال");
+  UpdateDisplayURL(kSimplifiedDomainDisplaySplitUrl);
+
+  // Call OnThemeChanged() to create the animations.
+  omnibox_view()->OnThemeChanged();
+
+  // Check that the path is not transparent.
+  EXPECT_NE(SK_ColorTRANSPARENT,
+            omnibox_view()->GetLatestColorForRange(gfx::Range(
+                kSimplifiedDomainDisplaySplitUrlHostnameAndScheme.size(),
+                kSimplifiedDomainDisplaySplitUrl.size())));
+
+  // Simulate mouse hovering to trigger the unelision animation.
+  omnibox_view()->OnMouseMoved(CreateMouseEvent(ui::ET_MOUSE_MOVED, {0, 0}));
+  OmniboxViewViews::ElideAnimation* hover_animation =
+      omnibox_view()->GetHoverElideOrUnelideAnimationForTesting();
+  ASSERT_TRUE(hover_animation);
+  ASSERT_TRUE(hover_animation->IsAnimating());
+
+  // Advance the clock to let the elision animation finish. Assume it takes less
+  // than 2 seconds.
+  omnibox_view()->StepSimplifiedDomainHoverAnimation(2000);
+
+  // Exit the mouse and let the elision animation run until it finishes.
+  omnibox_view()->OnMouseExited(CreateMouseEvent(ui::ET_MOUSE_EXITED, {0, 0}));
+  ASSERT_TRUE(hover_animation->IsAnimating());
+  omnibox_view()->StepSimplifiedDomainHoverAnimation(2000);
+
+  // Check the path is still not transparent after the animation runs.
+  EXPECT_NE(SK_ColorTRANSPARENT,
+            omnibox_view()->GetLatestColorForRange(gfx::Range(
+                kSimplifiedDomainDisplaySplitUrlHostnameAndScheme.size(),
+                kSimplifiedDomainDisplaySplitUrl.size())));
+}
+
 // Tests the field trial variation that shows a simplified domain by default
 // using a private registry (https://publicsuffix.org/list/). Private registries
 // should be ignored when computing the simplified domain, to avoid creating