Yeah this is annoying - we might need to get creative with this one, or borrow some ideas from WebKit. E.g. this is consistent with how combos in the content area behave in Blink. WebKit does a better job - basically combobox menus get a different menu animation that doesn't fade out, so you never see the old text, and that's more consistent with combox in, e.g., System Prefs.

We should be able to poke around in the WebKit source for this and emulate what they're doing, then look at improving this for the content area as well.

Moving this nonessential bug to the next milestone.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

I had a bit more of a look at this, and it's pretty subtle, so it's probably not P1. We seem to have the same animation that the system popups have now.

shrike@, do you still see this issue?

The system popups animate the new title into the popup button. In MacViews, the popup window disappears and then after 250-500ms the button title changes. The delay looks terrible and we should not ship it this way.

The native menu blocks the UI thread in annoying places (and doesn't pump events), but the menu animation itself seems to be threaded. So we just need to do as much work as we can in the menu delegate callbacks (and do our own blocking), before the method that shows the menu returns.

Something like this: https://codereview.chromium.org/2852233002

Orthogonal to the title changing is the style of menu close animation. If we want to do what native popup menus do, we can use a dummy NSPopUpButton, and performClick: on it. Like,

    base::scoped_nsobject<NSPopUpButton> anchor_view(
        CreateMenuAnchorView(parent->GetNativeWindow(), bounds, checked_item));
    NSMenu* menu = [menu_controller_ menu];
    [menu setMinimumWidth:bounds.width() + kNativeCheckmarkWidth];
    [anchor_view setMenu:menu];
    [anchor_view selectItem:checked_item];
    [anchor_view performClick:nil];


Or we can do what happens under the hood with that performClick and call the private @selector and pass the same/similar flags/options. I'm assuming one of these flags is "animate like a popup".

-[NSCarbonMenuImpl popUpMenu:atLocation:width:forView:withSelectedItem:withFont:withFlags:withOptions:]

that's the method that's called from -[NSPopUpButtonCell trackMouse:inRect:ofView:untilMouseUp:], but there are some other methods taking "flags" that might be simpler to use:

NSMenu
- (BOOL)popUpMenuPositioningItem:(id)arg1 atLocation:(struct CGPoint)arg2 inView:(id)arg3 appearance:(id)arg4

NSCarbonMenuImpl
- (void)popUpMenu:(id)arg1 atLocation:(struct CGPoint)arg2 width:(double)arg3 forView:(id)arg4 withSelectedItem:(long long)arg5 withFont:(id)arg6 withFlags:(unsigned long long)arg7 withOptions:(id)arg8;
- (void)popUpMenu:(id)arg1 atLocation:(struct CGPoint)arg2 width:(double)arg3 forView:(id)arg4 withSelectedItem:(long long)arg5 withFont:(id)arg6;
- (BOOL)_popUpMenuPositioningItem:(id)arg1 atCocoaIndex:(unsigned long long)arg2 atLocation:(struct CGPoint)arg3 inView:(id)arg4 withPrivateFlags:(unsigned long long)arg5 appearance:(id)arg6;

There's some pretty detailed investigation regarding these animations in issue 651203, as well as in the sub-bugs of issue 640466.

Also, did you look at NSMenuWillSendActionNotification? There's no equivalent delegate method, but I think it *may* be posted at the right time.

NSMenuWillSendActionNotification is sent between menuDidClose and [item action], which is too late. In one trace (code: https://codereview.chromium.org/2863883002), the times are

 t = 0: leftMouseUp seen in EventTap
0.25ms: -[NSMenuItem _sendItemSelectedNote] seen
+344ms: -[NSMenuDelegate menuDidClose:] seen
+347ms: NSMenuWillSendActionNotification seen
+348ms: [item action] received
+449ms: [NSMenu popUpMenuPositioningItem:...] returns.


I haven't found another promising hook that is sent before menuDidClose. (There's a crapton of ObjC method calls.. but nothing else stood out - it's truly bizarre all the stuff AppKit feels it needs to do to run a menu).

I recorded a video to see whether the item flash and menu fadeout are split across those two timeline blocks, but they're not. menuDidClose: is received only after the menu has fully faded out. So I have no idea what's happening in that 100ms until popUpMenuPositioningItem: returns and we have our RunLoop back. This means that pumping frames in [item action] could get us a 100ms jump slightly better responsiveness, but you still see the old item since the menu has fully faded out.


[79514:775:0505/110826.316325:INFO:menu_runner_impl_cocoa.mm(136)] event: 2
[79514:775:0505/110826.316592:INFO:menu_controller.mm(294)] early
[79514:775:0505/110826.660704:INFO:menu_controller.mm(269)] did close
[79514:775:0505/110826.664053:INFO:menu_controller.mm(280)] will send action
[79514:775:0505/110826.664178:INFO:menu_controller.mm(218)] got action
[79514:775:0505/110826.765470:INFO:menu_runner_impl_cocoa.mm(175)] method return

Hrm, okay. I poked at this a bit more today and took another gander at your CL in #9. I've come up with a few other options:

1) Swizzle -[NSCarbonMenuImpl _carbonMenuItemSelectedEvent:(EventRef)event handlerCallRef:(EventHandlerRef)handler] and dispatch our own delegate method. I've verified that this gets called very early in the event dispatch code.

An implementation like this seems to work pretty well (swizzling not pictured):

- (void)_carbonMenuItemSelectedEvent:(EventRef)event handlerCallRef:(EventHandlerRef)handler {
  NSMenu* menuTarget = [self menu];
  id delegate = [menuTarget delegate];
  if ([delegate conformsToProtocol:@protocol(MenuItemEarlyDispatch)]) {
    [delegate menu:menuTarget willSelectItem:[self targetedItem]];
  }
  g_invoke(self, _cmd, event, handler);  // Swizzle'd IMP.
}

2) Do what is discussed in issue 651203 and pump our MessageLoop work in private AppKit modes. I talked again with Mark about this and it's not entirely unreasonable. The approach in (1) would still require us to do the hacks to get the compositor to update in time, so this approach is maybe a bit cleaner.

