Support prefix typing in Combobox while menu is open

Support prefix typing in Combobox while menu is open

BUG=343710
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=253170

[Evan Stade, 2014-02-15 02:37:32]

it seems like the PrefixDelegate ought to just be the MenuController, but
PrefixDelegate has to be a View. This is because of how it interacts with IMEs.
Since the MenuController already doesn't support IMEs, this is added complexity
for no win. Perhaps PrefixDelegate should not inherit from View, and
PrefixSelector should take a separate delegate and view parameter (the latter
may be NULL).

[sky, 2014-02-18 17:08:49]

I agree that PrefixDelegate shouldn't be a view. Did you want to this in this
change?

[Evan Stade, 2014-02-18 23:04:06]

On 2014/02/18 17:08:49, sky wrote:
> I agree that PrefixDelegate shouldn't be a view. Did you want to this in this
> change?

I started to make this change and I no longer think it would be less complex to
make the MenuController the PrefixDelegate. All the functions require calling
into the SubmenuView.

I still think the PrefixDelegate should probably not be a View, but that's now
orthogonal to this CL.

[sky, 2014-02-18 23:11:15]

Ok, LGTM

[Evan Stade, 2014-02-19 00:08:21]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-02-19 00:10:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/167643003/1

[commit-bot: I haz the power, 2014-02-19 01:59:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-19 01:59:51]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Evan Stade, 2014-02-19 02:25:07]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-02-19 02:28:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/167643003/1

[commit-bot: I haz the power, 2014-02-19 02:28:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-19 02:28:07]

Failed to apply patch for ui/views/controls/menu/menu_controller.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file ui/views/controls/menu/menu_controller.cc
  Hunk #1 FAILED at 86.
  Hunk #2 succeeded at 2046 (offset 3 lines).
  Hunk #3 succeeded at 2057 (offset 3 lines).
  Hunk #4 succeeded at 2110 (offset 3 lines).
  1 out of 4 hunks FAILED -- saving rejects to file
ui/views/controls/menu/menu_controller.cc.rej

Patch:       ui/views/controls/menu/menu_controller.cc
Index: ui/views/controls/menu/menu_controller.cc
diff --git a/ui/views/controls/menu/menu_controller.cc
b/ui/views/controls/menu/menu_controller.cc
index
480c5aed0b3ad65d9f9499c3d5136e9d03f86fa2..4246e1e2341409b12332ae29f7818c16e4fdc309
100644
--- a/ui/views/controls/menu/menu_controller.cc
+++ b/ui/views/controls/menu/menu_controller.cc
@@ -86,21 +86,6 @@ const int kBubbleTipSizeTopBottom = 11;
 // been showing).
 const float kMaximumLengthMovedToActivate = 4.0f;
 
-// Returns true if the mnemonic of |menu| matches key.
-bool MatchesMnemonic(MenuItemView* menu, base::char16 key) {
-  return menu->GetMnemonic() == key;
-}
-
-// Returns true if |menu| doesn't have a mnemonic and first character of the
its
-// title is |key|.
-bool TitleMatchesMnemonic(MenuItemView* menu, base::char16 key) {
-  if (menu->GetMnemonic())
-    return false;
-
-  base::string16 lower_title = base::i18n::ToLower(menu->title());
-  return !lower_title.empty() && lower_title[0] == key;
-}
-
 }  // namespace
 
 // Returns the first descendant of |view| that is hot tracked.
