All praise the mighty felt@, for she provided https://permission.site/ to
repro issues like this.

(And yes, it's likely focus. Bubbles and focus... are inconsistent. IIRC,
we want permission bubbles to have focus. +felt@ for permission-y things,
+ainslie@ for bubbliness)

I don't know if you're reporting behavior that has changed but I don't think File -> Close Tab should close the bubble.

Now _actually_ cc'ing ainslie and felt (please see c#1)

And re c#3: yes, File->Close tab should close the bubble - it's a permission request for the current tab, why would we not close it when the tab goes away?

For what it's worth, there's already a separate path for ESC because we want that to close the permission bubble. See bug #385088 for lengthy discussion of the madness of the key path when a bubble is up.

Ugh - nevermind. Misread pinkerton@'s bug report.

+benwells, who has offered to help w/ permission bubble bugs

Yurch. I think some kind of mac-person, or smarter-than-me-person, should look at this.

shrike: any ideas who could look at this?

I can give some history:

- InfoBubbleWindow, which PermissionBubbleWindow descends from, does a pretty good job of dealing with the keyboard. The page info bubble, for instance, works well and doesn't have this particular bug, and I think it handles ESC properly

- Initially there was no PermissionBubbleWindow, the permission bubble just used InfoBubbleWindow

- A bug was reported on permission bubbles where it didn't handle Cmd+Shift+[ or +] to cycle tabs.

- So, a PermissionBubbleWindow was introduced with some code to address this. I'm not sure why it wasn't added to InfoBubbleWindow to fix tab cycling with other bubbles. I'm guessing this is the change which broke shortcuts like Cmd+W but it could have been later changes

- Over time the code in PermissionBubbleWindow grew, e.g. it was also updated to handle ESC properly which was broken

