[Mac] Use a nonactivating panel for notifications so that mouse down doesn't activate Chrome.

[Mac] Use a nonactivating panel for notifications so that mouse down doesn't activate Chrome.

Currently, pressing the mouse button over a notification brings Chrome
to the front, even though the tab which sent the notification won't be
shown unless it calls `window.focus()` from its onclick listener.

This CL switches notifications to use NSPanel, which can receive mouse
events without activating the application.

Notifications were previously switched away from NSPanel to work around
crbug.com/459306. I tried fairly hard to repro that issue with this CL,
and couldn't. If it comes back, I feel okay about reverting this change
or taking another crack at its root cause.

BUG=653868
Committed: https://crrev.com/629a5d065848adbac50931ae305a3d8597578e4d
Cr-Commit-Position: refs/heads/master@{#424505}

[Sidney San Martín, 2016-10-07 17:49:12]

Patchset #3 (id:40001) has been deleted

[Sidney San Martín, 2016-10-07 21:29:36]

Description was changed from

==========
[Mac] Use a nonactivating panel for notifications so that clicking doensn't
activate Chrome.

BUG=653868
==========

to

==========
[Mac] Use a nonactivating panel for notifications so that clicking them doesn't
activate Chrome.

Chrome doesn't bring the sending tab to the foreground when a
notification is clicked (on purpose: it's up to the page to call
window.focus() if it wants to be made visible), so right now clicking a
notification will just bring the last-used Chrome window to the front.

Notifications were switched from NSPanel to NSWindow to work around
crbug.com/459306. I tried fairly hard to repro that issue, and couldn't.
Because apps have been deprecated and it seems to have always been
tricky to reproduce, I feel okay about maybe resurfacing that bug (and
either reverting this CL or taking another crack at the root cause
if it comes back).

BUG=653868
==========

[Sidney San Martín, 2016-10-07 21:29:36]

sdy@chromium.org changed reviewers:
+ avi@chromium.org, rsesek@chromium.org, tapted@chromium.org

[Sidney San Martín, 2016-10-07 21:51:45]

avi@ for OWNERship, tapted@ and rsesek@ for sanity because I'm removing a
workaround :).

[Avi (use Gerrit), 2016-10-07 22:02:18]

Good luck? 😬

LGTM

[tapted, 2016-10-10 03:13:25]

it's only packaged apps that are deprecated (and not on ChromeOS, and not on
other platforms for a while). But the main culprit in crbug.com/459306 was the
hangouts extension - not the hangouts app. We can't justify this change using
anything to do with apps. gmail and calendar notifications would also trigger
that bug - they're just more annoying to convince to emit a notification.

E.g. the hangouts *extension* is still supported - when I patch this in and get
a chat notification, clicking the bubble no longer activates Chrome, which isn't
an acceptable regression. (note the chat window is raised within the Chrome
process, but Chrome isn't activated). It also defeats attempts to repro
crbug.com/459306 :).

[Sidney San Martín, 2016-10-10 16:48:35]

On 2016/10/10 03:13:25, tapted wrote:
> it's only packaged apps that are deprecated (and not on ChromeOS, and not on
> other platforms for a while). But the main culprit in crbug.com/459306 was the
> hangouts extension - not the hangouts app. We can't justify this change using
> anything to do with apps. gmail and calendar notifications would also trigger
> that bug - they're just more annoying to convince to emit a notification.
> 
> E.g. the hangouts *extension* is still supported - when I patch this in and
get
> a chat notification, clicking the bubble no longer activates Chrome, which
isn't
> an acceptable regression. (note the chat window is raised within the Chrome
> process, but Chrome isn't activated). It also defeats attempts to repro
> crbug.com/459306 :).

It looks like that was a separate regression that popped up last week! I just
updated this CL to pull in the fix, could you play with it again?

[tapted, 2016-10-11 03:55:44]

lgtm with an updated CL description

https://codereview.chromium.org/2393323005/diff/160001/ui/message_center/coco...
File ui/message_center/cocoa/popup_controller.mm (right):

https://codereview.chromium.org/2393323005/diff/160001/ui/message_center/coco...
ui/message_center/cocoa/popup_controller.mm:115: [window
setBecomesKeyOnlyIfNeeded:YES];
ah-ha! So If I comment out this line, then crbug.com/459306 easily reproduces
for me again with this patched in (maybe worth trying out so you have an idea of
the badness).

So perhaps so long as we don't put textfields into the notification I think this
will hold up -- we're essentially just adding an alternative workaround. I'm
pretty sure we don't put textfields in - but we _do_ have buttons. However, said
buttons are not keyboard accessible. But if we want them to be, then this bug
might crop up again. (and it's possible a way to resolve the issue then is to
have canBecomeKeyWindow report YES rather than NO). But, maybe we'll have native
notifications by then.

The CL description still needs to be updated - please remove any mention of
apps.

I also wouldn't use a tricky repro as a justification for risk taking. However,
since we've narrowed it down to this `setBecomesKeyOnlyIfNeeded` line, then we
should talk about that instead.

[Sidney San Martín, 2016-10-11 13:54:40]

Description was changed from

==========
[Mac] Use a nonactivating panel for notifications so that clicking them doesn't
activate Chrome.

Chrome doesn't bring the sending tab to the foreground when a
notification is clicked (on purpose: it's up to the page to call
window.focus() if it wants to be made visible), so right now clicking a
notification will just bring the last-used Chrome window to the front.

