[MacViews] Show combobox menu popup at mouse press

ui/views/controls/combobox/combobox.cc

Index: ui/views/controls/combobox/combobox.cc
diff --git a/ui/views/controls/combobox/combobox.cc b/ui/views/controls/combobox/combobox.cc
index 3d7cfd1e26585dae22ab9fd8f8cfe5d876987304..9c5b6f61da07fd6ba516a6bed0fd70b8ebeb13de 100644
--- a/ui/views/controls/combobox/combobox.cc
+++ b/ui/views/controls/combobox/combobox.cc
@@ -110,13 +110,17 @@ class TransparentButton : public CustomButton {
       : CustomButton(listener) {
     SetAnimationDuration(LabelButton::kHoverAnimationDurationMs);
     SetFocusBehavior(FocusBehavior::NEVER);
+    set_notify_action(PlatformStyle::kMenuActivationAction);
   }
   ~TransparentButton() override {}
 
+#if defined(OS_MACOSX)
   bool OnMousePressed(const ui::MouseEvent& mouse_event) override {

[tapted, 2016/06/23 00:54:29]

nit: comment here like
  // Override OnMousePressed() to transfer focus the the parent() on a click,
except on Mac which doesn't transfer focus when buttons are clicked.

there might be an alternative that involves setting request_focus_on_press, then
checking CustomButton::request_focus_on_press() inside
TransparentButton::OnMousePressed(), but then there's the issue of whether
OnMousePressed should invoke super::OnMousePressed at all, since it will
interpret request_focus_on_press a second time.

We could also add a PlatformStyle setting for this, and update
CustomButton::set_request_focus_on_press..

perhaps raise this with sky@

I think this is OK, since TransparentButton is a little odd anyway. And, e.g.,
having these child views might be messing up a11y hit tests, as well as
intercepting these mouse presses.


But! This is all pretty subtle - more subtle than I first realised - sorry!. We
should update

TEST_F(ComboboxTest, NotifyOnClickWithMouse) {

to test the platform specific stuff. It currently uses PerformClick() which does
both a mouse press and a mouse up. I think we can rewite PerformClick as {
  PerformMousePress();
  PerformMouseRelease();
}

then have NotifyOnClickWithMouse call PerformMouse/Release with checks in
between for Mac and not-Mac to validate the platform differences.

[spqchan, 2016/06/28 17:05:59]

Added the comment in. I'm a bit confused with what you mean, should we leave
parent()->RequestFocus() as it is or not?

I updated the test. I noticed that other tests also check for the menu after
PerformClick() is called. Should we make changes to them as well?

[tapted, 2016/06/29 12:02:55]

Yes - it should stay (just not on Mac). Sorry - had a typo in my sample comment.
I think PerformClick() is OK coverage for most - we don't want to litter the
test file with #ifdefs

     parent()->RequestFocus();
+    CustomButton::OnMousePressed(mouse_event);
     return true;
   }
+#endif
 
   double GetAnimationValue() const {
     return hover_animation().GetCurrentValue();