Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(873)

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

Created:
10 months, 1 week ago by tapted
Modified:
10 months, 1 week 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

Messages

Total messages: 35 (26 generated)
tapted
Hi Nico, please take a look
10 months, 1 week 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 ...
10 months, 1 week ago (2017-04-21 14:27:17 UTC) #19
tapted
On 2017/04/21 14:27:17, Nico wrote: > lgtm > > TEST= were originally for explaining to ...
10 months, 1 week 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
10 months, 1 week 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)
10 months, 1 week ago (2017-04-22 00:32:42 UTC) #26
tapted
+asvitkine for OWNERS - please take a look. Thanks!
10 months, 1 week ago (2017-04-22 01:22:03 UTC) #28
Alexei Svitkine (slow)
lgtm
10 months, 1 week 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
10 months, 1 week ago (2017-04-22 21:38:13 UTC) #31
commit-bot: I haz the power
10 months, 1 week 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...

Powered by Google App Engine
This is Rietveld 408576698