Closed Bug 1194337 Opened 7 years ago Closed 7 years ago

Context menus opened close to the right side of the screen put first item under mouse cursor

Categories

(Core :: XUL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox42 --- unaffected
firefox43 + fixed

People

(Reporter: mconley, Assigned: nicko.robson)

References

Details

(Keywords: regression)

Attachments

(2 files, 5 obsolete files)

STR:

1) On a Nightly on OS X, open many tabs. So many tabs that you have tabs filling the whole tab strip.
2) Right click on the right-most tab.

ER:

A context menu should open and let you choose an item.

AR:

A context menu opens, but it's placed immediately beneath the mouse cursor. When I release the mouse button, this automatically selects the first item in the context menu (which happens to reload the tab).

This makes it really difficult to do other operations on that right-most tab in that context menu. The context menus for the other tabs that are further away from the right side of the screen seem to be unaffected.

This seems to be a recent regression.
Hey Neil,

Do you know of any recent changes to popup code that might be causing this?
Flags: needinfo?(enndeakin)
According to bgrins, this also seems to affect context menus in content.
Attached image reload-click.gif
I see what is probably the same thing when right clicking near the right side of any window.  It ends up auto-clicking the page reload button (see attached gif)
Regressed from 2015-08-11 to 2015-08-12
Bug 1075089 and bug 1186972 look like potential regressors in that range.
Yes this is caused by bug 1075089. It looks like context menus should be flipped horizontally on Mac when there isn't enough room to display them, yet not flipped vertically. Currently, the reverse is true.

This should be changed at the line in nsMenuPopupFrame.cpp with the comment 'screen positioned popups can be flipped vertically but never horizontally'.

Windows - flipped vertically, not flipped horizontally (current behaviour on all platforms)
Mac - not flipped vertically, flipped horizontally
Linux - flipped in both directions
Flags: needinfo?(enndeakin)
Blocks: 1075089
Duplicate of this bug: 1194611
Hey Nicko,

Any chance you could take a look at this? See comment 6.
Flags: needinfo?(nicko.robson)
Hello, yes I'll take a look asap
Flags: needinfo?(nicko.robson)
[Tracking Requested - why for this release]: This is a regression that makes it hard to interact with the rightmost tab.
Any ETA? This is pretty annoying.
ETA is next day or so. The fix is easy and done but the number of mochitests which rely on the direction the context menu opens is large so that's what's taking the time.
That makes sense, thanks for the update!
Added an ifdef for OSX to flip only horizontally as suggested. Fixed large menu tests. Other tests seemed to be ok.
Attachment #8651378 - Flags: review?(enndeakin)
Comment on attachment 8651378 [details] [diff] [review]
0001-Bug-1194337-Context-menu-positioned-incorrectly-on-O.patch

>     // screen positioned popups can be flipped vertically but never horizontally
>+    #ifdef XP_MACOSX
>+    hFlip = FlipStyle_Outside;
>+    #else
>     vFlip = FlipStyle_Outside;
>+    #endif

No indenting on preprocessor lines.