@@ -2043,8 +2028,7 @@ void MenuController::CloseSubmenu() {
 
 MenuController::SelectByCharDetails MenuController::FindChildForMnemonic(
     MenuItemView* parent,
-    base::char16 key,
-    bool (*match_function)(MenuItemView* menu, base::char16 mnemonic)) {
+    base::char16 key) {
   SubmenuView* submenu = parent->GetSubmenu();
   DCHECK(submenu);
   SelectByCharDetails details;
@@ -2055,7 +2039,7 @@ MenuController::SelectByCharDetails
MenuController::FindChildForMnemonic(
     if (child->enabled() && child->visible()) {
       if (child == pending_state_.item)
         details.index_of_item = i;
-      if (match_function(child, key)) {
+      if (child->GetMnemonic() == key) {
         if (details.first_match == -1)
           details.first_match = i;
         else
@@ -2108,16 +2092,11 @@ bool MenuController::SelectByChar(base::char16
character) {
     return false;
 
   // Look for matches based on mnemonic first.
-  SelectByCharDetails details =
-      FindChildForMnemonic(item, key, &MatchesMnemonic);
-  if (details.first_match != -1)
-    return AcceptOrSelect(item, details);
-
-  // If no mnemonics found, look at first character of titles.
-  details = FindChildForMnemonic(item, key, &TitleMatchesMnemonic);
+  SelectByCharDetails details = FindChildForMnemonic(item, key);
   if (details.first_match != -1)
     return AcceptOrSelect(item, details);
 
+  item->GetSubmenu()->GetTextInputClient()->InsertChar(character, 0);
   return false;
 }

[Evan Stade, 2014-02-20 00:09:19]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-02-20 00:14:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/167643003/490001

[commit-bot: I haz the power, 2014-02-20 03:07:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/167643003/490001

[commit-bot: I haz the power, 2014-02-20 05:23:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/167643003/490001

[commit-bot: I haz the power, 2014-02-20 09:24:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/167643003/490001

[commit-bot: I haz the power, 2014-02-20 12:38:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/167643003/490001

[commit-bot: I haz the power, 2014-02-20 12:51:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-20 12:51:21]

Try jobs failed on following builders: linux_chromium_rel

[Evan Stade, 2014-02-20 20:11:55]

I had to revert some changes so that other menus (e.g. context menus) would
continue to behave in the previous manner. As odd as I think it is, it matches
Windows native menus.

[Evan Stade, 2014-02-21 22:47:20]

On 2014/02/20 20:11:55, Evan Stade wrote:
> I had to revert some changes so that other menus (e.g. context menus) would
> continue to behave in the previous manner. As odd as I think it is, it matches
> Windows native menus.

ping sky^, PTAL

[sky, 2014-02-21 23:39:12]

Is it possible to have combobox handle some of this? Having Submenuview
implement mnemonic lookup differently for comboboxs feels wrong.

[Evan Stade, 2014-02-24 19:31:12]

On 2014/02/21 23:39:12, sky wrote:
> Is it possible to have combobox handle some of this? Having Submenuview
> implement mnemonic lookup differently for comboboxs feels wrong.

Well, comboboxes don't really have mnemonics. It's the fallback---what to do in
case of no mnemonic---which is different for comboboxes vs menus.

I don't think combobox can handle this because it has no concept of the list of
menu items, its scroll offset, the hover selection, etc.

I do think it's weird that MenuController handles some input and SubmenuView
handles other input. Perhaps SelectByChar should be moved into SubmenuView (this
would also fix the n^2 loop in FindChildForMnemonic --- GetMenuItemAt is an O(n)
operation).

[sky, 2014-02-24 20:22:53]

Ok, go with this for now then LGTM

[Evan Stade, 2014-02-24 20:38:31]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-02-24 20:39:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/167643003/860001

[commit-bot: I haz the power, 2014-02-24 21:18:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/167643003/860001

[commit-bot: I haz the power, 2014-02-24 22:23:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-24 22:23:20]

Retried try job too often on android_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[Evan Stade, 2014-02-25 01:23:36]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-02-25 02:06:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/167643003/860001

[commit-bot: I haz the power, 2014-02-25 07:38:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/167643003/860001

[commit-bot: I haz the power, 2014-02-25 09:12:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/167643003/860001

[commit-bot: I haz the power, 2014-02-25 09:31:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/167643003/860001

[commit-bot: I haz the power, 2014-02-25 13:37:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/167643003/860001

[commit-bot: I haz the power, 2014-02-25 15:31:21]

Change committed as 253170