Open the system menu below the window titlebar/tabstrip.

This also moves ShowSystemMenuAtPoint() from gfx (why was it there?) to views.

BUG=none
TEST=Hit alt-space; system menu should appear below the tabstrip.

Change-Id: Iba2ca9fc54746f253ef7d61ed5b190d61de47f5f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1049063Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Auto-Submit: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#670348}

chrome/browser/ui/views/frame/browser_non_client_frame_view.cc

@@ -379,6 +379,15 @@ void BrowserNonClientFrameView::OnProfileHighResAvatarLoaded(
 #endif
 }
 
+#if defined(OS_WIN)
+int BrowserNonClientFrameView::GetSystemMenuY() const {
+  if (!browser_view()->IsTabStripVisible())
+    return GetTopInset(false);
+  return GetBoundsForTabStripRegion(browser_view()->tabstrip()).bottom() -
+         GetLayoutConstant(TABSTRIP_TOOLBAR_OVERLAP);
+}
+#endif
+
 void BrowserNonClientFrameView::MaybeObserveTabstrip() {
   if (browser_view_->tabstrip()) {
     DCHECK(!tab_strip_observer_.IsObserving(browser_view_->tabstrip()));

chrome/browser/ui/views/frame/browser_non_client_frame_view.h

@@ -172,6 +172,11 @@ class BrowserNonClientFrameView : public views::NonClientFrameView,
   }
 
  private:
+  // views::NonClientFrameView:
+#if defined(OS_WIN)
+  int GetSystemMenuY() const override;
+#endif
+
   void MaybeObserveTabstrip();
 
   // Gets a theme provider that should be non-null even before we're added to a

chrome/browser/ui/views/tabs/new_tab_button.cc

@@ -32,7 +32,6 @@
 
 #if defined(OS_WIN)
 #include "ui/display/win/screen_win.h"
-#include "ui/gfx/win/hwnd_util.h"
 #include "ui/views/win/hwnd_util.h"
 #endif
 
@@ -152,12 +151,14 @@ void NewTabButton::OnMouseReleased(const ui::MouseEvent& event) {
     return;
   }
 
+  // TODO(pkasting): If we handled right-clicks on the frame, and we made sure
+  // this event was not handled, it seems like things would Just Work.
   gfx::Point point = event.location();
   views::View::ConvertPointToScreen(this, &point);
   point = display::win::ScreenWin::DIPToScreenPoint(point);
   bool destroyed = false;
   destroyed_ = &destroyed;
-  gfx::ShowSystemMenuAtPoint(views::HWNDForView(this), point);
+  views::ShowSystemMenuAtScreenPixelLocation(views::HWNDForView(this), point);
   if (!destroyed)
     SetState(views::Button::STATE_NORMAL);
 }

ui/base/ime/win/input_method_win_base.cc

@@ -26,7 +26,6 @@
 #include "ui/events/event_constants.h"
 #include "ui/events/event_utils.h"
 #include "ui/events/keycodes/keyboard_codes.h"
-#include "ui/gfx/win/hwnd_util.h"
 
 namespace ui {
 namespace {
@@ -297,12 +296,6 @@ LRESULT InputMethodWinBase::OnChar(HWND window_handle,
     }
   }
 
-  // Explicitly show the system menu at a good location on [Alt]+[Space].
-  // Note: Setting |handled| to FALSE for DefWindowProc triggering of the system
-  //       menu causes undesirable titlebar artifacts in the classic theme.
-  if (message == WM_SYSCHAR && wparam == VK_SPACE)
-    gfx::ShowSystemMenu(window_handle);
-
   return 0;
 }
 

ui/gfx/win/hwnd_util.cc

@@ -4,10 +4,8 @@
 
 #include "ui/gfx/win/hwnd_util.h"
 
-#include "base/i18n/rtl.h"
 #include "base/strings/string_util.h"
 #include "base/win/win_util.h"
-#include "ui/gfx/geometry/point.h"
 #include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/geometry/size.h"
 
@@ -200,29 +198,6 @@ void CheckWindowCreated(HWND hwnd) {
   }
 }
 
-void ShowSystemMenu(HWND window) {
-  RECT rect;
-  GetWindowRect(window, &rect);
-  Point point = Point(base::i18n::IsRTL() ? rect.right : rect.left, rect.top);
-  static const int kSystemMenuOffset = 10;
-  point.Offset(base::i18n::IsRTL() ? -kSystemMenuOffset : kSystemMenuOffset,
-               kSystemMenuOffset);
-  ShowSystemMenuAtPoint(window, point);
-}
-
-void ShowSystemMenuAtPoint(HWND window, const Point& point) {
-  UINT flags = TPM_LEFTBUTTON | TPM_RIGHTBUTTON | TPM_RETURNCMD;
-  if (base::i18n::IsRTL())
-    flags |= TPM_RIGHTALIGN;
-  HMENU menu = GetSystemMenu(window, FALSE);
-
-  const int command =
-      TrackPopupMenu(menu, flags, point.x(), point.y(), 0, window, NULL);
-
-  if (command)
-    SendMessage(window, WM_SYSCOMMAND, command, 0);
-}
-
 extern "C" {
   typedef HWND (*RootWindow)();
 }

