Closed
Bug 1194337
Opened 9 years ago
Closed 9 years ago
Context menus opened close to the right side of the screen put first item under mouse cursor
Categories
(Core :: XUL, defect)
Core
XUL
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)
73.45 KB,
image/gif
|
Details | |
9.20 KB,
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•9 years ago
|
||
Hey Neil, Do you know of any recent changes to popup code that might be causing this?
Flags: needinfo?(enndeakin)
Reporter | ||
Comment 2•9 years ago
|
||
According to bgrins, this also seems to affect context menus in content.
Comment 3•9 years ago
|
||
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)
Comment 4•9 years ago
|
||
Regressed from 2015-08-11 to 2015-08-12
Reporter | ||
Comment 5•9 years ago
|
||
Bug 1075089 and bug 1186972 look like potential regressors in that range.
Comment 6•9 years ago
|
||
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)
Reporter | ||
Comment 8•9 years ago
|
||
Hey Nicko, Any chance you could take a look at this? See comment 6.
Flags: needinfo?(nicko.robson)
Reporter | ||
Updated•9 years ago
|
Keywords: regressionwindow-wanted
Reporter | ||
Updated•9 years ago
|
status-firefox43:
--- → affected
Comment 10•9 years ago
|
||
[Tracking Requested - why for this release]: This is a regression that makes it hard to interact with the rightmost tab.
tracking-firefox43:
--- → ?
Comment 11•9 years ago
|
||
Any ETA? This is pretty annoying.
Assignee | ||
Comment 12•9 years ago
|
||
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.
Comment 13•9 years ago
|
||
That makes sense, thanks for the update!
Assignee | ||
Comment 14•9 years ago
|
||
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)
Updated•9 years ago
|
Updated•9 years ago
|
status-firefox42:
--- → unaffected
Comment 15•9 years ago
|
||
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-
Assignee | ||
Comment 16•9 years ago
|
||
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
Assignee | ||
Comment 17•9 years ago
|
||
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 18•9 years ago
|
||
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+
Assignee | ||
Comment 19•9 years ago
|
||
Yes please, Neil. When you have a minute
Comment 20•9 years ago
|
||
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.
Comment 22•9 years ago
|
||
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.
Assignee | ||
Comment 24•9 years ago
|
||
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 25•9 years ago
|
||
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
Assignee | ||
Comment 26•9 years ago
|
||
Fixed trailing whitespace
Attachment #8654590 -
Attachment is obsolete: true
Assignee | ||
Comment 27•9 years ago
|
||
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?
Comment 28•9 years ago
|
||
(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...
Assignee | ||
Comment 29•9 years ago
|
||
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?
Comment 30•9 years ago
|
||
(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)
Assignee | ||
Comment 31•9 years ago
|
||
Added r=enn to comment.
Attachment #8657897 -
Attachment is obsolete: true
Flags: needinfo?(nicko.robson)
Assignee | ||
Comment 32•9 years ago
|
||
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)
Comment 33•9 years ago
|
||
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)
Comment 35•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/19081055c2c8
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Updated•5 years ago
|
Component: XP Toolkit/Widgets: Menus → XUL
You need to log in
before you can comment on or make changes to this bug.
Description
•