1075089 - Mac context menus appear in (slightly) the wrong place

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Ken Harris]

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.

Comment 1

[Neil Deakin]

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.

Comment 2

[Kushan Joshi [:kepta]]

Hi,
Can I work on this bug ?

Comment 3

[Loic]

Neil, could you mentor the user?

Comment 4

[Neil Deakin]

Yes. The comment above describes what needs to be done. Let me know if you need some help.

Comment 5

[nicko.robson]

Hello, can I work on this bug? I'm new and it will be my first one.

Comment 6

[Neil Deakin]

Sure, let me know if you need help.

Comment 7

[nicko.robson]

Great, thanks

Comment 8

[nicko.robson]

Attachment: Initial patch for 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.

Comment 9

[nicko.robson]

Attachment: Video of cursor and popup behavior on OS X

Comment 10

[Neil Deakin]

Thanks! I'll take a look at this on Tuesday.

Comment 11

[Ash Grigas]

Looks ok from a UX perspective

Comment 12

[Neil Deakin]

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, [e-mail], 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/nsLookAndFeel.mm
>+++ b/widget/cocoa/nsLookAndFeel.mm
>@@ -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.

Comment 13

[nicko.robson]

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?

Comment 14

[Neil Deakin]

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.

Comment 15

[nicko.robson]

Attachment: Second pass at patch including code review suggestions

Comment 16

[Neil Deakin]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5dc70d878f5

Comment 17

[nicko.robson]

So I guess that means I broke the build. Running tests again locally...

Comment 18

[Neil Deakin]

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.

Comment 19

[Ken Harris]

> 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.

Comment 20

[Neil Deakin]

> 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.

Comment 21

[nicko.robson]

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?

Comment 22

[nicko.robson]

* without my changes

Comment 23

[Neil Deakin]

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.

Comment 24

[nicko.robson]

Thanks. I'm installing Snow Leopard on VirtualBox, hopefully that fixes it!

Comment 25

[nicko.robson]

Attachment: 0001-Bug-1075089-Moved-popup-menu-frame-offset-to-LookAnd-V3.patch

Third patch submission, this time with passing mochitests.

Comment 26

[Neil Deakin]

Did you get a run on the try server? Would you like me to do one for you?

Comment 27

[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...

Comment 28

[Neil Deakin]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f12cfac7b0db

Comment 29

[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.

Comment 30

[nicko.robson]

Attachment: 0001-Bug-1075089-Moved-popup-menu-frame-offset-to-LookAnd-V4.patch

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.

Comment 31

[Neil Deakin]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4dddec9b6f1

Comment 32

[Neil Deakin]

Comment on attachment 8640774 [details] [diff] [review]
0001-Bug-1075089-Moved-popup-menu-frame-offset-to-LookAnd-V4.patch

>   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.

Comment 33

[nicko.robson]

Attachment: 0001-Bug-1075089-Moved-popup-menu-frame-offset-to-LookAnd-V5.patch

Fixed remaining if and ternary statement order issues.

Comment 34

[nicko.robson]

And thanks again for your continued patience :)

Comment 35

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7edc58c272f1

Comment 36

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=12356769&repo=mozilla-inbound

Comment 37

[Neil Deakin]

OK, so this worked well, but there's a failure on the test testing/marionette/client/marionette/tests/unit/test_single_finger_desktop.py 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 https://treeherder.mozilla.org/logviewer.html#?job_id=12357000&repo=mozilla-inbound 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 38

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf4c24bd6301

Comment 39

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8641306 [details] [diff] [review]
0001-Bug-1075089-Moved-popup-menu-frame-offset-to-LookAnd-V5.patch

In the future, please just use the checkin-needed bug keyword. Our automated bug marking tools work better with it. Thanks!

Comment 40

[nicko.robson]

Ok thanks Ryan I'll do that next time. Thanks Neil & Pulsebot I'll fix this up asap!

Comment 41

[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.

Comment 42

[Neil Deakin]

I thought I had needinfo'd jgriffin for further help, but I guess I didn't. I did so now.

Comment 43

[Jonathan Griffin (: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.

Comment 44

[nicko.robson]

Attachment: 0001-Bug-1075089-Moved-popup-menu-frame-offset-to-LookAnd-V6.patch

Raised a bug for the Marionette context menu issue here: https://bugzilla.mozilla.org/show_bug.cgi?id=1191066

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

Neil when you have a minute can you please run it on the Try server again?

Comment 45

[Neil Deakin]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c449fb1ceb94

Comment 46

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a68c49ad5acc

Comment 47

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a68c49ad5acc

Comment 48

[Ken Harris]

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.