Chromium Code Reviews
Help | Chromium Project | Sign in
(5)

Issue 2834763002: Mac: Never dispatch keyEquivalents to the web contents if the window is not Key. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 days, 7 hours ago by tapted (OOO until 2017-05-01)
Modified:
4 days, 13 hours ago
CC:
chromium-reviews, yusukes+watch_chromium.org, shuchen+watch_chromium.org, jam, nona+watch_chromium.org, darin-cc_chromium.org, mac-reviews_chromium.org, James Su, chrome-apps-syd-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Mac: Never dispatch keyEquivalents to the web contents if the window is not Key. When pressing a hotkey on Mac, AppKit asks pretty much everything in the UI if it wants to handle it. When a dialog is showing, we want the browser window to handle commands such as "change tabs". So events can propagate up to a browser window, but WebContents shouldn't see events when a dialog is active. In fact, the WebContents doesn't: the dialog machinery invokes RenderWidgetHostImpl::SetIgnoreInputEvents(..) while the dialog is showing, but RenderWidgetHostImpl ignores the event only *after* [RenderWidgetHostViewCocoa performKeyEquivalent:] has returned NO. Ignored events are not sent over IPC, so they are never sent back "unhandled" to be redispatched. So this prevents other parts of the UI (such as the mainMenu) from participating in event dispatch - the event is lost. This became an issue when dialogs started doing their own command propagation rather than hooking the -[NSWindow _sharesParentKeyState] SPI which causes a bunch of other weird issues. To fix, have RenderWidgetHostViewCocoa return NO immediately if its window is not the keyWindow. It currently just checks if it's the firstResponder in its window. BUG=692377 TEST=On Mac, Enable chrome://flags/#secondary-ui-md, Navigate to https://httpbin.org/basic-auth/user/passwd, cancel the dialog, then refresh the page (dialog should show a second time). Type some text and press Cmd+x to 'Cut' - text should be cut and Edit menu should flash. Review-Url: https://codereview.chromium.org/2834763002 Cr-Commit-Position: refs/heads/master@{#466553} Committed: https://chromium.googlesource.com/chromium/src/+/a9e14964b037e70a90936908b8740d50b73e3b73

Patch Set 1 #

Patch Set 2 : fix a test that will probably fail #

Patch Set 3 : Add a bespoke test #

Unified diffs Side-by-side diffs Delta from patch set Stats (+51 lines, -2 lines) Patch
M content/browser/renderer_host/render_widget_host_view_mac.mm View 2 1 chunk +4 lines, -2 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_mac_unittest.mm View 1 2 3 chunks +47 lines, -0 lines 0 comments Download
Trybot results:  win_clang   win_chromium_compile_dbg_ng   win_chromium_x64_rel_ng   win_chromium_rel_ng   mac_chromium_compile_dbg_ng   mac_chromium_rel_ng   ios-simulator-xcode-clang   ios-simulator   linux_chromium_tsan_rel_ng   ios-device-xcode-clang   ios-device   linux_chromium_rel_ng   linux_chromium_compile_dbg_ng   linux_chromium_chromeos_ozone_rel_ng   linux_chromium_chromeos_rel_ng   chromeos_daisy_chromium_compile_only_ng   chromium_presubmit   linux_chromium_asan_rel_ng   cast_shell_linux   chromeos_amd64-generic_chromium_compile_only_ng   cast_shell_android   linux_android_rel_ng   android_cronet   android_n5x_swarming_rel   android_clang_dbg_recipe   android_compile_dbg   android_arm64_dbg_recipe   chromium_presubmit   chromium_presubmit   win_clang   win_chromium_x64_rel_ng   win_chromium_compile_dbg_ng   mac_chromium_rel_ng   win_chromium_rel_ng   mac_chromium_compile_dbg_ng   ios-simulator   ios-device-xcode-clang   ios-simulator-xcode-clang   linux_chromium_tsan_rel_ng   ios-device   linux_chromium_rel_ng   linux_chromium_chromeos_rel_ng   linux_chromium_compile_dbg_ng   linux_chromium_asan_rel_ng   linux_chromium_chromeos_ozone_rel_ng   chromeos_daisy_chromium_compile_only_ng   chromium_presubmit   cast_shell_linux   chromeos_amd64-generic_chromium_compile_only_ng   linux_android_rel_ng   cast_shell_android   android_n5x_swarming_rel   android_cronet   android_clang_dbg_recipe   android_compile_dbg   android_arm64_dbg_recipe 
Commit queue not available (can’t edit this change).

Messages

Total messages: 35 (26 generated)
tapted (OOO until 2017-05-01)
Hi Nico, please take a look
6 days, 4 hours ago (2017-04-21 06:14:40 UTC) #18
Nico
lgtm TEST= were originally for explaining to QA how to manually test what the CL ...
5 days, 20 hours ago (2017-04-21 14:27:17 UTC) #19
tapted (OOO until 2017-05-01)
On 2017/04/21 14:27:17, Nico wrote: > lgtm > > TEST= were originally for explaining to ...
5 days, 10 hours ago (2017-04-22 00:22:36 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2834763002/60001
5 days, 10 hours ago (2017-04-22 00:23:36 UTC) #24
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/418076)
5 days, 10 hours ago (2017-04-22 00:32:42 UTC) #26
tapted (OOO until 2017-05-01)
+asvitkine for OWNERS - please take a look. Thanks!
5 days, 9 hours ago (2017-04-22 01:22:03 UTC) #28
Alexei Svitkine (slow)
lgtm
4 days, 14 hours ago (2017-04-22 20:40:57 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2834763002/60001
4 days, 13 hours ago (2017-04-22 21:38:13 UTC) #31
commit-bot: I haz the power
4 days, 13 hours ago (2017-04-22 21:57:13 UTC) #35
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/a9e14964b037e70a90936908b874...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cc6ac46