Bug 1831760 - Use a native popup menu for positioned popups on Mac. r=dao,mstange

This does two things:

* Modify `nsXULPopupManager::ShowPopup()` so it calls `ShowPopupAsNativeMenu()`
  as long as an anchor wasn't passed in, and only on Mac.
* Modify `-[MOZMenuOpeningCoordinator _openMenu:atScreenPosition:forView:withAppearance:]`
  so it also takes a `aIsContextMenu` param. If the param is true, we synthesize
  a right-click event and pop up a context menu as usual. If it's false, we use
  `-[NSMenu popUpMenuPositioningItem:atLocation:inView:]` instead.

The reason this works is because `-[NSMenu popUpMenuPositioningItem:atLocation:inView:]`
opens the menu in a sensible place when the x-y coords are near the right edge
of the screen. In contrast, `+[NSMenu popUpContextMenu:withEvent:forView:]` will
anchor the menu's top-right corner to the mouse cursor when near the right edge.

Differential Revision: https://phabricator.services.mozilla.com/D177355
This commit is contained in:
Drew Willcoxon 2023-05-18 05:51:19 +00:00
parent bfacab9393
commit 929e1e4d77
11 changed files with 136 additions and 43 deletions

View File

@ -540,7 +540,7 @@ add_task(async function testArowsContext() {
await expectFocusAfterKey("ArrowDown", gMainButton1);
let shown = BrowserTestUtils.waitForEvent(gMainContext, "popupshown");
// There's no cross-platform way to open a context menu from the keyboard.
gMainContext.openPopup();
gMainContext.openPopup(gMainButton1);
await shown;
let item = gMainContext.children[0];
ok(

View File

@ -2,6 +2,7 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
import { AppConstants } from "resource://gre/modules/AppConstants.sys.mjs";
import { XPCOMUtils } from "resource://gre/modules/XPCOMUtils.sys.mjs";
const lazy = {};
@ -841,7 +842,26 @@ export class UrlbarView {
openResultMenu(result, anchor) {
this.#resultMenuResult = result;
this.resultMenu.openPopup(anchor, "bottomright topright");
if (AppConstants.platform == "macosx") {
// `openPopup(anchor)` doesn't use a native context menu, which is very
// noticeable on Mac. Use `openPopup()` with x and y coords instead. See
// bug 1831760 and bug 1710459.
let rect = getBoundsWithoutFlushing(anchor);
rect = this.window.windowUtils.toScreenRectInCSSUnits(
rect.x,
rect.y,
rect.width,
rect.height
);
this.resultMenu.openPopup(null, {
x: rect.x,
y: rect.y + rect.height,
});
} else {
this.resultMenu.openPopup(anchor, "bottomright topright");
}
anchor.toggleAttribute("open", true);
let listener = event => {
if (event.target == this.resultMenu) {

View File

@ -329,12 +329,24 @@ export var UrlbarTestUtils = {
throw new Error("Submenu item not found for selector: " + selector);
}
this._testScope?.info("Clicking submenu item with selector: " + selector);
let promisePopup = lazy.BrowserTestUtils.waitForEvent(
window.gURLBar.view.resultMenu,
"popupshown"
);
this.EventUtils.synthesizeMouseAtCenter(menuitem, {}, window);
if (AppConstants.platform == "macosx") {
// Synthesized clicks don't work in the native Mac menu.
this._testScope?.info(
"Calling openMenu() on submenu item with selector: " + selector
);
menuitem.openMenu(true);
} else {
this._testScope?.info(
"Clicking submenu item with selector: " + selector
);
this.EventUtils.synthesizeMouseAtCenter(menuitem, {}, window);
}
this._testScope?.info("Waiting for submenu popupshown event");
await promisePopup;
this._testScope?.info("Got the submenu popupshown event");
@ -380,21 +392,33 @@ export var UrlbarTestUtils = {
openByMouse = false,
} = {}
) {
await this.openResultMenuAndGetItem({
let menuitem = await this.openResultMenuAndGetItem({
accesskey,
resultIndex,
openByMouse,
window: win,
});
if (!menuitem) {
throw new Error("Menu item not found for accesskey: " + accesskey);
}
this._testScope?.info(
`pressing access key (${accesskey}) to activate menu item`
);
let promiseCommand = lazy.BrowserTestUtils.waitForEvent(
win.gURLBar.view.resultMenu,
"command"
);
this.EventUtils.synthesizeKey(accesskey, {}, win);
if (AppConstants.platform == "macosx") {
// The native Mac menu doesn't support access keys.
this._testScope?.info("calling doCommand() to activate menu item");
menuitem.doCommand();
win.gURLBar.view.resultMenu.hidePopup(true);
} else {
this._testScope?.info(
`pressing access key (${accesskey}) to activate menu item`
);
this.EventUtils.synthesizeKey(accesskey, {}, win);
}
this._testScope?.info("waiting for command event");
await promiseCommand;
this._testScope?.info("got the command event");
@ -442,12 +466,21 @@ export var UrlbarTestUtils = {
throw new Error("Menu item not found for command: " + command);
}
this._testScope?.info("Clicking menu item with command: " + command);
let promiseCommand = lazy.BrowserTestUtils.waitForEvent(
win.gURLBar.view.resultMenu,
"command"
);
this.EventUtils.synthesizeMouseAtCenter(menuitem, {}, win);
if (AppConstants.platform == "macosx") {
// Synthesized clicks don't work in the native Mac menu.
this._testScope?.info("calling doCommand() to activate menu item");
menuitem.doCommand();
win.gURLBar.view.resultMenu.hidePopup(true);
} else {
this._testScope?.info("Clicking menu item with command: " + command);
this.EventUtils.synthesizeMouseAtCenter(menuitem, {}, win);
}
this._testScope?.info("Waiting for command event");
await promiseCommand;
this._testScope?.info("Got the command event");

View File

@ -810,11 +810,34 @@ void nsXULPopupManager::ShowMenu(nsIContent* aMenu, bool aSelectFirstItem) {
BeginShowingPopup(pendingPopup, parentIsContextMenu, aSelectFirstItem);
}
static bool ShouldUseNativeContextMenus() {
#ifdef HAS_NATIVE_MENU_SUPPORT
return mozilla::widget::NativeMenuSupport::ShouldUseNativeContextMenus();
#else
return false;
#endif
}
void nsXULPopupManager::ShowPopup(Element* aPopup, nsIContent* aAnchorContent,
const nsAString& aPosition, int32_t aXPos,
int32_t aYPos, bool aIsContextMenu,
bool aAttributesOverride,
bool aSelectFirstItem, Event* aTriggerEvent) {
#ifdef XP_MACOSX
// On Mac, use a native menu if possible since the non-native menu looks out
// of place. Native menus for anchored popups are not currently implemented,
// so fall back to the non-native path below if `aAnchorContent` is given. We
// also fall back if the position string is not empty so we don't break tests
// that either themselves call or test app features that call
// `openPopup(null, "position")`.
if (!aAnchorContent && aPosition.IsEmpty() && ShouldUseNativeContextMenus() &&
aPopup->IsAnyOfXULElements(nsGkAtoms::menu, nsGkAtoms::menupopup) &&
ShowPopupAsNativeMenu(aPopup, aXPos, aYPos, aIsContextMenu,
aTriggerEvent)) {
return;
}
#endif
nsMenuPopupFrame* popupFrame = GetPopupFrameForContent(aPopup, true);
if (!popupFrame || !MayShowPopup(popupFrame)) {
return;
@ -830,14 +853,6 @@ void nsXULPopupManager::ShowPopup(Element* aPopup, nsIContent* aAnchorContent,
BeginShowingPopup(pendingPopup, aIsContextMenu, aSelectFirstItem);
}
static bool ShouldUseNativeContextMenus() {
#ifdef HAS_NATIVE_MENU_SUPPORT
return mozilla::widget::NativeMenuSupport::ShouldUseNativeContextMenus();
#else
return false;
#endif
}
void nsXULPopupManager::ShowPopupAtScreen(Element* aPopup, int32_t aXPos,
int32_t aYPos, bool aIsContextMenu,
Event* aTriggerEvent) {
@ -912,7 +927,8 @@ bool nsXULPopupManager::ShowPopupAsNativeMenu(Element* aPopup, int32_t aXPos,
if (!frame) {
frame = presContext->PresShell()->GetRootFrame();
}
mNativeMenu->ShowAsContextMenu(frame, CSSIntPoint(aXPos, aYPos));
mNativeMenu->ShowAsContextMenu(frame, CSSIntPoint(aXPos, aYPos),
aIsContextMenu);
// While the native menu is open, it consumes mouseup events.
// Clear any :active state, mouse capture state and drag tracking now.

View File

@ -31,7 +31,8 @@ class NativeMenu {
// This call assumes that the popupshowing event for the root popup has
// already been sent and "approved", i.e. preventDefault() was not called.
virtual void ShowAsContextMenu(nsIFrame* aClickedFrame,
const CSSIntPoint& aPosition) = 0;
const CSSIntPoint& aPosition,
bool aIsContextMenu) = 0;
// Close the menu and synchronously fire popuphiding / popuphidden events.
// Returns false if the menu wasn't open.

View File

@ -33,7 +33,8 @@ class Runnable;
- (NSInteger)asynchronouslyOpenMenu:(NSMenu*)aMenu
atScreenPosition:(NSPoint)aPosition
forView:(NSView*)aView
withAppearance:(NSAppearance*)aAppearance;
withAppearance:(NSAppearance*)aAppearance
asContextMenu:(BOOL)aIsContextMenu;
// If the menu opening request for aHandle hasn't been processed yet, cancel it.
// Can only be called on the main thread.

View File

@ -27,6 +27,7 @@ static BOOL sNeedToUnwindForMenuClosing = NO;
@property NSPoint position;
@property(retain) NSView* view;
@property(retain) NSAppearance* appearance;
@property BOOL isContextMenu;
@end
@implementation MOZMenuOpeningInfo
@ -64,7 +65,8 @@ static BOOL sNeedToUnwindForMenuClosing = NO;
- (NSInteger)asynchronouslyOpenMenu:(NSMenu*)aMenu
atScreenPosition:(NSPoint)aPosition
forView:(NSView*)aView
withAppearance:(NSAppearance*)aAppearance {
withAppearance:(NSAppearance*)aAppearance
asContextMenu:(BOOL)aIsContextMenu {
MOZ_RELEASE_ASSERT(!mPendingOpening,
"A menu is already waiting to open. Before opening the next one, either wait "
"for this one to open or cancel the request.");
@ -77,6 +79,7 @@ static BOOL sNeedToUnwindForMenuClosing = NO;
info.position = aPosition;
info.view = aView;
info.appearance = aAppearance;
info.isContextMenu = aIsContextMenu;
mPendingOpening = [info retain];
[info release];
@ -102,7 +105,8 @@ static BOOL sNeedToUnwindForMenuClosing = NO;
[self _openMenu:info.menu
atScreenPosition:info.position
forView:info.view
withAppearance:info.appearance];
withAppearance:info.appearance
asContextMenu:info.isContextMenu];
} @catch (NSException* exception) {
nsObjCExceptionLog(exception);
}
@ -126,7 +130,8 @@ static BOOL sNeedToUnwindForMenuClosing = NO;
- (void)_openMenu:(NSMenu*)aMenu
atScreenPosition:(NSPoint)aPosition
forView:(NSView*)aView
withAppearance:(NSAppearance*)aAppearance {
withAppearance:(NSAppearance*)aAppearance
asContextMenu:(BOOL)aIsContextMenu {
// There are multiple ways to display an NSMenu as a context menu.
//
// 1. We can return the NSMenu from -[ChildView menuForEvent:] and the NSView will open it for
@ -149,8 +154,10 @@ static BOOL sNeedToUnwindForMenuClosing = NO;
// NativeMenuMac::ShowAsContextMenu can be called at any time. It could be called during a
// menuForEvent call (during a "contextmenu" event handler), or during a mouseDown handler, or at
// a later time.
// The code below uses option 4 as the preferred option because it's the simplest: It works in all
// scenarios and it doesn't have the positioning drawbacks of option 5.
// The code below uses option 4 as the preferred option for context menus because it's the
// simplest: It works in all scenarios and it doesn't have the drawbacks of option 5. For popups
// that aren't context menus and that should be positioned as close as possible to the given
// screen position, we use option 5.
if (aAppearance) {
#if !defined(MAC_OS_VERSION_11_0) || MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_VERSION_11_0
@ -167,18 +174,28 @@ static BOOL sNeedToUnwindForMenuClosing = NO;
if (aView) {
NSWindow* window = aView.window;
// Create a synthetic event at the right location and open the menu [option 4].
NSPoint locationInWindow = nsCocoaUtils::ConvertPointFromScreen(window, aPosition);
NSEvent* event = [NSEvent mouseEventWithType:NSEventTypeRightMouseDown
location:locationInWindow
modifierFlags:0
timestamp:NSProcessInfo.processInfo.systemUptime
windowNumber:window.windowNumber
context:nil
eventNumber:0
clickCount:1
pressure:0.0f];
[NSMenu popUpContextMenu:aMenu withEvent:event forView:aView];
if (aIsContextMenu) {
// Create a synthetic event at the right location and open the menu [option 4].
NSEvent* event = [NSEvent mouseEventWithType:NSEventTypeRightMouseDown
location:locationInWindow
modifierFlags:0
timestamp:NSProcessInfo.processInfo.systemUptime
windowNumber:window.windowNumber
context:nil
eventNumber:0
clickCount:1
pressure:0.0f];
[NSMenu popUpContextMenu:aMenu withEvent:event forView:aView];
} else {
// For popups which are not context menus, we open the menu using [option
// 5]. We pass `nil` to indicate that we're positioning the top left
// corner of the menu. This path is used for anchored menupopups, so we
// prefer option 5 over option 4 so that the menu doesn't get flipped if
// space is tight.
NSPoint locationInView = [aView convertPoint:locationInWindow fromView:nil];
[aMenu popUpMenuPositioningItem:nil atLocation:locationInView inView:aView];
}
} else {
// Open the menu using popUpMenuPositioningItem:atLocation:inView: [option 5].
// This is not preferred, because it positions the menu differently from how a native context

View File

@ -29,7 +29,8 @@ class NativeMenuMac : public NativeMenu,
explicit NativeMenuMac(dom::Element* aElement);
// NativeMenu
void ShowAsContextMenu(nsIFrame* aClickedFrame, const CSSIntPoint& aPosition) override;
void ShowAsContextMenu(nsIFrame* aClickedFrame, const CSSIntPoint& aPosition,
bool aIsContextMenu) override;
bool Close() override;
void ActivateItem(dom::Element* aItemElement, Modifiers aModifiers, int16_t aButton,
ErrorResult& aRv) override;

View File

@ -237,7 +237,8 @@ static NSAppearance* NativeAppearanceForContent(nsIContent* aContent) {
return NSAppearanceForColorScheme(LookAndFeel::ColorSchemeForFrame(f));
}
void NativeMenuMac::ShowAsContextMenu(nsIFrame* aClickedFrame, const CSSIntPoint& aPosition) {
void NativeMenuMac::ShowAsContextMenu(nsIFrame* aClickedFrame, const CSSIntPoint& aPosition,
bool aIsContextMenu) {
nsPresContext* pc = aClickedFrame->PresContext();
auto cssToDesktopScale =
pc->CSSToDevPixelScale() / pc->DeviceContext()->GetDesktopToDeviceScale();
@ -255,7 +256,8 @@ void NativeMenuMac::ShowAsContextMenu(nsIFrame* aClickedFrame, const CSSIntPoint
mOpeningHandle = [MOZMenuOpeningCoordinator.sharedInstance asynchronouslyOpenMenu:menu
atScreenPosition:locationOnScreen
forView:view
withAppearance:appearance];
withAppearance:appearance
asContextMenu:aIsContextMenu];
}
bool NativeMenuMac::Close() {

View File

@ -356,7 +356,8 @@ NativeMenuGtk::~NativeMenuGtk() {
RefPtr<dom::Element> NativeMenuGtk::Element() { return mMenuModel->Element(); }
void NativeMenuGtk::ShowAsContextMenu(nsIFrame* aClickedFrame,
const CSSIntPoint& aPosition) {
const CSSIntPoint& aPosition,
bool aIsContextMenu) {
if (mMenuModel->IsShowing()) {
return;
}

View File

@ -30,7 +30,8 @@ class NativeMenuGtk : public NativeMenu {
// NativeMenu
MOZ_CAN_RUN_SCRIPT_BOUNDARY void ShowAsContextMenu(
nsIFrame* aClickedFrame, const CSSIntPoint& aPosition) override;
nsIFrame* aClickedFrame, const CSSIntPoint& aPosition,
bool aIsContextMenu) override;
bool Close() override;
void ActivateItem(dom::Element* aItemElement, Modifiers aModifiers,
int16_t aButton, ErrorResult& aRv) override;