Omnibox: Prevent ctrl-enter behavior when we believe the ctrl key was pressed...

Omnibox: Prevent ctrl-enter behavior when we believe the ctrl key was pressed in order to perform another action.

For example, if the user presses ctrl-a to select all the text in the omnibox, then presses enter without releasing ctrl, we do not perform ctrl-enter behavior. On the other hand, if the user releases and represses ctrl after performing a ctrl action, we do perform ctrl-enter behavior.

This cl does two changes for the above goal.
1. Previously, ctrl-enter was disabled when the omnibox is focused via ctrl-l but not when the omnibox was already focused and the user presses ctrl-l. Now, we disable ctrl-enter in both cases.
2. Previously, ctrl-enter was disabled if a ctrl action caused either the omnibox selection or text to change; e.g. ctrl-v when the clipboard is non-empty causes a text change, and ctrl-a when the omnibox is non-empty causes a selection change. However, ctrl-enter was not disabled if the ctrl-action did not cause a selction or text change; e.g. ctrl-c, ctrl-v when the clipboard is empty, ctrl-a when the entire omnibox text is already selected, ctrl-left when no omnibox text is selected, etc. Now we disable ctrl-enter if an action was executed, regardless of whether that action caused the omnibox text or selection to change. For example, if text selection is not empty, ctrl-c will disable ctrl-enter, but if selection is empty, ctrl-c will not disable ctrl-enter. Ctrl-left/right/home/end are the 1 notable exception, where they will disable ctrl-enter regardless of whether the cursor was already at either edge.

Bug: 75715

Change-Id: I8b2b4829697bf66855879366e4429494c3e95ab9
Reviewed-on: https://chromium-review.googlesource.com/1173424
Commit-Queue: manuk hovanesian <manukh@chromium.org>
Reviewed-by: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583640}

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