I think the best approach is to remove the PermissionBubbleWindow class and fix the Cmd+Shift+[ bug in InfoBubbleWindow, while not breaking other things.

We should re-triage rather than leave available and unowned. 

This is a polish bug, and the broken behavior is pretty annoying [at least to me].

That being said, we should just fix this in views, and then use views [rather than reduplicating effort].

tapted: Where does this fall in the timeline of views-ification?

It's using those webui-ish 'ConstrainedWindowButtons`, so Phase2 (m53). But we haven't plumbed these dialogs under chrome://flags/#mac-views-dialogs yet.

We *have* plumbed the other, "page action" bubbles (the ones that appear with an icon on the right hand side of the location bar -- see attached).

Testing these in Canary+mac-views-dialogs, Cmd+W works (tab closes), but the bubble isn't automatically dismissed when its originator tab disappears - that's something to fix. Filed  Issue 605374 .

But there's a lot of code shared for these bubbles under views - that the keyhandling works as expected for the page action bubble suggests it would work for the permission bubble.


But Ironically...... the problem on Windows is the reverse. Ctrl+w/Ctrl+F4 works for permission bubbles, but is ignored for page action bubbles. So there's something to investigate there as well.

Deleted: Screen Shot 2016-04-21 at 11.29.46 AM.png 36.0 KB

	Screen Shot 2016-04-21 at 11.29.46 AM.png 36.0 KB View Download

#9: what's more annoying, Cmd+W not working, or Cmd+Shift+[ not working? Because I think it should be easy to fix Cmd+W if you're OK with Cmd+Shift+[ breaking :)

#10: does Cmd+Shift+[ cycle tabs in the page action bubbles? I think when we tested it it didn't.

PermissionBubbleWindow seems pretty small. Can we just move its -performKeyEquivalent: logic into InfoBubbleWindow? That's where Esc and Cmd+. is currently handled.

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm&q=PermissionBubbleWindow&sq=package:chromium&type=cs&l=154

I'm not sure that works, because PermissionBubbleWindow assumes that its parent is a BrowserWindow. Do all InfoBubbleWindows satisfy that criterion?

Quick scan points out ValidationMessageBubbleCocoa, which has as parent [RVH->GetWidget()->GetView()->GetNativeView() window] - I have no idea what that resolves to :)

I vaguely recall AutofillBubble and AutofillTooltip also violate that, but not sure. (And at least their requestAutocomplete incarnations, which _def_ use a different window, will be gone soon)


But it will also mean that all bubbles now handle all browser keys when they have focus. I _assume_ that's correct, but we should probably double-check. 

I think it only requires the window to be a ChromeEventProcessingWindow, despite the BrowserWindowUtils class name. An -isKindOfClass: check could work in the InfoBubbleWindow.

For more background on this, see  bug #380382  & http://crrev.com/315413003

Given that all that hackery is due to allowShareParentKeyState, and that was a 10.7 specific hack... can we just rip that out completely?

Yes, it does look like we can remove allowShareParentKeyState.

I think `allowShareParentKeyState` is what allows the traffic lights on the browser window to "remain lit" when a bubble is shown.

Removing that would make bubbles behave more like, e.g., http-auth dialogs, where the traffic lights on the browser window will become greyed out.

But there a *lot* of hacks to make allowShareParentKeyState work properly. It's a private API, and really only used natively for displaying window-modal sheets (AFAIK). And it messes with various things around key state. If all we really care about is the color of the traffic lights, there are probably cleaner hacks (perhaps even swizzling the NSButton subclass used for the traffic lights..)

Oh, you're right. I thought child windows by default shared key state with the parent, but that's a nasty hack we found. I do think we want that behavior {un,}fortunatley.

According to the comments, that hack is 10.7 only. Since 10.7 is gone... :)

I think the comment is wrong. https://bugs.chromium.org/p/chromium/issues/detail?id=116179#c14 indicates that _sharesParentKeyState does something on 10.8.

 Issue 555084  has been merged into this issue.

[mac triage] mrh

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1b0d712e570d6800634910954d6a6690b1ae5d17

commit 1b0d712e570d6800634910954d6a6690b1ae5d17
Author: tapted <tapted@chromium.org>
Date: Mon May 09 22:31:37 2016

Mac: Use a "less" private API for keeping traffic lights lit while bubbles are showing

The approach: Rather than overriding -[NSWindow _sharesParentKeyState]
on the child, override -[NSWindow hasKeyAppearance] on the parent.  This
is used, e.g., by WebKit since http://wkrev.com/164173. It doesn't have
an underscore on the front which I guess is nice.

After this, we should be able to phase out the overrides of the slightly
"more" private API -_sharesParentKeyState. info_bubble_window.mm still
uses this, and it causes a lot of complex interactions with hotkeys and
dialog dismissal. Leave this for a follow-up since it will be tricky to
ensure there are no regressions while unwinding the existing workarounds
for _sharesParentKeyState weirdness.

This would also give a nice way to keep traffic lights lit when WebModal
dialogs are showing. This isn't currently done, and would make their
behaviour more consistent with native sheets. However, WebModals are
actually *grand*-children of the Cocoa browser window and have other
complications. Leave that for a follow-up as well.

BUG= 602846 ,  603881 

Review-Url: https://codereview.chromium.org/1955343002
Cr-Commit-Position: refs/heads/master@{#392448}

[modify] https://crrev.com/1b0d712e570d6800634910954d6a6690b1ae5d17/chrome/browser/ui/cocoa/chrome_browser_window.mm
[modify] https://crrev.com/1b0d712e570d6800634910954d6a6690b1ae5d17/ui/views/cocoa/native_widget_mac_nswindow.mm
[modify] https://crrev.com/1b0d712e570d6800634910954d6a6690b1ae5d17/ui/views/widget/native_widget_mac_interactive_uitest.mm

TIL: every keyboard shortcut calls nearly one million ObjC functions in my Chromium (916,867 to be precise -- see attached).

Interestingly, 2,377 of these method calls are calls to -[BookmarkMenuCocoaController respondsToSelector:]. Yes, every hotkey pressed in a Mac Cocoa browser will ask every bookmark you have whether it wants to process the command. Gross.

Anyway, I think the problem is that the machinery in -[mainMenu performKeyEquivalent] asks a bunch of things in the responder chain if they want to do the menu item. This is happening after the permission bubble has attempted to foist key handling off to the browser window, but the permission bubble is still first in the responder chain so the NSMenu machinery gets super confused.

I have a neat fix for this in https://codereview.chromium.org/2666523002

Deleted: menu_perform_key.zip 927 KB

menu_perform_key.zip 927 KB Download

😳 I wonder if that has any impact on something like issue 651203.

Would NSPopover fix this bug?

NSPopover has a lot of internal private magic, so.. maybe? But by that same reasoning we may be forced into a lot of behaviours/conflicts that UX do not want. The fix in #c24 is straightforward.

Definitely not trying to veto the fix in #24! I'm just suggesting NSPopover as a next step to simplify our code, if it's workable, since it looks like the supported way to show a child window while forwarding keyboard shortcuts to the parent and keeping its window buttons active, without *us* having to use private APIs.

[I see it less as "internal private magic" and more as the implementation details of built-in behavior — just like the magic NSWindow uses to show itself, put itself into the Dock, etc.]

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e91c25b113011f055428c46609e19ce6550b41ea

commit e91c25b113011f055428c46609e19ce6550b41ea
Author: tapted <tapted@chromium.org>
Date: Thu Feb 02 22:43:45 2017

Allow permission bubbles to participate in key event dispatch as if they were a Browser.

Accomplish this by allowing the `CommandDispatcher` we use for
ChomeEventProcessingWindows to bubble up key handling, and command
validation/dispatch. Dispatch goes up to a parent window's
CommandDispatcher when certain conditions are met.

Currently all ChomeEventProcessingWindows and Views'
NativeWidgetMacNSWindows have a CommandDispatcher, but only browser
windows provide a CommandHandler. By asking the CommandHandler in the
parent window, we can validate commands in the mainMenu, then forward
the -commandDispatch: action from the menu item when NSMenu calls it on
the key window (which might not be a browser).

Adds a rather fun interactive UI test for this that runs with Views and
Cocoa permissions bubbles, and tests commands dispatched via the
mainMenu (Cmd+w, Cmd+Alt+Left) and performKeyEquivalent (Cmd+Shift+'{')

BUG= 603881 , 679339

Review-Url: https://codereview.chromium.org/2666523002
Cr-Commit-Position: refs/heads/master@{#447864}

[modify] https://crrev.com/e91c25b113011f055428c46609e19ce6550b41ea/chrome/browser/permissions/permission_request_manager.h
[add] https://crrev.com/e91c25b113011f055428c46609e19ce6550b41ea/chrome/browser/permissions/permission_request_manager_test_api.cc
[add] https://crrev.com/e91c25b113011f055428c46609e19ce6550b41ea/chrome/browser/permissions/permission_request_manager_test_api.h
[modify] https://crrev.com/e91c25b113011f055428c46609e19ce6550b41ea/chrome/browser/ui/cocoa/chrome_event_processing_window.mm
[add] https://crrev.com/e91c25b113011f055428c46609e19ce6550b41ea/chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_interactive_uitest.mm
[modify] https://crrev.com/e91c25b113011f055428c46609e19ce6550b41ea/chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm
[modify] https://crrev.com/e91c25b113011f055428c46609e19ce6550b41ea/chrome/test/BUILD.gn
[modify] https://crrev.com/e91c25b113011f055428c46609e19ce6550b41ea/ui/base/cocoa/command_dispatcher.h
[modify] https://crrev.com/e91c25b113011f055428c46609e19ce6550b41ea/ui/base/cocoa/command_dispatcher.mm
[modify] https://crrev.com/e91c25b113011f055428c46609e19ce6550b41ea/ui/views/cocoa/native_widget_mac_nswindow.mm

 Issue 626525  has been merged into this issue.

Fixed - I think permissions bubbles now behave as expected on Mac under Cocoa/Harmony.

For Harmony, there's some other weirdness to address in  Issue 605374 

(and on Windows shortcuts are blocked, but they shouldn't be - I guess that's Issue 319109)