We could even do a (2a) and introduce a scoper for MessagePumpMac that only temporary adds the callouts to the private runloop modes, and then removes them when it goes out of scope.

I haven't prototyped that this fixes the MacViews problem, but can you see if patching in https://codereview.chromium.org/2118883002/ does?

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

commit 335d78a0e0299bda3465cb7e303b79e043f680da
Author: tapted <tapted@chromium.org>
Date: Thu May 11 01:19:30 2017

Mac[Views]: Make native menus more responsive by pumping private runloop modes.

Native NSMenus run a UI-thread-blocking animation to flash the selected
item and fade out. This takes 300ms or more and events are only pumped
in private run loop modes. This can make menus feel really laggy -
<select> popups in particular.

A menu run by a native NSPopUpButton feels better since it uses a
different menu close animation which is opaque. Since it's opaque you
don't see how laggy the interface is behind it whilst the menu becomes
transparent. Using this animation is possible in MacViews, but its style
is fixed with a blue bar on the right, to match the native NSPopUpButton
appearance, so it looks weird. Changing any of the NSPopUpButtonCell's
style will just revert to a fade-out menu animation.

To get the menus feeling more responsive, "peek" at the processing on
the NSMenuItem by hooking -[NSMenuItem _sendItemSelectedNote]. This is
sent early in the event processing and before any animations start
blocking the UI thread. MenuController can peek at this and (optionally)
post the action handling as a task. Then, observe the private run loop
modes that AppKit uses during menu fade and "un" highlight of the menu
bar. This allows the posted task to execute and compositing tasks to run
in parallel with the menu fade.

BUG= 602914 , 640466
TEST=With chrome://flags#secondary-ui-md enabled, click the bookmark
star. Changing the Folder: should be immediately visible while the menu
is fading out.

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

[modify] https://crrev.com/335d78a0e0299bda3465cb7e303b79e043f680da/base/message_loop/message_pump_mac.mm
[modify] https://crrev.com/335d78a0e0299bda3465cb7e303b79e043f680da/chrome/browser/ui/cocoa/extensions/browser_action_button_interactive_uitest.mm
[modify] https://crrev.com/335d78a0e0299bda3465cb7e303b79e043f680da/ui/base/cocoa/menu_controller.h
[modify] https://crrev.com/335d78a0e0299bda3465cb7e303b79e043f680da/ui/base/cocoa/menu_controller.mm
[modify] https://crrev.com/335d78a0e0299bda3465cb7e303b79e043f680da/ui/base/cocoa/menu_controller_unittest.mm
[modify] https://crrev.com/335d78a0e0299bda3465cb7e303b79e043f680da/ui/views/controls/menu/menu_runner_impl_cocoa.mm