Notifications were switched from NSPanel to NSWindow to work around
crbug.com/459306. I tried fairly hard to repro that issue, and couldn't.
Because apps have been deprecated and it seems to have always been
tricky to reproduce, I feel okay about maybe resurfacing that bug (and
either reverting this CL or taking another crack at the root cause
if it comes back).

BUG=653868
==========

to

==========
[Mac] Use a nonactivating panel for notifications so that mouse down doesn't
activate Chrome.

Currently, pressing the mouse button over a notification brings Chrome
to the front, even though the tab which sent the notification won't be
shown unless it calls `window.focus()` from its onclick listener.

This CL switches notifications to use NSPanel, which can receive mouse
events without activating the application.

Notifications were previously switched away from NSPanel to work around
crbug.com/459306. I tried fairly hard to repro that issue with this CL,
and couldn't. If it comes back, I feel okay about reverting this change
or taking another crack at its root cause.

BUG=653868
==========

[Avi (use Gerrit), 2016-10-11 15:15:29]

On 2016/10/11 03:55:44, tapted wrote:
> I'm
> pretty sure we don't put textfields in - but we _do_ have buttons. However,
said
> buttons are not keyboard accessible.

How does turning Full Keyboard Access on
(https://support.apple.com/en-us/HT204434#fullkeyboard) affect your analysis?

[Sidney San Martín, 2016-10-11 18:15:23]

The CQ bit was checked by sdy@chromium.org

[Sidney San Martín, 2016-10-11 18:15:24]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org
Link to the patchset: https://codereview.chromium.org/2393323005/#ps160001
(title: "Make dependent on bugfix CL")

[commit-bot: I haz the power, 2016-10-11 18:15:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-11 19:10:57]

Description was changed from

==========
[Mac] Use a nonactivating panel for notifications so that mouse down doesn't
activate Chrome.

Currently, pressing the mouse button over a notification brings Chrome
to the front, even though the tab which sent the notification won't be
shown unless it calls `window.focus()` from its onclick listener.

This CL switches notifications to use NSPanel, which can receive mouse
events without activating the application.

Notifications were previously switched away from NSPanel to work around
crbug.com/459306. I tried fairly hard to repro that issue with this CL,
and couldn't. If it comes back, I feel okay about reverting this change
or taking another crack at its root cause.

BUG=653868
==========

to

==========
[Mac] Use a nonactivating panel for notifications so that mouse down doesn't
activate Chrome.

Currently, pressing the mouse button over a notification brings Chrome
to the front, even though the tab which sent the notification won't be
shown unless it calls `window.focus()` from its onclick listener.

This CL switches notifications to use NSPanel, which can receive mouse
events without activating the application.

Notifications were previously switched away from NSPanel to work around
crbug.com/459306. I tried fairly hard to repro that issue with this CL,
and couldn't. If it comes back, I feel okay about reverting this change
or taking another crack at its root cause.

BUG=653868
==========

[commit-bot: I haz the power, 2016-10-11 19:10:58]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2016-10-11 19:13:24]

Description was changed from

==========
[Mac] Use a nonactivating panel for notifications so that mouse down doesn't
activate Chrome.

Currently, pressing the mouse button over a notification brings Chrome
to the front, even though the tab which sent the notification won't be
shown unless it calls `window.focus()` from its onclick listener.

This CL switches notifications to use NSPanel, which can receive mouse
events without activating the application.

Notifications were previously switched away from NSPanel to work around
crbug.com/459306. I tried fairly hard to repro that issue with this CL,
and couldn't. If it comes back, I feel okay about reverting this change
or taking another crack at its root cause.

BUG=653868
==========

to

==========
[Mac] Use a nonactivating panel for notifications so that mouse down doesn't
activate Chrome.

Currently, pressing the mouse button over a notification brings Chrome
to the front, even though the tab which sent the notification won't be
shown unless it calls `window.focus()` from its onclick listener.

This CL switches notifications to use NSPanel, which can receive mouse
events without activating the application.

Notifications were previously switched away from NSPanel to work around
crbug.com/459306. I tried fairly hard to repro that issue with this CL,
and couldn't. If it comes back, I feel okay about reverting this change
or taking another crack at its root cause.

BUG=653868

Committed: https://crrev.com/629a5d065848adbac50931ae305a3d8597578e4d
Cr-Commit-Position: refs/heads/master@{#424505}
==========

[commit-bot: I haz the power, 2016-10-11 19:13:27]

Patchset 8 (id:??) landed as
https://crrev.com/629a5d065848adbac50931ae305a3d8597578e4d
Cr-Commit-Position: refs/heads/master@{#424505}

[tapted, 2016-10-11 22:33:16]

On 2016/10/11 15:15:29, Avi wrote:
> On 2016/10/11 03:55:44, tapted wrote:
> > I'm
> > pretty sure we don't put textfields in - but we _do_ have buttons. However,
> said
> > buttons are not keyboard accessible.
> 
> How does turning Full Keyboard Access on
> (https://support.apple.com/en-us/HT204434#fullkeyboard) affect your analysis?

Nah - I did try that. The issue for keyboard access is that these popups return
`NO` from canBecomeKeyWindow. That combined with NSNonactivatingPanelMask
(documented as "The panel
can receive keyboard input without activating the owning application.") seemed
to be the trigger for the nasty two-windows-have-key-state bug. But, if you also
pile setBecomesKeyOnlyIfNeeded:YES on top of that, then the main repro case no
longer reproduces (when I tried it on 10.11 at least).

Note there was still an obscure repro that I never fixed --
http://crbug.com/595926 -- but it had nothing to do with notifications (and is
probably triggered by one of the special weird things we do for apps).