summaryrefslogtreecommitdiffstats
path: root/chrome/browser
diff options
context:
space:
mode:
authorsuzhe@chromium.org <suzhe@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>2010-12-06 19:40:23 +0000
committersuzhe@chromium.org <suzhe@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>2010-12-06 19:40:23 +0000
commit0f72a2dc2d84f60e986337233c37457cb6e59d3e (patch)
tree102313fcdf7521af4914508de962b3b7802fafa6 /chrome/browser
parente90e522650e27a379fe8c3c6fe49b214e830f949 (diff)
downloadchromium_src-0f72a2dc2d84f60e986337233c37457cb6e59d3e.zip
chromium_src-0f72a2dc2d84f60e986337233c37457cb6e59d3e.tar.gz
chromium_src-0f72a2dc2d84f60e986337233c37457cb6e59d3e.tar.bz2
Fix BrowserKeyEventsTest.ReservedAccelerators on Mac 10.6
There are two reasons that cause this problem: 1. See issue http://crbug.com/65375 2. By some reason, when BrowserWindowCocoa::PreHandleKeyboardEvent() is called for Cmd-W, it fails to get its corresponding command id, because the corresponding menu item is not enabled at that time. This CL does following changes: 1. Change testing flow to work around issue 65375. 2. Fix the second problem by always retrieving the corresponding command id regardless of the menu item's state. The reason of the second issue: When Cmd-T gets handled in NSApp, BrowserWindowController's -validateUserInterfaceItem: method will be called for all menu items to determine their enable states and the states will be cached in each menu item. At this time, because there is only one tab, the "Close Tab" menu item will be disabled. Then when sending Cmd-W to the browser, it'll firstly be handled in BrowserWindowCocoa::PreHandleKeyboardEvent() method, which will look up the command id associated to Cmd-W, but at this time, BrowserWindowController's -validateUserInterfaceItem: method does not get called yet, so the corresponding menu item is still not enabled. Then BrowserWindowCocoa::PreHandleKeyboardEvent() will not handle Cmd-W because it cannot find its command id. After removing the check in cr_firesForKeyEvent method, BrowserWindowCocoa::PreHandleKeyboardEvent() method will always get the correct command id of a key equivalent regardless of its enable state, then if it's a reserved key equivalent, it will always be sent to NSApp for handling, which will call BrowserWindowController's -validateUserInterfaceItem: to update the enable states of all menu items. Then if the corresponding menu item is really disabled, NSApp will do nothing and return false, then the key event will be dispatched to web page as normal. BUG=50447 BrowserKeyEventsTest.ReservedAccelerators failed on Mac10.6 Tests (dbg)(1) bot. TEST=The test should not flaky. Review URL: http://codereview.chromium.org/5576002 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@68369 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'chrome/browser')
-rw-r--r--chrome/browser/browser_keyevents_browsertest.cc51
-rw-r--r--chrome/browser/ui/cocoa/browser_window_cocoa.mm2
-rw-r--r--chrome/browser/ui/cocoa/nsmenuitem_additions.h5
-rw-r--r--chrome/browser/ui/cocoa/nsmenuitem_additions.mm5
4 files changed, 43 insertions, 20 deletions
diff --git a/chrome/browser/browser_keyevents_browsertest.cc b/chrome/browser/browser_keyevents_browsertest.cc
index 021104b..5e3fb47 100644
--- a/chrome/browser/browser_keyevents_browsertest.cc
+++ b/chrome/browser/browser_keyevents_browsertest.cc
@@ -636,14 +636,7 @@ IN_PROC_BROWSER_TEST_F(BrowserKeyEventsTest, AccessKeys) {
#endif
}
-#if defined(OS_MACOSX)
-// See http://crbug.com/50447 for details.
-#define MAYBE_ReservedAccelerators FLAKY_ReservedAccelerators
-#else
-#define MAYBE_ReservedAccelerators ReservedAccelerators
-#endif
-
-IN_PROC_BROWSER_TEST_F(BrowserKeyEventsTest, MAYBE_ReservedAccelerators) {
+IN_PROC_BROWSER_TEST_F(BrowserKeyEventsTest, ReservedAccelerators) {
ASSERT_TRUE(test_server()->Start());
ASSERT_TRUE(ui_test_utils::BringBrowserWindowToFront(browser()));
@@ -653,6 +646,8 @@ IN_PROC_BROWSER_TEST_F(BrowserKeyEventsTest, MAYBE_ReservedAccelerators) {
ASSERT_NO_FATAL_FAILURE(ClickOnView(VIEW_ID_TAB_CONTAINER));
ASSERT_TRUE(IsViewFocused(VIEW_ID_TAB_CONTAINER_FOCUS_VIEW));
+ ASSERT_EQ(1, browser()->tab_count());
+
#if defined(OS_WIN) || defined(TOOLKIT_VIEWS)
static const KeyEventTestData kTestCtrlT = {
app::VKEY_T, true, false, false, false,
@@ -660,8 +655,7 @@ IN_PROC_BROWSER_TEST_F(BrowserKeyEventsTest, MAYBE_ReservedAccelerators) {
{ "D 17 0 true false false false" }
};
- ASSERT_EQ(1, browser()->tab_count());
- // Press Ctrl+T, which will open a new tab.
+ // Press Ctrl+T, which will open a new tab. It cannot be suppressed.
EXPECT_NO_FATAL_FAILURE(TestKeyEvent(0, kTestCtrlT));
EXPECT_EQ(2, browser()->tab_count());
browser()->SelectNumberedTab(0);
@@ -671,6 +665,8 @@ IN_PROC_BROWSER_TEST_F(BrowserKeyEventsTest, MAYBE_ReservedAccelerators) {
ASSERT_NO_FATAL_FAILURE(GetResultLength(0, &result_length));
EXPECT_EQ(1, result_length);
+ ASSERT_TRUE(IsViewFocused(VIEW_ID_TAB_CONTAINER_FOCUS_VIEW));
+
// Reserved accelerators can't be suppressed.
ASSERT_NO_FATAL_FAILURE(SuppressAllEvents(0, true));
// Press Ctrl+W, which will close the tab.
@@ -684,19 +680,35 @@ IN_PROC_BROWSER_TEST_F(BrowserKeyEventsTest, MAYBE_ReservedAccelerators) {
{ "D 91 0 false false false true" }
};
- ASSERT_EQ(1, browser()->tab_count());
- // Press Cmd+T, which will open a new tab.
+ ui_test_utils::WindowedNotificationObserver wait_for_new_tab(
+ NotificationType::TAB_PARENTED,
+ NotificationService::AllSources());
+
+ // Press Cmd+T, which will open a new tab. It cannot be suppressed.
EXPECT_NO_FATAL_FAILURE(TestKeyEvent(0, kTestCmdT));
- EXPECT_EQ(2, browser()->tab_count());
- browser()->SelectNumberedTab(0);
- ASSERT_EQ(0, browser()->selected_index());
+
+ wait_for_new_tab.WaitFor(Source<NavigationController>(
+ &browser()->GetTabContentsAt(1)->controller()));
int result_length;
ASSERT_NO_FATAL_FAILURE(GetResultLength(0, &result_length));
EXPECT_EQ(1, result_length);
+ EXPECT_EQ(2, browser()->tab_count());
+ ASSERT_EQ(1, browser()->selected_index());
+
+ // Because of issue http://crbug.com/65375, switching back to the first tab
+ // may cause the focus to be grabbed by omnibox. So instead, we load our
+ // testing page in the newly created tab and try Cmd-W here.
+ ui_test_utils::NavigateToURL(browser(), url);
+
+ // Make sure the focus is in the testing page.
+ ASSERT_NO_FATAL_FAILURE(ClickOnView(VIEW_ID_TAB_CONTAINER));
+ ASSERT_TRUE(IsViewFocused(VIEW_ID_TAB_CONTAINER_FOCUS_VIEW));
+
// Reserved accelerators can't be suppressed.
- ASSERT_NO_FATAL_FAILURE(SuppressAllEvents(0, true));
+ ASSERT_NO_FATAL_FAILURE(SuppressAllEvents(1, true));
+
// Press Cmd+W, which will close the tab.
ASSERT_TRUE(ui_test_utils::SendKeyPressSync(
browser(), app::VKEY_W, false, false, false, true));
@@ -740,8 +752,6 @@ IN_PROC_BROWSER_TEST_F(BrowserKeyEventsTest, MAYBE_ReservedAccelerators) {
"U 17 0 true false false false" }
};
- ASSERT_EQ(1, browser()->tab_count());
-
// Ctrl+T should be blockable.
EXPECT_NO_FATAL_FAILURE(TestKeyEvent(0, kTestCtrlTBlocked));
ASSERT_EQ(1, browser()->tab_count());
@@ -751,6 +761,7 @@ IN_PROC_BROWSER_TEST_F(BrowserKeyEventsTest, MAYBE_ReservedAccelerators) {
ASSERT_EQ(1, browser()->selected_index());
browser()->SelectNumberedTab(0);
ASSERT_EQ(0, browser()->selected_index());
+ ASSERT_TRUE(IsViewFocused(VIEW_ID_TAB_CONTAINER_FOCUS_VIEW));
// Ctrl+PageDown and Ctrl+Tab switches to the next tab.
EXPECT_NO_FATAL_FAILURE(TestKeyEvent(0, kTestCtrlPageDown));
@@ -758,12 +769,16 @@ IN_PROC_BROWSER_TEST_F(BrowserKeyEventsTest, MAYBE_ReservedAccelerators) {
browser()->SelectNumberedTab(0);
ASSERT_EQ(0, browser()->selected_index());
+ ASSERT_TRUE(IsViewFocused(VIEW_ID_TAB_CONTAINER_FOCUS_VIEW));
+
EXPECT_NO_FATAL_FAILURE(TestKeyEvent(0, kTestCtrlTab));
ASSERT_EQ(1, browser()->selected_index());
// Ctrl+W should be blockable.
browser()->SelectNumberedTab(0);
ASSERT_EQ(0, browser()->selected_index());
+ ASSERT_TRUE(IsViewFocused(VIEW_ID_TAB_CONTAINER_FOCUS_VIEW));
+
EXPECT_NO_FATAL_FAILURE(TestKeyEvent(0, kTestCtrlWBlocked));
ASSERT_EQ(2, browser()->tab_count());
diff --git a/chrome/browser/ui/cocoa/browser_window_cocoa.mm b/chrome/browser/ui/cocoa/browser_window_cocoa.mm
index d1e7f97..fc86977 100644
--- a/chrome/browser/ui/cocoa/browser_window_cocoa.mm
+++ b/chrome/browser/ui/cocoa/browser_window_cocoa.mm
@@ -465,7 +465,7 @@ void BrowserWindowCocoa::HandleKeyboardEvent(
if (submenu != [NSApp servicesMenu])
result = [self itemForKeyEquivalent:key
menu:submenu];
- } else if ([item cr_firesForKeyEvent:key]) {
+ } else if ([item cr_firesForKeyEventIfEnabled:key]) {
result = item;
}
diff --git a/chrome/browser/ui/cocoa/nsmenuitem_additions.h b/chrome/browser/ui/cocoa/nsmenuitem_additions.h
index 830498a..289926a 100644
--- a/chrome/browser/ui/cocoa/nsmenuitem_additions.h
+++ b/chrome/browser/ui/cocoa/nsmenuitem_additions.h
@@ -12,8 +12,13 @@
// Returns true exactly if the menu item would fire if it would be put into
// a menu and then |menu performKeyEquivalent:event| was called.
+// This method always returns NO if the menu item is not enabled.
- (BOOL)cr_firesForKeyEvent:(NSEvent*)event;
+// Like above method, but this method matches the key equivalent regardless of
+// the menu item's enable state.
+- (BOOL)cr_firesForKeyEventIfEnabled:(NSEvent*)event;
+
@end
#endif // CHROME_BROWSER_UI_COCOA_NSMENUITEM_ADDITIONS_H_
diff --git a/chrome/browser/ui/cocoa/nsmenuitem_additions.mm b/chrome/browser/ui/cocoa/nsmenuitem_additions.mm
index 90bb353..1799eb0 100644
--- a/chrome/browser/ui/cocoa/nsmenuitem_additions.mm
+++ b/chrome/browser/ui/cocoa/nsmenuitem_additions.mm
@@ -11,10 +11,13 @@
@implementation NSMenuItem(ChromeAdditions)
- (BOOL)cr_firesForKeyEvent:(NSEvent*)event {
- DCHECK([event type] == NSKeyDown);
if (![self isEnabled])
return NO;
+ return [self cr_firesForKeyEventIfEnabled:event];
+}
+- (BOOL)cr_firesForKeyEventIfEnabled:(NSEvent*)event {
+ DCHECK([event type] == NSKeyDown);
// In System Preferences->Keyboard->Keyboard Shortcuts, it is possible to add
// arbitrary keyboard shortcuts to applications. It is not documented how this
// works in detail, but |NSMenuItem| has a method |userKeyEquivalent| that
generated by cgit v1.1 at 2025-03-05 03:46:10 +0000