Closed Bug 1075089 Opened 9 years ago Closed 8 years ago

Mac context menus appear in (slightly) the wrong place


(Core :: XUL, defect)

32 Branch
Not set



Tracking Status
firefox43 --- fixed


(Reporter: kengruven, Assigned: nicko.robson, Mentored)



(Whiteboard: [good first bug])


(1 file, 6 obsolete files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:32.0) Gecko/20100101 Firefox/32.0
Build ID: 20140923175406

Steps to reproduce:

1. With Firefox 32 on OS X 10.9, open a browser window and go to any webpage that has a link.
2. Click a link, to go to a second page (so the Back button is active).
3. Right-click on an empty space in the middle of the page.
4. Move the mouse slowly down and right until the Back menu/icon is highlighted (and note how far you had to move it).

Actual results:

The contextual menu appears below and to the right of the mouse pointer, so I have to move the mouse to the right (2 pixels) and also down (6 pixels) to choose the first menu item.

Expected results:

On every other contextual menu on OS X, the menu appears directly to the right of the mouse pointer, so I only have to move the mouse to the right (and by only 1 pixel) to select the first item.

I don't think this is related to the new contextual menu design (bug 1000513), but for some reason it seems more apparent to me than before.

Using native contextual menus (bug 34572) would probably fix this for free, but it's a lot more work.
Blocks: 1000513
Component: Untriaged → XUL
Product: Firefox → Core
This is fairly simple to fix. The CONTEXT_MENU_OFFSET_PIXELS constant from nsMenuPopupFrame.h would need to be changed and separated into horizontal and vertical offsets. Since this will be platform specific, the constant should instead be a new value added to nsLookAndFeel::IntID.
Mentor: enndeakin
Ever confirmed: true
Whiteboard: [good first bug]
Can I work on this bug ?
Neil, could you mentor the user?
Flags: needinfo?(enndeakin)
Yes. The comment above describes what needs to be done. Let me know if you need some help.
Flags: needinfo?(enndeakin)
Hello, can I work on this bug? I'm new and it will be my first one.
Sure, let me know if you need help.
Assignee: nobody → nicko.robson
Great, thanks
Attached patch Initial patch for review (obsolete) — Splinter Review
Hello again. As mentioned in the above notes, I added new vertical and horizontal offset defaults to the LookAndFeel classes for each platform type, setting all of them to 2 px as they were originally. For Cocoa I changed them to 1 px horizontal and -4 px vertical so that it's possible to move the mouse only one pixel to the right and have it select the first row, if it's not disabled.
Attachment #8632390 - Flags: ui-review?
Attachment #8632390 - Flags: review?(enndeakin)
Flags: needinfo?(ux-review)
Attachment #8632391 - Flags: ui-review?(ux-review)
Thanks! I'll take a look at this on Tuesday.
Looks ok from a UX perspective
Flags: needinfo?(ux-review)
Comment on attachment 8632390 [details] [diff] [review]
Initial patch for review

>   nsDeviceContext* devContext = presContext->DeviceContext();
>-  nscoord offsetForContextMenu = 0;
>+  nsPoint offsetForContextMenu(0, 0);

You don't need the (0,0) as the values are initialized to 0 by default.

>-// XXX,, there are 4 definitions for the same purpose:
>-//  nsMenuPopupFrame.h, nsListControlFrame.cpp, listbox.xml, tree.xml
>-//  need to find a good place to put them together.
>-//  if someone changes one, please also change the other.

This comment is actually meant to be for the definition of nsMenuPopupFrame::sTimeoutOfIncrementalSearch (in nsMenuPopupFrame.cpp). We should move it there.

>--- a/widget/cocoa/
>+++ b/widget/cocoa/
>@@ -473,6 +473,12 @@ nsLookAndFeel::GetIntImpl(IntID aID, int32_t &aResult)
>     case eIntID_ColorPickerAvailable:
>       aResult = 1;
>       break;
>+    case eIntID_ContextMenuOffsetVertical:
>+      aResult = -6;
>+      break;
>+    case eIntID_ContextMenuOffsetHorizontal:
>+      aResult = 1;
>+      break;

Since this uses a negative value, it will cause an issue when the popup is flipped. For example, move the mouse pointer to the bottom pixel of the screen and open the context menu. The menu will be cut off and have a scrolling arrow.

