diff options
author | jonross <jonross@chromium.org> | 2016-02-01 06:38:53 -0800 |
---|---|---|
committer | Commit bot <commit-bot@chromium.org> | 2016-02-01 14:40:37 +0000 |
commit | 5a2c3579e35e5141546dc8e31b7682f9ea59b052 (patch) | |
tree | 2d78c2ccaf4c1c229037a5824e729308df8e1f00 /ui | |
parent | 641834fcfaace0131dc6c5ecdf6ec9b0923f09b6 (diff) | |
download | chromium_src-5a2c3579e35e5141546dc8e31b7682f9ea59b052.zip chromium_src-5a2c3579e35e5141546dc8e31b7682f9ea59b052.tar.gz chromium_src-5a2c3579e35e5141546dc8e31b7682f9ea59b052.tar.bz2 |
Merge: Update reposting of events from menus
Update reposting of events from menus
This change addresses a potential crash in MenuController::RepostEvent, along with a regression in touch event reposting on Chrome OS.
In MenuController::SetSelectionOnPointerDown the call of Cancel can lead to the deletion of the source of the event. RepostEvent can crash while attempting to transform the event location to screen coordinates. This change performs the transform before cancelling.
In MenuController::OnTouchEvent a call was added to RepostEvent. On Windows this eventually leads to the closing of the menu after capture is released. However this does not occur on Chrome OS, leading to the menu never closing. This change refactors out the Repost/Cancel logic from SetSelectionOnPointerDown into RepostEventAndCancel, providing one location to handle the reposting and shutdown logic differences of Windows and Chrome OS. SetSelectionOnPointerDown and OnTouchEvent now use this.
TEST=MenuControllerTest.AsynchronousRepostEvent, MenuControllerTest.AsynchronousTouchEventRepostEvent, as well as manual testing on a Chromebook to confirm touch regression has been fixed.
BUG=557130
NOTRY=true
NOPRESUBMIT=true
TBR=sky@chromium.org
Review URL: https://codereview.chromium.org/1586353004
Cr-Commit-Position: refs/heads/master@{#370143}
(cherry picked from commit 580431e42ab952e84b32e9244acb252b1a76c575)
Review URL: https://codereview.chromium.org/1644363002
Cr-Commit-Position: refs/branch-heads/2623@{#214}
Cr-Branched-From: 92d77538a86529ca35f9220bd3cd512cbea1f086-refs/heads/master@{#369907}
Diffstat (limited to 'ui')
-rw-r--r-- | ui/views/controls/menu/menu_controller.cc | 86 | ||||
-rw-r--r-- | ui/views/controls/menu/menu_controller.h | 10 | ||||
-rw-r--r-- | ui/views/controls/menu/menu_controller_unittest.cc | 85 |
3 files changed, 142 insertions, 39 deletions
diff --git a/ui/views/controls/menu/menu_controller.cc b/ui/views/controls/menu/menu_controller.cc index d16553b..10b2cd9 100644 --- a/ui/views/controls/menu/menu_controller.cc +++ b/ui/views/controls/menu/menu_controller.cc @@ -679,7 +679,7 @@ void MenuController::OnTouchEvent(SubmenuView* source, ui::TouchEvent* event) { if (event->type() == ui::ET_TOUCH_PRESSED) { MenuPart part = GetMenuPart(source, event->location()); if (part.type == MenuPart::NONE) { - RepostEvent(source, event); + RepostEventAndCancel(source, event); event->SetHandled(); } } @@ -1001,36 +1001,8 @@ void MenuController::SetSelectionOnPointerDown(SubmenuView* source, // then use this to figure out if this menu was finished with the same click // which is sent to it thereafter. closing_event_time_ = event->time_stamp(); - - // Mouse wasn't pressed over any menu, or the active menu, cancel. - -#if defined(OS_WIN) - // We're going to close and we own the mouse capture. We need to repost the - // mouse down, otherwise the window the user clicked on won't get the event. - RepostEvent(source, event); -#endif - - // And close. - ExitType exit_type = EXIT_ALL; - if (!menu_stack_.empty()) { - // We're running nested menus. Only exit all if the mouse wasn't over one - // of the menus from the last run. - gfx::Point screen_loc(event->location()); - View::ConvertPointToScreen(source->GetScrollViewContainer(), &screen_loc); - MenuPart last_part = GetMenuPartByScreenCoordinateUsingMenu( - menu_stack_.back().first.item, screen_loc); - if (last_part.type != MenuPart::NONE) - exit_type = EXIT_OUTERMOST; - } - Cancel(exit_type); - -#if defined(OS_CHROMEOS) - // We're going to exit the menu and want to repost the event so that is - // is handled normally after the context menu has exited. We call - // RepostEvent after Cancel so that mouse capture has been released so - // that finding the event target is unaffected by the current capture. - RepostEvent(source, event); -#endif + // Event wasn't pressed over any menu, or the active menu, cancel. + RepostEventAndCancel(source, event); // Do not repost events for Linux Aura because this behavior is more // consistent with the behavior of other Linux apps. return; @@ -2188,7 +2160,10 @@ void MenuController::SelectByChar(base::char16 character) { } void MenuController::RepostEvent(SubmenuView* source, - const ui::LocatedEvent* event) { + const ui::LocatedEvent* event, + const gfx::Point& screen_loc, + gfx::NativeView native_view, + gfx::NativeWindow window) { if (!event->IsMouseEvent() && !event->IsTouchEvent()) { // TODO(rbyers): Gesture event repost is tricky to get right // crbug.com/170987. @@ -2208,15 +2183,9 @@ void MenuController::RepostEvent(SubmenuView* source, state_.item->GetRootMenuItem()->GetSubmenu()->ReleaseCapture(); #endif - gfx::Point screen_loc(event->location()); - View::ConvertPointToScreen(source->GetScrollViewContainer(), &screen_loc); - gfx::NativeView native_view = source->GetWidget()->GetNativeView(); if (!native_view) return; - gfx::Screen* screen = gfx::Screen::GetScreenFor(native_view); - gfx::NativeWindow window = screen->GetWindowAtScreenPoint(screen_loc); - #if defined(OS_WIN) gfx::Point screen_loc_pixels = gfx::win::DIPToScreenPoint(screen_loc); HWND target_window = ::WindowFromPoint(screen_loc_pixels.ToPOINT()); @@ -2294,6 +2263,47 @@ void MenuController::RepostEvent(SubmenuView* source, MenuMessageLoop::RepostEventToWindow(event, window, screen_loc); } +void MenuController::RepostEventAndCancel(SubmenuView* source, + const ui::LocatedEvent* event) { + // Cancel can lead to the deletion |source| so we save the view and window to + // be used when reposting the event. + gfx::Point screen_loc(event->location()); + View::ConvertPointToScreen(source->GetScrollViewContainer(), &screen_loc); + gfx::NativeView native_view = source->GetWidget()->GetNativeView(); + gfx::NativeWindow window = nullptr; + if (native_view) { + gfx::Screen* screen = gfx::Screen::GetScreenFor(native_view); + window = screen->GetWindowAtScreenPoint(screen_loc); + } + +#if defined(OS_WIN) + // We're going to close and we own the event capture. We need to repost the + // event, otherwise the window the user clicked on won't get the event. + RepostEvent(source, event, screen_loc, native_view, window); +#endif + + // Determine target to see if a complete or partial close of the menu should + // occur. + ExitType exit_type = EXIT_ALL; + if (!menu_stack_.empty()) { + // We're running nested menus. Only exit all if the mouse wasn't over one + // of the menus from the last run. + MenuPart last_part = GetMenuPartByScreenCoordinateUsingMenu( + menu_stack_.back().first.item, screen_loc); + if (last_part.type != MenuPart::NONE) + exit_type = EXIT_OUTERMOST; + } + Cancel(exit_type); + +#if defined(OS_CHROMEOS) + // We're going to exit the menu and want to repost the event so that is + // is handled normally after the context menu has exited. We call + // RepostEvent after Cancel so that event capture has been released so + // that finding the event target is unaffected by the current capture. + RepostEvent(source, event, screen_loc, native_view, window); +#endif +} + void MenuController::SetDropMenuItem( MenuItemView* new_target, MenuDelegate::DropPosition new_position) { diff --git a/ui/views/controls/menu/menu_controller.h b/ui/views/controls/menu/menu_controller.h index 4325e64..12af0aa 100644 --- a/ui/views/controls/menu/menu_controller.h +++ b/ui/views/controls/menu/menu_controller.h @@ -503,7 +503,15 @@ class VIEWS_EXPORT MenuController : public WidgetObserver { // On non-aura Windows, a new mouse event is generated and posted to // the window (if there is one) at the location of the event. On // aura, the event is reposted on the RootWindow. - void RepostEvent(SubmenuView* source, const ui::LocatedEvent* event); + void RepostEvent(SubmenuView* source, + const ui::LocatedEvent* event, + const gfx::Point& screen_loc, + gfx::NativeView native_view, + gfx::NativeWindow window); + + // For Windows and Aura we repost an event which dismisses the |source| menu. + // The menu is also canceled dependent on the target of the event. + void RepostEventAndCancel(SubmenuView* source, const ui::LocatedEvent* event); // Sets the drop target to new_item. void SetDropMenuItem(MenuItemView* new_item, diff --git a/ui/views/controls/menu/menu_controller_unittest.cc b/ui/views/controls/menu/menu_controller_unittest.cc index 03ba671..e9074f0 100644 --- a/ui/views/controls/menu/menu_controller_unittest.cc +++ b/ui/views/controls/menu/menu_controller_unittest.cc @@ -9,9 +9,14 @@ #include "build/build_config.h" #include "ui/aura/scoped_window_targeter.h" #include "ui/aura/window.h" +#include "ui/events/event.h" +#include "ui/events/event_constants.h" #include "ui/events/event_handler.h" +#include "ui/events/event_utils.h" #include "ui/events/null_event_targeter.h" #include "ui/events/test/event_generator.h" +#include "ui/gfx/geometry/point.h" +#include "ui/gfx/geometry/rect.h" #include "ui/views/controls/menu/menu_controller_delegate.h" #include "ui/views/controls/menu/menu_delegate.h" #include "ui/views/controls/menu/menu_item_view.h" @@ -386,6 +391,11 @@ class MenuControllerTest : public ViewsTestBase { menu_controller_->set_is_combobox(is_combobox); } + void SetSelectionOnPointerDown(SubmenuView* source, + const ui::LocatedEvent* event) { + menu_controller_->SetSelectionOnPointerDown(source, event); + } + void RunMenu() { menu_controller_->message_loop_depth_++; menu_controller_->RunMessageLoop(false); @@ -825,6 +835,81 @@ TEST_F(MenuControllerTest, DoubleAsynchronousNested) { EXPECT_EQ(1, nested_delegate->on_menu_closed_called()); } +// Tests that an asynchronous menu nested within a synchronous menu does not +// crash when trying to repost events that occur outside of the bounds of the +// menu. Instead a proper shutdown should occur. +TEST_F(MenuControllerTest, AsynchronousRepostEvent) { + MenuController* controller = menu_controller(); + TestMenuControllerDelegate* delegate = menu_controller_delegate(); + scoped_ptr<TestMenuControllerDelegate> nested_delegate( + new TestMenuControllerDelegate()); + + ASSERT_FALSE(IsAsyncRun()); + + controller->AddNestedDelegate(nested_delegate.get()); + controller->SetAsyncRun(true); + + EXPECT_TRUE(IsAsyncRun()); + EXPECT_EQ(nested_delegate.get(), GetCurrentDelegate()); + + MenuItemView* item = menu_item(); + int mouse_event_flags = 0; + MenuItemView* run_result = + controller->Run(owner(), nullptr, item, gfx::Rect(), MENU_ANCHOR_TOPLEFT, + false, false, &mouse_event_flags); + EXPECT_EQ(run_result, nullptr); + + // Show a sub menu to targert with a pointer selection. However have the event + // occur outside of the bounds of the entire menu. + SubmenuView* sub_menu = item->GetSubmenu(); + sub_menu->ShowAt(owner(), item->bounds(), false); + gfx::Point location(sub_menu->bounds().bottom_right()); + location.Offset(1, 1); + ui::MouseEvent event(ui::ET_MOUSE_PRESSED, location, location, + ui::EventTimeForNow(), ui::EF_LEFT_MOUSE_BUTTON, 0); + + // When attempting to select outside of all menus this should lead to a + // shutdown. This should not crash while attempting to repost the event. + SetSelectionOnPointerDown(sub_menu, &event); + + EXPECT_FALSE(IsAsyncRun()); + EXPECT_EQ(delegate, GetCurrentDelegate()); + EXPECT_EQ(0, delegate->on_menu_closed_called()); + EXPECT_EQ(1, nested_delegate->on_menu_closed_called()); + EXPECT_EQ(nullptr, nested_delegate->on_menu_closed_menu()); + EXPECT_EQ(0, nested_delegate->on_menu_closed_mouse_event_flags()); + EXPECT_EQ(internal::MenuControllerDelegate::NOTIFY_DELEGATE, + nested_delegate->on_menu_closed_notify_type()); + EXPECT_EQ(MenuController::EXIT_ALL, controller->exit_type()); +} + +// Tests that an asynchronous menu reposts touch events that occur outside of +// the bounds of the menu, and that the menu closes. +TEST_F(MenuControllerTest, AsynchronousTouchEventRepostEvent) { + MenuController* controller = menu_controller(); + TestMenuControllerDelegate* delegate = menu_controller_delegate(); + controller->SetAsyncRun(true); + + // Show a sub menu to targert with a touch event. However have the event occur + // outside of the bounds of the entire menu. + MenuItemView* item = menu_item(); + SubmenuView* sub_menu = item->GetSubmenu(); + sub_menu->ShowAt(owner(), item->bounds(), false); + gfx::Point location(sub_menu->bounds().bottom_right()); + location.Offset(1, 1); + ui::TouchEvent event(ui::ET_TOUCH_PRESSED, location, 0, 0, + ui::EventTimeForNow(), 0, 0, 0, 0); + controller->OnTouchEvent(sub_menu, &event); + + EXPECT_FALSE(IsShowing()); + EXPECT_EQ(1, delegate->on_menu_closed_called()); + EXPECT_EQ(nullptr, delegate->on_menu_closed_menu()); + EXPECT_EQ(0, delegate->on_menu_closed_mouse_event_flags()); + EXPECT_EQ(internal::MenuControllerDelegate::NOTIFY_DELEGATE, + delegate->on_menu_closed_notify_type()); + EXPECT_EQ(MenuController::EXIT_ALL, controller->exit_type()); +} + // Tests that if you exit all menus when an asynchrnous menu is nested within a // synchronous menu, the message loop for the parent menu finishes running. TEST_F(MenuControllerTest, AsynchronousNestedExitAll) { |