[MacViews] Show combobox menu popup at mouse press

ui/views/controls/combobox/combobox.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/views/controls/combobox/combobox.h"
 
 #include <stddef.h>
 
 #include <utility>
 
@@ skipping 92 matching lines @@
   return bounds;
 }
 
 // The transparent button which holds a button state but is not rendered.
 class TransparentButton : public CustomButton {
  public:
   TransparentButton(ButtonListener* listener)
       : 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();
   }
 
  private:
   DISALLOW_COPY_AND_ASSIGN(TransparentButton);
 };
 
 // Returns the next or previous valid index (depending on |increment|'s value).
@@ skipping 764 matching lines @@
 }
 
 int Combobox::GetArrowContainerWidth() const {
   int padding = style_ == STYLE_NORMAL
                     ? PlatformStyle::kComboboxNormalArrowPadding * 2
                     : kActionLeftPadding + kActionRightPadding;
   return ArrowSize().width() + padding;
 }
 
 }  // namespace views