The easiest fix though is to change the use of aOffsetForContextMenu at the lines below (around line 1170) in nsMenuPopupFrame.cpp to ensure that its value it not negative. You could use std::max here.

        // if the newly calculated position is different than the existing
        // position, we flip such that the popup is to the left or top of the
        // anchor point instead.
        nscoord newScreenPoint = startpos - aSize - aMarginBegin - aOffsetForContextMenu;
        if (newScreenPoint != aScreenPoint) {
          *aFlipSide = true;

Note: On Mac though, and only on Mac, the context menu should appear aligned with the bottom edge of the screen with a small margin if there isn't room to show it. Fixing that is an alternative solution, but that could be done in a different bug.

Otherwise, this patch looks good.
Thanks for the review, I should have the revised patch ready soon. As for the margin, you mentioned it's an alternative solution... does that mean there's a better way of doing what I'm doing which would result in the OSX's margin requirements being observed too?
No what you did is good. The issue only occurs when there isn't enough room to display the context menu (the context menu would hit the bottom of the screen). Windows, Linux and Mozilla flip the popup so that opens upwards, also using flipped margins, so the context menu's bottomleft corner appears near the mouse pointer. On Mac though, the right behaviour is to just push the context menu up so that it fits on the screen. But I don't expect you to fix that in this bug.
Attachment #8632390 - Attachment is obsolete: true
Attachment #8632390 - Flags: ui-review?
Attachment #8632390 - Flags: review?(enndeakin)
Attachment #8636263 - Flags: review?(enndeakin)
So I guess that means I broke the build. Running tests again locally...
OK, most of the failures there are just the tests assuming that the context menu appears at what the offset used to be. For example, test_popup_attribute.xul and test_popup_button.xul should be simple to fix. The value is hardcoded in popup_trigger.js

As an aside, there is one bug here though, which is revealed by test_contextmenu_list.xul. When a context menu is opened with the keyboard, it shouldn't be offset -6 pixels or the menu will overlap the focused element. That said, a user can't open a context menu with only the keyboard on Mac, so this probably isn't a big deal, and I wouldn't worry about it.

So we just need to fix up the tests to check for different offsets on Mac. It also means we don't need a separate test for this bug since the existing tests will handle checking this, which is convenient.
> That said, a user can't open a context menu with only the keyboard on Mac

Control-Numpad5 (or Numpad0), if the MouseKeys accessibility option is enabled.
> Control-Numpad5 (or Numpad0), if the MouseKeys accessibility option is
> enabled.

This key just emulates a mouse right-click, so it should appear the offset -6 pixels in the same way as the mouse. On a PC keyboard though, there's a key on the keyboard that opens a context menu. On Windows it opens the context menu aligned from the focused element. On Mac, the key does nothing. So this is ok too.
I've been running the mochitests for test_popup_attribute.xul, test_popup_button.xul and test_contextmenu_list.xul on a fresh checkout without much changes and found that they fail - the same off-by-a-few-pixels errors. Is there a possibility it they fail under a different versions of OSX due to differences in shadow or border size?
* without my changes
They could fail for those reasons. Unfortunately these tests can be quite finicky as they rely on lots of precise coordinate checks. This makes it difficult to test locally. Make sure your dock is positioned on the bottom and you have the window and screen resolution at a good size. You might also try non-retina resolutions as well.

We can also run changes on the try server. Let me know if you need help. If you're having trouble I can take a look at fixing the tests too.
Thanks. I'm installing Snow Leopard on VirtualBox, hopefully that fixes it!
Third patch submission, this time with passing mochitests.
Attachment #8636263 - Attachment is obsolete: true
Attachment #8636263 - Flags: review?(enndeakin)
Attachment #8639170 - Flags: review?(enndeakin)
Did you get a run on the try server? Would you like me to do one for you?
Flags: needinfo?(nicko.robson)
Hi Neil, yes can you please run them on the try server? I found an old Mac at home that still had 10.8 on it. Not as old as Snow Leopard but was before the big UI change so the tests passed...
Flags: needinfo?(nicko.robson)
Thanks Neil. Looks like I missed one. It has an early escape for all the versions of OSX I have. I'll try to work out how to fix it.
This should be the last version. When you have a few moments, can you please run it on the Try server again? Thanks for your patience.
Attachment #8639170 - Attachment is obsolete: true
Attachment #8639170 - Flags: review?(enndeakin)
Attachment #8640774 - Flags: review?(enndeakin)
Comment on attachment 8640774 [details] [diff] [review]

>   test: function(testname, step) {
>     gTrigger.focus();
>-    synthesizeKey((navigator.platform.indexOf("Mac") == -1) ? "VK_F4" : " ", { });
>+    synthesizeKey(!platformIsMac() ? "VK_F4" : " ", { });

It would be clearer to reverse the condition here.

>-    if (navigator.platform.indexOf("Mac") == -1)
>+    if (!platformIsMac())
>       synthesizeKey("", { metaKey: true });
>     else
>       synthesizeKey("VK_F4", { altKey: true });

Similarly here.

Otherwise, this is good! The tests look ok.
Attachment #8640774 - Flags: review?(enndeakin) → review+
Fixed remaining if and ternary statement order issues.
Attachment #8632391 - Attachment is obsolete: true
Attachment #8640774 - Attachment is obsolete: true
Attachment #8632391 - Flags: ui-review?(ux-review)
Attachment #8641306 - Flags: checkin?(enndeakin)
And thanks again for your continued patience :)
sorry had to back this out for test failures like
Flags: needinfo?(nicko.robson)
OK, so this worked well, but there's a failure on the test testing/marionette/client/marionette/tests/unit/ I didn't have that test run in the try pushes I did. (I'll make sure to update the commands I use to include them in future).

See for the failure.

I think the test is opening a context menu, then doing some more mouse click related tests. It's unclear if the test is accounting for the click that would hide the context menu or not. If the test is using something akin the synthesizeMouse then I would bet the context menu is still open during the remainder of test.

Since this patch moves the position of the context menu on Mac, I suspect the later clicks are targetting the context menu not the button as the test expects.

I don't know anything about these tests and the patch author isn't available, so I've set needinfo to the patch reviewer to help out here.
Comment on attachment 8641306 [details] [diff] [review]

In the future, please just use the checkin-needed bug keyword. Our automated bug marking tools work better with it. Thanks!
Attachment #8641306 - Flags: checkin?(enndeakin)
Ok thanks Ryan I'll do that next time. Thanks Neil & Pulsebot I'll fix this up asap!
Flags: needinfo?(nicko.robson)
This is turning out to be quite an involved first bug! 

As you suspected Neil the issue is that one of the earlier tests in the suite opens a context menu but never closes it again, blocking later tests from interacting with the page. I've tried a number of ways to close the menu such as firing further mouse events to various page elements and dispatching keyboard "escape" events but nothing seems to be received. Even firing another marionette "context menu" event at another part of the page doesn't seem to close the popup. Is there a marionette expert that I can take this up with? I'm sure I'm missing something simple.
I thought I had needinfo'd jgriffin for further help, but I guess I didn't. I did so now.
Flags: needinfo?(jgriffin)
The tests seem to expect that because ui.click_hold_context_menus is true, that simply releasing the mouse button should close the context menu.  But, I'm not an expert on this; is that now how that pref is supposed to work?

If the test is buggy, I'm ok with you disabling the test in order to land this, and then filing a follow-up and assigning to me to investigate and fix the test.
Flags: needinfo?(jgriffin)
Raised a bug for the Marionette context menu issue here:

Uploaded a new patch with the offending tests commented out in so Marionette tests now pass. 

Neil when you have a minute can you please run it on the Try server again?
Attachment #8641306 - Attachment is obsolete: true
Keywords: checkin-needed
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Depends on: 1216284
I'm playing around with Firefox 50.1.0 now, and there's something funny about this still.

After right clicking, I either need to move the mouse 0.5 pixels or 1.5 pixels to the right, to select the top item in the context menu.

That is, sometimes I right-click, and move the mouse very slightly to the right, and the Back menu icon highlights, even though the pointer has not yet moved.  Other times, when I move the mouse slightly to the right, it jumps by one pixel and the item does not yet highlight, but then I move it slightly more and the menu highlights even though the pointer has not moved a second pixel.

I can wiggle the mouse left and right, and highlight and un-highlight the menu item, without the pointer visibly moving.  It's clearly tracking some sub-pixel position that does not correspond to the visible location of the mouse pointer.

One consequence of this is that the menu, in all cases, starts to highlight before the pointer is visibly over it.  The Mac doesn't consider the dark gray border to be part of the active part of the menu item, but in Firefox 50 context menus, it (half) is.
You need to log in before you can comment on or make changes to this bug.