ui/gfx/win/hwnd_util.h

@@ -11,7 +11,6 @@
 #include "ui/gfx/gfx_export.h"
 
 namespace gfx {
-class Point;
 class Size;
 
 // A version of the GetClassNameW API that returns the class name in an
@@ -40,10 +39,6 @@ GFX_EXPORT void CenterAndSizeWindow(HWND parent,
 // CreateWindow.
 GFX_EXPORT void CheckWindowCreated(HWND hwnd);
 
-// Shows |window|'s system menu (at a specified |point| in screen coordinates).
-GFX_EXPORT void ShowSystemMenu(HWND window);
-GFX_EXPORT void ShowSystemMenuAtPoint(HWND window, const gfx::Point& point);
-
 // Returns the window you can use to parent a top level window.
 // Note that in some cases we create child windows not parented to its final
 // container so in those cases you should pass true in |get_real_hwnd|.

ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc

@@ -900,6 +900,16 @@ bool DesktopWindowTreeHostWin::HandleMouseEvent(ui::MouseEvent* event) {
 }
 
 void DesktopWindowTreeHostWin::HandleKeyEvent(ui::KeyEvent* event) {
+  // Bypass normal handling of alt-space, which would otherwise consume the
+  // corresponding WM_SYSCHAR.  This allows HandleIMEMessage() to show the
+  // system menu in this case.  If we instead showed the system menu here, the
+  // WM_SYSCHAR would trigger a beep when processed by the native event handler.
+  if ((event->type() == ui::ET_KEY_PRESSED) &&
+      (event->key_code() == ui::VKEY_SPACE) &&
+      (event->flags() & ui::EF_ALT_DOWN) && GetWidget()->non_client_view()) {
+    return;
+  }
+
   SendEventToSink(event);
 }
 
@@ -935,6 +945,15 @@ bool DesktopWindowTreeHostWin::HandleIMEMessage(UINT message,
                                                 WPARAM w_param,
                                                 LPARAM l_param,
                                                 LRESULT* result) {
+  // Show the system menu at an appropriate location on alt-space.
+  if ((message == WM_SYSCHAR) && (w_param == VK_SPACE) &&
+      GetWidget()->non_client_view()) {
+    const auto* frame = GetWidget()->non_client_view()->frame_view();
+    ShowSystemMenuAtScreenPixelLocation(
+        GetHWND(), frame->GetSystemMenuScreenPixelLocation());
+    return true;
+  }
+
   MSG msg = {};
   msg.hwnd = GetHWND();
   msg.message = message;

ui/views/widget/widget.cc

@@ -1637,8 +1637,8 @@ void Widget::UnlockPaintAsActive() {
 }
 
 void Widget::UpdatePaintAsActiveState(bool paint_as_active) {
-  if (non_client_view())
-    non_client_view()->frame_view()->PaintAsActiveChanged(paint_as_active);
+  if (non_client_view_)
+    non_client_view_->frame_view()->PaintAsActiveChanged(paint_as_active);
   if (widget_delegate())
     widget_delegate()->OnPaintAsActiveChanged(paint_as_active);
 }

ui/views/win/hwnd_message_handler.cc

@@ -61,6 +61,7 @@
 #include "ui/views/widget/widget_hwnd_utils.h"
 #include "ui/views/win/fullscreen_handler.h"
 #include "ui/views/win/hwnd_message_handler_delegate.h"
+#include "ui/views/win/hwnd_util.h"
 #include "ui/views/win/scoped_fullscreen_visibility.h"
 
 namespace views {
@@ -2837,6 +2838,8 @@ LRESULT HWNDMessageHandler::HandleMouseEventInternal(UINT message,
   }
 
   if (message == WM_RBUTTONUP && is_right_mouse_pressed_on_caption_) {
+    // TODO(pkasting): Maybe handle this in DesktopWindowTreeHostWin, where we
+    // handle alt-space, or in the frame itself.
     is_right_mouse_pressed_on_caption_ = false;
     ReleaseCapture();
     // |point| is in window coordinates, but WM_NCHITTEST and TrackPopupMenu()
@@ -2846,7 +2849,7 @@ LRESULT HWNDMessageHandler::HandleMouseEventInternal(UINT message,
     w_param = SendMessage(hwnd(), WM_NCHITTEST, 0,
                           MAKELPARAM(screen_point.x, screen_point.y));
     if (w_param == HTCAPTION || w_param == HTSYSMENU) {
-      gfx::ShowSystemMenuAtPoint(hwnd(), gfx::Point(screen_point));
+      ShowSystemMenuAtScreenPixelLocation(hwnd(), gfx::Point(screen_point));
       return 0;
     }
   } else if (message == WM_NCLBUTTONDOWN &&

ui/views/win/hwnd_util.h

@@ -5,6 +5,7 @@
 #ifndef UI_VIEWS_WIN_HWND_UTIL_H_
 #define UI_VIEWS_WIN_HWND_UTIL_H_
 
+#include "ui/gfx/geometry/point.h"
 #include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/native_widget_types.h"
 #include "ui/views/views_export.h"
@@ -28,6 +29,12 @@ VIEWS_EXPORT HWND HWNDForNativeWindow(const gfx::NativeWindow window);
 
 VIEWS_EXPORT gfx::Rect GetWindowBoundsForClientBounds(
     View* view, const gfx::Rect& client_bounds);
+
+// Shows |window|'s system menu (at a specified |point| in screen physical
+// coordinates).
+VIEWS_EXPORT void ShowSystemMenuAtScreenPixelLocation(HWND window,
+                                                      const gfx::Point& point);
+
 }  // namespace views
 
 #endif  // UI_VIEWS_WIN_HWND_UTIL_H_

ui/views/win/hwnd_util_aurawin.cc

@@ -4,6 +4,7 @@
 
 #include "ui/views/win/hwnd_util.h"
 
+#include "base/i18n/rtl.h"
 #include "ui/aura/window.h"
 #include "ui/aura/window_tree_host.h"
 #include "ui/views/widget/widget.h"
@@ -44,4 +45,17 @@ gfx::Rect GetWindowBoundsForClientBounds(View* view,
   return client_bounds;
 }
 
+void ShowSystemMenuAtScreenPixelLocation(HWND window, const gfx::Point& point) {
+  UINT flags = TPM_LEFTALIGN | TPM_TOPALIGN | TPM_RIGHTBUTTON | TPM_RETURNCMD;
+  if (base::i18n::IsRTL())
+    flags |= TPM_RIGHTALIGN;
+  HMENU menu = GetSystemMenu(window, FALSE);
+
+  const int command =
+      TrackPopupMenu(menu, flags, point.x(), point.y(), 0, window, nullptr);
+
+  if (command)
+    SendMessage(window, WM_SYSCOMMAND, command, 0);
+}
+
 }  // namespace views

ui/views/window/non_client_view.cc

@@ -15,6 +15,10 @@
 #include "ui/views/widget/widget.h"
 #include "ui/views/window/client_view.h"
 
+#if defined(OS_WIN)
+#include "ui/display/win/screen_win.h"
+#endif
+
 namespace views {
 
 // The frame view and the client view are always at these specific indices,
@@ -33,6 +37,17 @@ bool NonClientFrameView::GetClientMask(const gfx::Size& size,
   return false;
 }
 
+#if defined(OS_WIN)
+gfx::Point NonClientFrameView::GetSystemMenuScreenPixelLocation() const {
+  gfx::Point point(GetMirroredXInView(GetBoundsForClientView().x()),
+                   GetSystemMenuY());
+  View::ConvertPointToScreen(this, &point);
+  point = display::win::ScreenWin::DIPToScreenPoint(point);
+  // The native system menu seems to overlap the titlebar by 1 px.  Match that.
+  return point - gfx::Vector2d(0, 1);
+}
+#endif
+
 ////////////////////////////////////////////////////////////////////////////////
 // NonClientView, public:
 
@@ -322,6 +337,15 @@ bool NonClientFrameView::DoesIntersectRect(const View* target,
   return !GetWidget()->client_view()->bounds().Intersects(rect);
 }
 
+////////////////////////////////////////////////////////////////////////////////
+// NonClientFrameView, private:
+
+#if defined(OS_WIN)
+int NonClientFrameView::GetSystemMenuY() const {
+  return GetBoundsForClientView().y();
+}
+#endif
+
 BEGIN_METADATA(NonClientFrameView)
 METADATA_PARENT_CLASS(View)
 END_METADATA()

ui/views/window/non_client_view.h

@@ -6,6 +6,7 @@
 #define UI_VIEWS_WINDOW_NON_CLIENT_VIEW_H_
 
 #include "base/macros.h"
+#include "build/build_config.h"
 #include "ui/views/view.h"
 #include "ui/views/view_targeter_delegate.h"
 
@@ -65,6 +66,12 @@ class VIEWS_EXPORT NonClientFrameView : public View,
   // used.
   virtual bool GetClientMask(const gfx::Size& size, SkPath* mask) const;
 
+#if defined(OS_WIN)
+  // Returns the point in screen physical coordinates at which the system menu
+  // should be opened.
+  virtual gfx::Point GetSystemMenuScreenPixelLocation() const;
+#endif
+
   // This function must ask the ClientView to do a hittest.  We don't do this in
   // the parent NonClientView because that makes it more difficult to calculate
   // hittests for regions that are partially obscured by the ClientView, e.g.
@@ -98,6 +105,13 @@ class VIEWS_EXPORT NonClientFrameView : public View,
                          const gfx::Rect& rect) const override;
 
  private:
+#if defined(OS_WIN)
+  // Returns the y coordinate, in local coordinates, at which the system menu
+  // should be opened.  Since this is in DIP, it does not include the 1 px
+  // offset into the caption area; the caller will take care of this.
+  virtual int GetSystemMenuY() const;
+#endif
+
   DISALLOW_COPY_AND_ASSIGN(NonClientFrameView);
 };