From c67b85c499e7b85007d32bac2ee9b01493825e44 Mon Sep 17 00:00:00 2001 From: "sky@google.com" Date: Thu, 13 Nov 2008 23:09:49 +0000 Subject: Three menu bugs: . If a context menu was shown from a menu, then hidden we did not keep mouse capture and bad things happened (menu wouldn't go away when clicking else where, crash ...). . We only update menu sizes on first show of a menu. That way if a context menu doesn't have icons things don't shift around. . If nothing was selected in the menu pressing the context menu showed the context menu for the root. It doesn't make sense for the root menu to have a context menu. BUG=4364 TEST=see bug Review URL: http://codereview.chromium.org/10706 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@5397 0039d316-1c4b-4281-b951-d872f2087c98 --- chrome/browser/views/bookmark_bar_view_test.cc | 68 ++++++++++++++++++++++++++ chrome/views/chrome_menu.cc | 11 +++-- chrome/views/chrome_menu.h | 3 +- 3 files changed, 76 insertions(+), 6 deletions(-) diff --git a/chrome/browser/views/bookmark_bar_view_test.cc b/chrome/browser/views/bookmark_bar_view_test.cc index c97bf3b..94d9a9c 100644 --- a/chrome/browser/views/bookmark_bar_view_test.cc +++ b/chrome/browser/views/bookmark_bar_view_test.cc @@ -824,3 +824,71 @@ class BookmarkBarViewTest10 : public BookmarkBarViewEventTestBase { }; VIEW_TEST(BookmarkBarViewTest10, KeyEvents) + +// Make sure the menu closes with the following sequence: show menu, show +// context menu, close context menu (via escape), then click else where. This +// effectively verifies we maintain mouse capture after the context menu is +// hidden. +class BookmarkBarViewTest11 : public BookmarkBarViewEventTestBase { + protected: + virtual void DoTestOnMessageLoop() { + // Move the mouse to the first folder on the bookmark bar and press the + // mouse. + views::TextButton* button = bb_view_->other_bookmarked_button(); + ui_controls::MoveMouseToCenterAndPress(button, ui_controls::LEFT, + ui_controls::DOWN | ui_controls::UP, + CreateEventTask(this, &BookmarkBarViewTest11::Step2)); + } + + private: + void Step2() { + // Menu should be showing. + views::MenuItemView* menu = bb_view_->GetMenu(); + ASSERT_TRUE(menu != NULL); + ASSERT_TRUE(menu->GetSubmenu()->IsShowing()); + + views::MenuItemView* child_menu = + menu->GetSubmenu()->GetMenuItemAt(0); + ASSERT_TRUE(child_menu != NULL); + + // Right click on the first child to get its context menu. + ui_controls::MoveMouseToCenterAndPress(child_menu, ui_controls::RIGHT, + ui_controls::DOWN | ui_controls::UP, + CreateEventTask(this, &BookmarkBarViewTest11::Step3)); + } + + void Step3() { + // Send escape so that the context menu hides. + ui_controls::SendKeyPressNotifyWhenDone(VK_ESCAPE, false, false, false, + CreateEventTask(this, &BookmarkBarViewTest11::Step4)); + } + + void Step4() { + // Make sure the context menu is no longer showing. + views::MenuItemView* menu = bb_view_->GetContextMenu(); + ASSERT_TRUE(!menu || !menu->GetSubmenu() || + !menu->GetSubmenu()->IsShowing()); + + // But the menu should be showing. + menu = bb_view_->GetMenu(); + ASSERT_TRUE(menu && menu->GetSubmenu() && menu->GetSubmenu()->IsShowing()); + + // Now click on empty space. + gfx::Point mouse_loc; + views::View::ConvertPointToScreen(bb_view_, &mouse_loc); + ui_controls::SendMouseMove(mouse_loc.x(), mouse_loc.y()); + ui_controls::SendMouseEventsNotifyWhenDone( + ui_controls::LEFT, ui_controls::UP | ui_controls::DOWN, + CreateEventTask(this, &BookmarkBarViewTest11::Step5)); + } + + void Step5() { + // Make sure the menu is not showing. + views::MenuItemView* menu = bb_view_->GetMenu(); + ASSERT_TRUE(!menu || !menu->GetSubmenu() || + !menu->GetSubmenu()->IsShowing()); + Done(); + } +}; + +VIEW_TEST(BookmarkBarViewTest11, CloseMenuAfterClosingContextMenu) diff --git a/chrome/views/chrome_menu.cc b/chrome/views/chrome_menu.cc index 959c442..d1f4eed 100644 --- a/chrome/views/chrome_menu.cc +++ b/chrome/views/chrome_menu.cc @@ -1329,7 +1329,11 @@ void MenuItemView::PrepareForRun(bool has_mnemonics) { AddEmptyMenus(); - UpdateMenuPartSizes(has_icons_); + if (!MenuController::GetActiveInstance()) { + // Only update the menu size if there are no menus showing, otherwise + // things may shift around. + UpdateMenuPartSizes(has_icons_); + } font_ = GetMenuFont(); @@ -1578,8 +1582,6 @@ MenuItemView* MenuController::Run(HWND parent, pending_state_.monitor_bounds = gfx::Rect(mi.rcMonitor); } - did_capture_ = false; - any_menu_contains_mouse_ = false; // Set the selection, which opens the initial menu. @@ -1624,6 +1626,7 @@ MenuItemView* MenuController::Run(HWND parent, menu_stack_.pop_back(); } else { showing_ = false; + did_capture_ = false; } MenuItemView* result = result_; @@ -2045,7 +2048,7 @@ bool MenuController::Dispatch(const MSG& msg) { switch (msg.message) { case WM_CONTEXTMENU: { MenuItemView* item = pending_state_.item; - if (item) { + if (item && item->GetRootMenuItem() != item) { gfx::Point screen_loc(0, item->height()); View::ConvertPointToScreen(item, &screen_loc); item->GetDelegate()->ShowContextMenu( diff --git a/chrome/views/chrome_menu.h b/chrome/views/chrome_menu.h index ec31e35..a92a45c 100644 --- a/chrome/views/chrome_menu.h +++ b/chrome/views/chrome_menu.h @@ -635,7 +635,7 @@ class MenuController : public MessageLoopForUI::Dispatcher { bool update_immediately); // Cancels the current Run. If all is true, any nested loops are canceled - // as well. This immediatley hides all menus. + // as well. This immediately hides all menus. void Cancel(bool all); // An alternative to Cancel(true) that can be used with a OneShotTimer. @@ -949,4 +949,3 @@ class MenuController : public MessageLoopForUI::Dispatcher { } // namespace views #endif // CHROME_VIEWS_CHROME_MENU_H__ - -- cgit v1.1