Fix RTL URL rendering in Omnibox (domain off screen on long URL).

BUG=656417

Review-Url: https://codereview.chromium.org/2855793003
Cr-Commit-Position: refs/heads/master@{#473830}

chrome/browser/ui/omnibox/omnibox_view_browsertest.cc

@@ -1865,28 +1865,6 @@ IN_PROC_BROWSER_TEST_F(OmniboxViewTest, EditSearchEngines) {
   EXPECT_FALSE(omnibox_view->model()->popup_model()->IsOpen());
 }
 
-IN_PROC_BROWSER_TEST_F(OmniboxViewTest, BeginningShownAfterBlur) {
-  OmniboxView* omnibox_view = NULL;
-  ASSERT_NO_FATAL_FAILURE(GetOmniboxView(&omnibox_view));
-
-  omnibox_view->OnBeforePossibleChange();
-  omnibox_view->SetWindowTextAndCaretPos(ASCIIToUTF16("data:text/plain,test"),
-      5U, false, false);
-  omnibox_view->OnAfterPossibleChange(true);
-  EXPECT_TRUE(ui_test_utils::IsViewFocused(browser(), VIEW_ID_OMNIBOX));
-  size_t start, end;
-  omnibox_view->GetSelectionBounds(&start, &end);
-  EXPECT_EQ(5U, start);
-  EXPECT_EQ(5U, end);
-
-  ui_test_utils::FocusView(browser(), VIEW_ID_TAB_CONTAINER);
-  EXPECT_FALSE(ui_test_utils::IsViewFocused(browser(), VIEW_ID_OMNIBOX));
-
-  omnibox_view->GetSelectionBounds(&start, &end);
-  EXPECT_EQ(0U, start);
-  EXPECT_EQ(0U, end);
-}
-
 IN_PROC_BROWSER_TEST_F(OmniboxViewTest, CtrlArrowAfterArrowSuggestions) {
   OmniboxView* omnibox_view = NULL;
   ASSERT_NO_FATAL_FAILURE(GetOmniboxView(&omnibox_view));

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

@@ -782,9 +782,9 @@ void OmniboxViewViews::OnBlur() {
   // at least call CloseOmniboxPopup(), so that if ZeroSuggest is in the
   // midst of running but hasn't yet opened the popup, it will be halted.
   // If we fully reverted in this case, we'd lose the cursor/highlight
-  // information saved above.
-  if (!model()->user_input_in_progress() && model()->popup_model()->IsOpen() &&
-      text() != model()->PermanentText())
+  // information saved above. Note: popup_model() can be null in tests.
+  if (!model()->user_input_in_progress() && model()->popup_model() &&
+      model()->popup_model()->IsOpen() && text() != model()->PermanentText())
     RevertAll();
   else
     CloseOmniboxPopup();
@@ -792,17 +792,27 @@ void OmniboxViewViews::OnBlur() {
   // Tell the model to reset itself.
   model()->OnKillFocus();
 
-  // Make sure the beginning of the text is visible.
-  SelectRange(gfx::Range(0));
-
-  GetRenderText()->SetElideBehavior(gfx::ELIDE_TAIL);
+  // When deselected, elide and reset scroll position. After eliding, the old
+  // scroll offset is meaningless (since the string is guaranteed to fit within
+  // the view). The scroll must be reset or the text may be rendered partly or
+  // wholly off-screen.
+  //
+  // Important: Since the URL can contain bidirectional text, it is important to
+  // set the display offset directly to 0 (not simply scroll to the start of the
+  // text, since the start of the text may not be at the left edge).
+  gfx::RenderText* render_text = GetRenderText();
+  render_text->SetElideBehavior(gfx::ELIDE_TAIL);
+  render_text->SetDisplayOffset(0);
 
   // Focus changes can affect the visibility of any keyword hint.
+  // |location_bar_view_| can be null in tests.
+  if (location_bar_view_) {
     if (model()->is_keyword_hint())
       location_bar_view_->Layout();
 
     // The location bar needs to repaint without a focus ring.
     location_bar_view_->SchedulePaint();
+  }
 }
 
 bool OmniboxViewViews::IsCommandIdEnabled(int command_id) const {

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

@@ -108,6 +108,7 @@ class OmniboxViewViews
  private:
   FRIEND_TEST_ALL_PREFIXES(OmniboxViewViewsTest, CloseOmniboxPopupOnTextDrag);
   FRIEND_TEST_ALL_PREFIXES(OmniboxViewViewsTest, MaintainCursorAfterFocusCycle);
+  FRIEND_TEST_ALL_PREFIXES(OmniboxViewViewsTest, OnBlur);
 
   // Update the field with |text| and set the selection.
   void SetTextAndSelectedRange(const base::string16& text,

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

@@ -149,10 +149,9 @@ IN_PROC_BROWSER_TEST_F(OmniboxViewViewsTest, SelectAllOnClick) {
   EXPECT_TRUE(ui_test_utils::IsViewFocused(browser(), VIEW_ID_OMNIBOX));
   EXPECT_TRUE(omnibox_view->IsSelectAll());
 
-  // Clicking in another view should clear focus and the selection.
+  // Clicking in another view should clear focus.
   ASSERT_NO_FATAL_FAILURE(ClickBrowserWindowCenter());
   EXPECT_FALSE(ui_test_utils::IsViewFocused(browser(), VIEW_ID_OMNIBOX));
-  EXPECT_FALSE(omnibox_view->IsSelectAll());
 
   // Clicking in the omnibox again should take focus and select all text again.
   ASSERT_NO_FATAL_FAILURE(Click(ui_controls::LEFT,
@@ -245,7 +244,6 @@ IN_PROC_BROWSER_TEST_F(OmniboxViewViewsTest, SelectAllOnTap) {
   // Take the focus away from the omnibox.
   ASSERT_NO_FATAL_FAILURE(TapBrowserWindowCenter());
   EXPECT_FALSE(ui_test_utils::IsViewFocused(browser(), VIEW_ID_OMNIBOX));
-  EXPECT_FALSE(omnibox_view->IsSelectAll());
 
   // Tapping in the omnibox should take focus and select all text.
   const gfx::Rect omnibox_bounds = BrowserView::GetBrowserViewForBrowser(
@@ -255,10 +253,9 @@ IN_PROC_BROWSER_TEST_F(OmniboxViewViewsTest, SelectAllOnTap) {
   EXPECT_TRUE(ui_test_utils::IsViewFocused(browser(), VIEW_ID_OMNIBOX));
   EXPECT_TRUE(omnibox_view->IsSelectAll());
 
-  // Tapping in another view should clear focus and the selection.
+  // Tapping in another view should clear focus.
   ASSERT_NO_FATAL_FAILURE(TapBrowserWindowCenter());
   EXPECT_FALSE(ui_test_utils::IsViewFocused(browser(), VIEW_ID_OMNIBOX));
-  EXPECT_FALSE(omnibox_view->IsSelectAll());
 
   // Tapping in the omnibox again should take focus and select all text again.
   ASSERT_NO_FATAL_FAILURE(Tap(tap_location, tap_location));

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

@@ -24,6 +24,9 @@
 #include "ui/base/ime/text_edit_commands.h"
 #include "ui/events/event_utils.h"
 #include "ui/events/keycodes/dom/dom_code.h"
+#include "ui/gfx/geometry/rect.h"
+#include "ui/gfx/geometry/vector2d.h"
+#include "ui/gfx/render_text.h"
 #include "ui/views/controls/textfield/textfield_test_api.h"
 
 #if defined(OS_CHROMEOS)
@@ -53,6 +56,8 @@ class TestingOmniboxView : public OmniboxViewViews {
     return emphasize ? EMPHASIZED : DEEMPHASIZED;
   }
 
+  using views::Textfield::GetRenderText;
+
   void ResetEmphasisTestState();
 
   void CheckUpdatePopupCallInfo(size_t call_count,
@@ -295,6 +300,41 @@ TEST_F(OmniboxViewViewsTest, ScheduledTextEditCommand) {
             scheduled_text_edit_command());
 }
 
+TEST_F(OmniboxViewViewsTest, OnBlur) {
+  // Make the Omnibox very narrow (so it couldn't fit the whole string).
+  int kOmniboxWidth = 60;
+  gfx::RenderText* render_text = omnibox_view()->GetRenderText();
+  render_text->SetDisplayRect(gfx::Rect(0, 0, kOmniboxWidth, 10));
+  render_text->SetHorizontalAlignment(gfx::ALIGN_LEFT);
+
+  // (In this example, uppercase Latin letters represent Hebrew letters.)
+  // The string |kContentsRtl| is equivalent to:
+  //     RA.QWM/0123/abcd
+  // This is displayed as:
+  //     0123/MWQ.AR/abcd
+  // Enter focused mode, where the text should *not* be elided, and we expect
+  // SetWindowTextAndCaretPos to scroll such that the start of the string is
+  // on-screen. Because the domain is RTL, this scrolls to an offset greater
+  // than 0.
+  omnibox_view()->OnFocus();
+  const base::string16 kContentsRtl =
+      base::WideToUTF16(L"\x05e8\x05e2.\x05e7\x05d5\x05dd/0123/abcd");
+  static_cast<OmniboxView*>(omnibox_view())
+      ->SetWindowTextAndCaretPos(kContentsRtl, 0, false, false);
+  EXPECT_EQ(gfx::NO_ELIDE, render_text->elide_behavior());
+  // NOTE: Technically (depending on the font), this expectation could fail if
+  // the entire domain fits in 60 pixels. However, 60px is so small it should
+  // never happen with any font.
+  EXPECT_GT(0, render_text->GetUpdatedDisplayOffset().x());
+
+  // Now enter blurred mode, where the text should be elided to 60px. This means
+  // the string itself is truncated. Scrolling would therefore mean the text is
+  // off-screen. Ensure that the horizontal scrolling has been reset to 0.
+  omnibox_view()->OnBlur();
+  EXPECT_EQ(gfx::ELIDE_TAIL, render_text->elide_behavior());
+  EXPECT_EQ(0, render_text->GetUpdatedDisplayOffset().x());
+}
+
 TEST_F(OmniboxViewViewsTest, Emphasis) {
   constexpr struct {
     const char* input;