>+    // would put it off screen so ...
>+    if (platformIsMac()) {
>+      // On OSX the popup is constrained so it remains within the 
>+      // bounds of the screen
>+      ok(Math.round(rect.top) + gScreenY >= screen.top, gTests[gTestIndex] + " top");
>+      ok(Math.round(rect.bottom) + gScreenY <= screen.height, gTests[gTestIndex] + " bottom");

I think the bottom check should test using == as it should appear right at the bottom edge of the screen. You probably want to use screen.availTop + screen.availHeight to get this position offset by the context menu offset.


>+      // On windows the menu should be flipped to have its bottom

On other platforms...


>+      if (platformIsMac()) {
>+        let screenY;
>+        [, screenY] = getScreenXY(popup);
>+        // Macs constrain their popup menus vertically rather than flip them.
>+        // 10px error margin here due to different versions of OSX using different margins
>+        // between the bottom of the menu and the bottom of the screen.
>+        ok(screenY - (screen.height - rect.height) < 10, gTests[gTestIndex] + " top");

I'm not sure I understand this comparison. Maybe you meant to use screen.availHeight here too (which excludes the space taken up by the menubar/dock/etc)

Otherwise, this looks good! But I'd like to see the test fixed up. It would also be nice to have a test for the horizontal mac behaviour but we can do that later.
Attachment #8651378 - Flags: review?(enndeakin) → review-
Thanks. I've cleaned up the various points and you were right about the screen space - it was my dock that was throwing out the measurements.
Attachment #8651378 - Attachment is obsolete: true
V3 of patch. I've included a mochitest for the horizontally flipping popup on OSX
Attachment #8652624 - Attachment is obsolete: true
Attachment #8653139 - Flags: review?(enndeakin)
Comment on attachment 8653139 [details] [diff] [review]
0001-Bug-1194337-Context-menu-positioned-incorrectly-V3.patch

OK! Do you need to test it out on the try server?
Attachment #8653139 - Flags: review?(enndeakin) → review+
Yes please, Neil. When you have a minute
OK, I will later tonight. Now that you have a couple of patches done, you can also file a bug to get access (level 1) yourself. See https://www.mozilla.org/en-US/about/governance/policies/commit/  for instructions and I can provide the needed voucher.
Looks like there are still a couple of failures in that test as well as test_bug624329.xul They shouldn't do too hard to fix up; the largemenu test can be quite finicky.

I'll be away for the next week so let someone know if you need to test patches on try again.
Duplicate of this bug: 1194186
Attached V4 of patch, adjusted failing tests to make them more adaptable to the testing server configurations. 

Is anyone able to run this on a try server for me please?
Attachment #8653139 - Attachment is obsolete: true
Comment on attachment 8654590 [details] [diff] [review]
0001-Bug-1194337-Context-menu-positioned-incorrectly-V4.patch

Review of attachment 8654590 [details] [diff] [review]:
-----------------------------------------------------------------

(not a thorough review, but I noticed this, and pushed to try for you:

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d39c582e329


Apologies for the delay - I was on vacation, not sure about Neil / others.)

::: toolkit/content/tests/chrome/window_largemenu.xul
@@ +119,5 @@
>    else if (gTestIndex == 1) {
>      // the popup was supposed to open 100 pixels from the bottom, but that
> +    // would put it off screen so ...
> +    if (platformIsMac()) {
> +      // On OSX the popup is constrained so it remains within the 

Nit: trailing whitespace
Fixed trailing whitespace
Attachment #8654590 - Attachment is obsolete: true
Hi Gijs, thanks for doing that. Any ideas about the jetpack-package errors? I've never seen them before but they don't seem to be expected. The didn't appear last time at least so I assume they were a random failure. Would it be possible to push it to Try again and run the marionette tests too please?
(In reply to nicko.robson from comment #27)
> Hi Gijs, thanks for doing that. Any ideas about the jetpack-package errors?
> I've never seen them before but they don't seem to be expected. The didn't
> appear last time at least so I assume they were a random failure. Would it
> be possible to push it to Try again and run the marionette tests too please?

The marionette tests ran, I think? They're marked Mn and Mn-e10s, and they're green.

I retriggered the jetpack things. I have no idea how your patch could cause issues with those, but there we are...
Hello again. I've tried re-running the jetpack mochitests a number of times on both OSX and Ubuntu and never managed to recreate the failures shown in your Try run before. I've had random other ones which look related appear (test-content-event.js) but never the same ones twice. Is it possible this is just a transient, unrelated build issue?
(In reply to nicko.robson from comment #29)
> Hello again. I've tried re-running the jetpack mochitests a number of times
> on both OSX and Ubuntu and never managed to recreate the failures shown in
> your Try run before. I've had random other ones which look related appear
> (test-content-event.js) but never the same ones twice. Is it possible this
> is just a transient, unrelated build issue?

Hi, sorry for not following up immediately. The retriggers of the jetpack tests were green, so I think this is landable, assuming I'm not missing anything important. I'd update the patch carrying over r=enn, and set the checkin-needed keyword. Thanks for chasing this!
Flags: needinfo?(nicko.robson)
Added r=enn to comment.
Attachment #8657897 - Attachment is obsolete: true
Flags: needinfo?(nicko.robson)
Sorry to bother you again but I'm unable to set keywords on this bug (though it's been fine on other bugs). Can you please add the "checkin-needed" keyword?
Flags: needinfo?(gijskruitbosch+bugs)
No worries. I just landed this for you:

remote:   https://hg.mozilla.org/integration/fx-team/rev/19081055c2c8
Assignee: nobody → nicko.robson
Status: NEW → ASSIGNED
Flags: needinfo?(gijskruitbosch+bugs)
Duplicate of this bug: 1203662
https://hg.mozilla.org/mozilla-central/rev/19081055c2c8
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Component: XP Toolkit/Widgets: Menus → XUL
You need to log in before you can comment on or make changes to this bug.