@@ -317,6 +317,11 @@ void OmniboxViewViews::SetFocus() {
   // OmniboxEditModel::OnSetFocus(), which handles restoring visibility when the
   // omnibox regains focus after losing focus.
   model()->SetCaretVisibility(true);
+  // If the user attempts to focus the omnibox, and the ctrl key is pressed, we
+  // want to prevent ctrl-enter behavior until the ctrl key is released and
+  // re-pressed. This occurs even if the omnibox is already focused and we
+  // re-request focus (e.g. pressing ctrl-l twice).
+  model()->ConsumeCtrlKey();
 }
 
 int OmniboxViewViews::GetTextWidth() const {

components/omnibox/browser/omnibox_edit_model.cc

@@ -491,7 +491,7 @@ void OmniboxEditModel::AcceptInput(WindowOpenDisposition disposition,
   // typed. If we can successfully generate a URL_WHAT_YOU_TYPED match doing
   // that, then we use this. These matches are marked as generated by the
   // HistoryURLProvider so we only generate them if this provider is present.
-  if (control_key_state_ == DOWN_WITHOUT_CHANGE && !is_keyword_selected() &&
+  if (control_key_state_ == DOWN && !is_keyword_selected() &&
       autocomplete_controller()->history_url_provider()) {
     // Generate a new AutocompleteInput, copying the latest one but using "com"
     // as the desired TLD. Then use this autocomplete input to generate a
@@ -912,7 +912,10 @@ void OmniboxEditModel::OnSetFocus(bool control_down) {
   // focus can be regained without an accompanying call to
   // OmniboxView::SetFocus(), e.g. by tabbing in.
   SetFocusState(OMNIBOX_FOCUS_VISIBLE, OMNIBOX_FOCUS_CHANGE_EXPLICIT);
-  control_key_state_ = control_down ? DOWN_WITHOUT_CHANGE : UP;
+  // On focusing the omnibox, if the ctrl key is pressed, we don't want to
+  // trigger ctrl-enter behavior unless it is released and re-pressed. For
+  // example, if the user presses ctrl-l to focus the omnibox.
+  control_key_state_ = control_down ? DOWN_AND_CONSUMED : UP;
 
   // Try to get ZeroSuggest suggestions if a page is loaded and the user has
   // not been typing in the omnibox.  The |user_input_in_progress_| check is
@@ -945,6 +948,11 @@ void OmniboxEditModel::SetCaretVisibility(bool visible) {
   }
 }
 
+void OmniboxEditModel::ConsumeCtrlKey() {
+  if (control_key_state_ == DOWN)
+    control_key_state_ = DOWN_AND_CONSUMED;
+}
+
 void OmniboxEditModel::OnWillKillFocus() {
   if (user_input_in_progress_ || !in_revert_)
     client_->OnInputStateChanged();
@@ -1007,7 +1015,7 @@ bool OmniboxEditModel::OnEscapeKeyPressed() {
 
 void OmniboxEditModel::OnControlKeyChanged(bool pressed) {
   if (pressed == (control_key_state_ == UP))
-    control_key_state_ = pressed ? DOWN_WITHOUT_CHANGE : UP;
+    control_key_state_ = pressed ? DOWN : UP;
 }
 
 void OmniboxEditModel::OnPaste() {
@@ -1077,16 +1085,15 @@ void OmniboxEditModel::OnPopupDataChanged(
       inline_autocomplete_text_.clear();
       view_->OnInlineAutocompleteTextCleared();
     }
-    if (control_key_state_ == DOWN_WITHOUT_CHANGE) {
-      // Arrowing around the popup cancels control-enter.
-      control_key_state_ = DOWN_WITH_CHANGE;
-      // Now things are a bit screwy: the desired_tld has changed, but if we
-      // update the popup, the new order of entries won't match the old, so the
-      // user's selection gets screwy; and if we don't update the popup, and the
-      // user reverts, then the selected item will be as if control is still
-      // pressed, even though maybe it isn't any more.  There is no obvious
-      // right answer here :(
-    }
+    // Arrowing around the popup cancels control-enter.
+    ConsumeCtrlKey();
+    // Now things are a bit screwy: the desired_tld has changed, but if we
+    // update the popup, the new order of entries won't match the old, so the
+    // user's selection gets screwy; and if we don't update the popup, and the
+    // user reverts, then the selected item will be as if control is still
+    // pressed, even though maybe it isn't any more.  There is no obvious
+    // right answer here :(
+
     const AutocompleteMatch& match = CurrentMatch(nullptr);
     view_->OnTemporaryTextMaybeChanged(MaybeStripKeyword(text), match,
                                        save_original_selection, true);
@@ -1167,11 +1174,10 @@ bool OmniboxEditModel::OnAfterPossibleChange(
     SetFocusState(OMNIBOX_FOCUS_VISIBLE, OMNIBOX_FOCUS_CHANGE_TYPING);
   }
 
-  // If something has changed while the control key is down, prevent
-  // "ctrl-enter" until the control key is released.
-  if ((state_changes.text_differs || state_changes.selection_differs) &&
-      (control_key_state_ == DOWN_WITHOUT_CHANGE))
-    control_key_state_ = DOWN_WITH_CHANGE;
+  // When the user performs an action with the ctrl key pressed down, we assume
+  // the ctrl key was intended for that action. If they then press enter without
+  // releasing the ctrl key, we prevent "ctrl-enter" behavior.
+  ConsumeCtrlKey();
 
   // If the user text does not need to be changed, return now, so we don't
   // change any other state, lest arrowing around the omnibox do something like

components/omnibox/browser/omnibox_edit_model.h

@@ -290,6 +290,10 @@ class OmniboxEditModel {
   // switching tabs.
   void SetCaretVisibility(bool visible);
 
+  // If the ctrl key is down, marks it as consumed to prevent it from triggering
+  // ctrl-enter behavior unless it is released and re-pressed.
+  void ConsumeCtrlKey();
+
   // Sent before |OnKillFocus| and before the popup is closed.
   void OnWillKillFocus();
 
@@ -360,6 +364,9 @@ class OmniboxEditModel {
 
  private:
   friend class OmniboxControllerTest;
+  FRIEND_TEST_ALL_PREFIXES(OmniboxEditModelTest, ConsumeCtrlKey);
+  FRIEND_TEST_ALL_PREFIXES(OmniboxEditModelTest, ConsumeCtrlKeyOnRequestFocus);
+  FRIEND_TEST_ALL_PREFIXES(OmniboxEditModelTest, ConsumeCtrlKeyOnCtrlAction);
 
   enum PasteState {
     NONE,           // Most recent edit was not a paste.
@@ -373,17 +380,14 @@ class OmniboxEditModel {
   };
 
   enum ControlKeyState {
-    UP,                   // The control key is not depressed.
-    DOWN_WITHOUT_CHANGE,  // The control key is depressed, and the edit's
-                          // contents/selection have not changed since it was
-                          // depressed.  This is the only state in which we
-                          // do the "ctrl-enter" behavior when the user hits
-                          // enter.
-    DOWN_WITH_CHANGE,     // The control key is depressed, and the edit's
-                          // contents/selection have changed since it was
-                          // depressed.  If the user now hits enter, we assume
-                          // they simply haven't released the key, rather than
-                          // that they intended to hit "ctrl-enter".
+    UP,                // The control key is not depressed.
+    DOWN,              // The control key is depressed and should trigger the
+                       // "ctrl-enter" behavior when the user hits enter.
+    DOWN_AND_CONSUMED  // The control key is depressed, but has been consumed
+                       // and should not trigger the "ctrl-enter" behavior.
+                       // The control key becomes consumed if it has been used
+                       // for another action such as focusing the location bar
+                       // with ctrl-l or copying the selected text with ctrl-c.
   };
 
   // Returns true if a query to an autocomplete provider is currently

components/omnibox/browser/omnibox_edit_model_unittest.cc

@@ -261,3 +261,38 @@ TEST_F(OmniboxEditModelTest, IgnoreInvalidSavedFocusStates) {
   EXPECT_TRUE(model()->has_focus());
   EXPECT_TRUE(model()->is_caret_visible());
 }
+
+// Tests ConsumeCtrlKey() consumes ctrl key when down, but does not affect ctrl
+// state otherwise.
+TEST_F(OmniboxEditModelTest, ConsumeCtrlKey) {
+  model()->control_key_state_ = TestOmniboxEditModel::UP;
+  model()->ConsumeCtrlKey();
+  EXPECT_EQ(model()->control_key_state_, TestOmniboxEditModel::UP);
+  model()->control_key_state_ = TestOmniboxEditModel::DOWN;
+  model()->ConsumeCtrlKey();
+  EXPECT_EQ(model()->control_key_state_,
+            TestOmniboxEditModel::DOWN_AND_CONSUMED);
+  model()->ConsumeCtrlKey();
+  EXPECT_EQ(model()->control_key_state_,
+            TestOmniboxEditModel::DOWN_AND_CONSUMED);
+}
+
+// Tests ctrl_key_state_ is set consumed if the ctrl key is down on focus.
+TEST_F(OmniboxEditModelTest, ConsumeCtrlKeyOnRequestFocus) {
+  model()->control_key_state_ = TestOmniboxEditModel::DOWN;
+  model()->OnSetFocus(false);
+  EXPECT_EQ(model()->control_key_state_, TestOmniboxEditModel::UP);
+  model()->OnSetFocus(true);
+  EXPECT_EQ(model()->control_key_state_,
+            TestOmniboxEditModel::DOWN_AND_CONSUMED);
+}
+
+// Tests the ctrl key is consumed on a ctrl-action (e.g. ctrl-c to copy)
+TEST_F(OmniboxEditModelTest, ConsumeCtrlKeyOnCtrlAction) {
+  model()->control_key_state_ = TestOmniboxEditModel::DOWN;
+  OmniboxView::StateChanges state_changes{nullptr, nullptr, 0,     0,
+                                          false,   false,   false, false};
+  model()->OnAfterPossibleChange(state_changes, false);
+  EXPECT_EQ(model()->control_key_state_,
+            TestOmniboxEditModel::DOWN_AND_CONSUMED);
+}