Omnibox: Steady State Elisions - Stop abusing user_input_in_progress.
Currently, when Steady State Elisions is on, and the user unelides the
URL, we put the full URL into the user text.
This was convenient, but has some unintended consequences. Namely:
1) user_input_in_progress() as a boolean state becomes confusing, as
sometimes it means the user has edited the URL, and sometimes it
means the the user has merely unelided the URL.
2) Consequently, in many places in the code, we check if user input in
progress AND if the user has meaningfully edited the URL away from
the full URL text.
3) That's not obvious to non-omnibox owners, so it creates bugs like
the one linked below.
This CL changes how unelision works so it only updates the View's text
and leaves the model's user-text alone. It no longer sets
user_input_in_progress to true for mere unelision.
This allows us to clean up fair number of callsites and puts the nail
in the coffin for the below linked bug too.
Bug: 921777
Change-Id: I9d113649f50c0bfafa1d112002add577e05928f7
Reviewed-on: https://chromium-review.googlesource.com/c/1427244
Reviewed-by: manuk hovanesian <manukh@chromium.org>
Commit-Queue: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#625358}diff --git a/chrome/browser/ui/views/omnibox/omnibox_view_views.cc b/chrome/browser/ui/views/omnibox/omnibox_view_views.cc
index b98328b..0a88597 100644
--- a/chrome/browser/ui/views/omnibox/omnibox_view_views.cc
+++ b/chrome/browser/ui/views/omnibox/omnibox_view_views.cc
@@ -1230,15 +1230,16 @@
// Save the user's existing selection to restore it later.
saved_selection_for_focus_change_ = GetSelectedRange();
- // Revert the URL if the user has not made any changes. If steady-state
- // elisions is on, this will also re-elide the URL.
+ // If the view is showing text that's not user-text, revert the text to the
+ // permanent display text. This usually occurs if Steady State Elisions is on
+ // and the user has unelided, but not edited the URL.
//
// Because merely Alt-Tabbing to another window and back should not change the
// Omnibox state, we only revert the text only if the Omnibox is blurred in
// favor of some other View in the same Widget.
if (GetWidget() && GetWidget()->IsActive() &&
- model()->user_input_in_progress() &&
- text() == model()->GetPermanentDisplayText()) {
+ !model()->user_input_in_progress() &&
+ text() != model()->GetPermanentDisplayText()) {
RevertAll();
}
diff --git a/chrome/browser/ui/views/omnibox/omnibox_view_views.h b/chrome/browser/ui/views/omnibox/omnibox_view_views.h
index 0890b887..cffc8a7 100644
--- a/chrome/browser/ui/views/omnibox/omnibox_view_views.h
+++ b/chrome/browser/ui/views/omnibox/omnibox_view_views.h
@@ -103,6 +103,10 @@
using OmniboxView::SetUserText;
void SetUserText(const base::string16& text,
bool update_popup) override;
+ void SetWindowTextAndCaretPos(const base::string16& text,
+ size_t caret_pos,
+ bool update_popup,
+ bool notify_text_changed) override;
void EnterKeywordModeForDefaultSearchProvider() override;
bool IsSelectAll() const override;
void GetSelectionBounds(base::string16::size_type* start,
@@ -192,10 +196,6 @@
bool MaybeUnfocusTabButton();
// OmniboxView:
- void SetWindowTextAndCaretPos(const base::string16& text,
- size_t caret_pos,
- bool update_popup,
- bool notify_text_changed) override;
void SetCaretPos(size_t caret_pos) override;
void UpdatePopup() override;
void ApplyCaretVisibility() override;
diff --git a/chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc b/chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc
index 819d0da0..7e8706f 100644
--- a/chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc
+++ b/chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc
@@ -386,8 +386,7 @@
location_bar_model()->set_url(GURL("http://www.example.com/?query=1"));
const base::string16 text =
base::ASCIIToUTF16("http://www.example.com/?query=1");
- static_cast<OmniboxView*>(omnibox_view())
- ->SetWindowTextAndCaretPos(text, 23U, false, false);
+ omnibox_view()->SetWindowTextAndCaretPos(text, 23U, false, false);
ui::KeyEvent shift_up_pressed(ui::ET_KEY_PRESSED, ui::VKEY_UP,
ui::EF_SHIFT_DOWN);
@@ -399,8 +398,7 @@
EXPECT_EQ(0U, end);
omnibox_view()->CheckUpdatePopupNotCalled();
- static_cast<OmniboxView*>(omnibox_view())
- ->SetWindowTextAndCaretPos(text, 18U, false, false);
+ omnibox_view()->SetWindowTextAndCaretPos(text, 18U, false, false);
ui::KeyEvent shift_down_pressed(ui::ET_KEY_PRESSED, ui::VKEY_DOWN,
ui::EF_SHIFT_DOWN);
@@ -431,8 +429,7 @@
omnibox_textfield()->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);
+ 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
@@ -497,14 +494,28 @@
}
TEST_F(OmniboxViewViewsTest, RevertOnBlur) {
- location_bar_model()->set_url(GURL("https://permanent-text.com/"));
+ location_bar_model()->set_url(GURL("https://example.com/"));
omnibox_view()->model()->ResetDisplayTexts();
omnibox_view()->RevertAll();
- EXPECT_EQ(base::ASCIIToUTF16("https://permanent-text.com/"),
- omnibox_view()->text());
+ EXPECT_EQ(base::ASCIIToUTF16("https://example.com/"), omnibox_view()->text());
EXPECT_FALSE(omnibox_view()->model()->user_input_in_progress());
+ // Set the view text without updating the model's user text. This usually
+ // occurs when the omnibox unapplies Steady State Elisions to temporarily show
+ // the full URL to the user.
+ omnibox_view()->SetWindowTextAndCaretPos(base::ASCIIToUTF16("view text"), 0,
+ false, false);
+ EXPECT_EQ(base::ASCIIToUTF16("view text"), omnibox_view()->text());
+ EXPECT_FALSE(omnibox_view()->model()->user_input_in_progress());
+
+ // Expect that on blur, we revert to the original text and are not in user
+ // input mode.
+ omnibox_textfield()->OnBlur();
+ EXPECT_EQ(base::ASCIIToUTF16("https://example.com/"), omnibox_view()->text());
+ EXPECT_FALSE(omnibox_view()->model()->user_input_in_progress());
+
+ // Now set user text, which is reflected into the model as well.
omnibox_view()->SetUserText(base::ASCIIToUTF16("user text"));
EXPECT_EQ(base::ASCIIToUTF16("user text"), omnibox_view()->text());
EXPECT_TRUE(omnibox_view()->model()->user_input_in_progress());
@@ -513,16 +524,6 @@
omnibox_textfield()->OnBlur();
EXPECT_EQ(base::ASCIIToUTF16("user text"), omnibox_view()->text());
EXPECT_TRUE(omnibox_view()->model()->user_input_in_progress());
-
- // Expect that on blur, if the text is the same as the
- // https://permanent-text.com, exit user input mode.
- omnibox_view()->SetUserText(
- base::ASCIIToUTF16("https://permanent-text.com/"));
- EXPECT_TRUE(omnibox_view()->model()->user_input_in_progress());
- omnibox_textfield()->OnBlur();
- EXPECT_EQ(base::ASCIIToUTF16("https://permanent-text.com/"),
- omnibox_view()->text());
- EXPECT_FALSE(omnibox_view()->model()->user_input_in_progress());
}
TEST_F(OmniboxViewViewsTest, RevertOnEscape) {
@@ -696,13 +697,7 @@
void ExpectFullUrlDisplayed() {
EXPECT_EQ(base::UTF8ToUTF16(kFullUrl.spec()), omnibox_view()->text());
- EXPECT_TRUE(omnibox_view()->model()->user_input_in_progress());
-
- // We test the user text stored in the model has been updated as well. The
- // model user text is used to populate the text in the Omnibox after some
- // state transitions, such as the ZeroSuggest popup opening.
- EXPECT_EQ(base::UTF8ToUTF16(kFullUrl.spec()),
- omnibox_view()->model()->GetUserTextForTesting());
+ EXPECT_FALSE(omnibox_view()->model()->user_input_in_progress());
}
bool IsElidedUrlDisplayed() {
diff --git a/components/omnibox/browser/omnibox_edit_model.cc b/components/omnibox/browser/omnibox_edit_model.cc
index f7bd5ce..a8f1623 100644
--- a/components/omnibox/browser/omnibox_edit_model.cc
+++ b/components/omnibox/browser/omnibox_edit_model.cc
@@ -232,12 +232,6 @@
bool OmniboxEditModel::ResetDisplayTexts() {
const base::string16 old_display_text = GetPermanentDisplayText();
- // Track if the user has modified the text. This is different from
- // |user_input_in_progress_| because we care if the user has actually
- // modified the text, while |user_input_in_progress_| may be true even if
- // the user has merely made a partial selection.
- bool user_has_modified_text = view_->GetText() != old_display_text;
-
LocationBarModel* location_bar_model = controller()->GetLocationBarModel();
url_for_editing_ = location_bar_model->GetFormattedFullURL();
@@ -266,7 +260,7 @@
// URL" (which sounds as if it might be persistent) from seeing just that URL
// forever afterwards.
return (GetPermanentDisplayText() != old_display_text) &&
- (!has_focus() || (!user_has_modified_text && !PopupIsOpen()));
+ (!has_focus() || (!user_input_in_progress_ && !PopupIsOpen()));
}
GURL OmniboxEditModel::PermanentURL() const {
@@ -275,9 +269,6 @@
}
base::string16 OmniboxEditModel::GetPermanentDisplayText() const {
- if (user_input_in_progress_)
- return url_for_editing_;
-
return display_text_;
}
@@ -308,7 +299,6 @@
location_bar_model->GetDisplaySearchTerms(nullptr))
return false;
- SetUserText(url_for_editing_);
view_->SetWindowTextAndCaretPos(url_for_editing_, 0, false, false);
// Select all in reverse to ensure the beginning of the URL is shown.
@@ -366,7 +356,7 @@
// If the user has not modified the display text and is copying the whole
// display text, copy the omnibox contents as a hyperlink to the current page.
- if (!user_input_in_progress_ && *text == GetPermanentDisplayText()) {
+ if (!user_input_in_progress_ && *text == display_text_) {
*url_from_text = PermanentURL();
*write_url = true;
@@ -1230,7 +1220,7 @@
view_->OnInlineAutocompleteTextCleared();
const base::string16& user_text =
- user_input_in_progress_ ? user_text_ : GetPermanentDisplayText();
+ user_input_in_progress_ ? user_text_ : view_->GetText();
if (keyword_state_changed && is_keyword_selected()) {
// If we reach here, the user most likely entered keyword mode by inserting
// a space between a keyword name and a search string (as pressing space or
diff --git a/components/omnibox/browser/omnibox_edit_model_unittest.cc b/components/omnibox/browser/omnibox_edit_model_unittest.cc
index d5d5235..5b2f744a 100644
--- a/components/omnibox/browser/omnibox_edit_model_unittest.cc
+++ b/components/omnibox/browser/omnibox_edit_model_unittest.cc
@@ -229,6 +229,32 @@
EXPECT_EQ(base::string16(), view()->inline_autocomplete_text());
}
+// iOS doesn't use elisions in the Omnibox textfield.
+#if !defined(OS_IOS)
+TEST_F(OmniboxEditModelTest, RespectUnelisionInZeroSuggest) {
+ location_bar_model()->set_url(GURL("https://www.example.com/"));
+ location_bar_model()->set_url_for_display(base::ASCIIToUTF16("example.com"));
+
+ EXPECT_TRUE(model()->ResetDisplayTexts());
+ model()->Revert();
+
+ // Set up view with unelided text.
+ EXPECT_EQ(base::ASCIIToUTF16("example.com"), view()->GetText());
+ EXPECT_TRUE(model()->Unelide(false /* exit_query_in_omnibox */));
+ EXPECT_EQ(base::ASCIIToUTF16("https://www.example.com/"), view()->GetText());
+ EXPECT_FALSE(model()->user_input_in_progress());
+ EXPECT_TRUE(view()->IsSelectAll());
+
+ // Test that we don't clobber the unelided text with inline autocomplete text.
+ EXPECT_EQ(base::string16(), view()->inline_autocomplete_text());
+ model()->OnPopupDataChanged(base::string16(), nullptr, base::string16(),
+ false);
+ EXPECT_EQ(base::ASCIIToUTF16("https://www.example.com/"), view()->GetText());
+ EXPECT_FALSE(model()->user_input_in_progress());
+ EXPECT_TRUE(view()->IsSelectAll());
+}
+#endif // !defined(OS_IOS)
+
// This verifies the fix for a bug where calling OpenMatch() with a valid
// alternate nav URL would fail a DCHECK if the input began with "http://".
// The failure was due to erroneously trying to strip the scheme from the
@@ -295,6 +321,7 @@
// permanent display text. Unelision should return false.
EXPECT_EQ(base::ASCIIToUTF16("https://www.example.com/"),
model()->GetPermanentDisplayText());
+ EXPECT_EQ(base::ASCIIToUTF16("https://www.example.com/"), view()->GetText());
EXPECT_FALSE(model()->Unelide(false /* exit_query_in_omnibox */));
EXPECT_FALSE(model()->user_input_in_progress());
EXPECT_FALSE(view()->IsSelectAll());
@@ -302,8 +329,9 @@
// Verify we can unelide and show the full URL properly.
EXPECT_EQ(base::ASCIIToUTF16("example.com"),
model()->GetPermanentDisplayText());
+ EXPECT_EQ(base::ASCIIToUTF16("example.com"), view()->GetText());
EXPECT_TRUE(model()->Unelide(false /* exit_query_in_omnibox */));
- EXPECT_TRUE(model()->user_input_in_progress());
+ EXPECT_FALSE(model()->user_input_in_progress());
EXPECT_TRUE(view()->IsSelectAll());
#endif
@@ -332,7 +360,7 @@
// Verify we can exit Query in Omnibox mode properly.
EXPECT_TRUE(model()->Unelide(true /* exit_query_in_omnibox */));
EXPECT_EQ(base::ASCIIToUTF16("https://www.example.com/"), view()->GetText());
- EXPECT_TRUE(model()->user_input_in_progress());
+ EXPECT_FALSE(model()->user_input_in_progress());
EXPECT_TRUE(view()->IsSelectAll());
EXPECT_TRUE(model()->CurrentTextIsURL());
diff --git a/components/omnibox/browser/test_omnibox_view.cc b/components/omnibox/browser/test_omnibox_view.cc
index a2a1cda..6250397 100644
--- a/components/omnibox/browser/test_omnibox_view.cc
+++ b/components/omnibox/browser/test_omnibox_view.cc
@@ -54,7 +54,12 @@
const bool text_changed = text_ != display_text;
text_ = display_text;
inline_autocomplete_text_ = display_text.substr(user_text_length);
- selection_ = gfx::Range(text_.size(), user_text_length);
+
+ // Just like the Views control, only change the selection if the text has
+ // actually changed.
+ if (text_changed)
+ selection_ = gfx::Range(text_.size(), user_text_length);
+
return text_changed;
}