r470769 is in Version 60.0.3097.0 Canary and fixes this neatly. Let's hope it sticks :).

We can also explore other things we may want to opt in to [menu_controller_ setPostItemSelectedAsTask:YES];. E.g. <select> elements on web pages, which also suffer from this. (they may have gotten a 100ms improvement with r470769, but the lag is still quite noticeable without setPostItemSelectedAsTask:YES).

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

commit 08c4e3028cf0a15630a07874da6eec8694682ce4
Author: tapted <tapted@chromium.org>
Date: Thu May 25 10:43:53 2017

Mac: Disable r470769. I.e., Don't pump chrome tasks in private message loop modes.

It interacts badly with NSMenus on the native print dialog.

To fix, we may need to use a scoping class that adds these modes only
for menus that Chrome runs. Defer that to m61.

BUG= 726200 ,  602914 , 640466
TEST=Click the "Print using system dialog..." link in Print Preview,
then select "Save as PDF" via the [PDF] button in the native print
dialog. Chrome shouldn't hang.

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

[modify] https://crrev.com/08c4e3028cf0a15630a07874da6eec8694682ce4/base/message_loop/message_pump_mac.mm

For the animation style, I tried creating a regular NSPopupButton, then calling `button.transparent = YES`, which is documented to do this:

> A transparent button never draws itself, but it receives mouse events, sends its action, and tracks the mouse properly.

It does the right thing. What if, in the mouse handler, we created a transparent NSPopupButton on the fly, gave it the event, and then destroyed it afterwards? Could work for web content too. (This doesn't address pumping frames, but I'm hopeful that we can move that to another thread and not worry about the main event loop — see issue 640466).

Deleted: transparent_popup_button.gif 136 KB

	transparent_popup_button.gif 136 KB View Download

…Okay, I'm now realizing that this is basically what you originally did in crrev/2852233002 :). Sorry!

I don't know how to triage MacViews in depth, so not changing owner/priority.  This hasn't been touched in a while; please retriage.

I'm still going on this. There's probably a ~day to decompile enough of AppKit to figure out why the solution doesn't work in modal windows. So I guess the "active" task is  Issue 726200 .

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

commit 24ab401b3662d052facf19a169f553ea9a6bf7e1
Author: Trent Apted <tapted@chromium.org>
Date: Wed Sep 20 11:34:22 2017

Re-enable listening on private runloop modes, for a whitelist.

r470769 started observing private AppKit run loop modes to keep
UI and web pages responsive while menus were fading out, but it
interacted badly with app-modal dialogs.

This re-enables r470769, but only in 3 places:
 - Web page context menus
 - Web page <select> menus
 - menus run from toolkit-views UI.

      pause briefly when either the <select> menu or the right-
      click context menu are fading out. (The main menubar and the
      Chrome app/wrench menu still cause a brief pause when dismissed).

Bug:  726200 ,  602914 , 640466, 652007
Test: https://jsfiddle.net/kvmy8q9d/5/ - the animation shouldn't
Change-Id: Idf769c1f830ccb3ddc67205acb9e86aac2ec0460
Reviewed-on: https://chromium-review.googlesource.com/647836
Reviewed-by: Mark Mentovai <mark@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#503112}
[modify] https://crrev.com/24ab401b3662d052facf19a169f553ea9a6bf7e1/base/message_loop/message_pump_mac.h
[modify] https://crrev.com/24ab401b3662d052facf19a169f553ea9a6bf7e1/base/message_loop/message_pump_mac.mm
[modify] https://crrev.com/24ab401b3662d052facf19a169f553ea9a6bf7e1/base/message_loop/message_pump_mac_unittest.cc
[modify] https://crrev.com/24ab401b3662d052facf19a169f553ea9a6bf7e1/chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm
[modify] https://crrev.com/24ab401b3662d052facf19a169f553ea9a6bf7e1/content/browser/renderer_host/webmenurunner_mac.mm
[modify] https://crrev.com/24ab401b3662d052facf19a169f553ea9a6bf7e1/ui/views/controls/menu/menu_runner_impl_cocoa.mm