Closed
Bug 1240533
Opened 9 years ago
Closed 9 years ago
Context menu sometimes appears on secondary screen
Categories
(Core :: Widget, defect)
Tracking
()
VERIFIED
FIXED
mozilla47
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
People
(Reporter: rfeeley, Assigned: jfkthame)
References
()
Details
(Keywords: regression)
Attachments
(2 files)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
emk
:
review+
|
Details | Diff | Splinter Review |
MacBook Pro (Retina, 15-inch, Mid 2015)
OS X 10.11.2 (15C50)
I have two HiDPI screens vertical aligned (primary external above, laptop below).
On any website, when I right-click in the bottom-right quadrant of the browser window, the context menu appears on the bottom screen.
Comment 1•9 years ago
|
||
Hey Enn - do you think Widget:Cocoa is the right component here, or is this possibly something going wrong in our popup code?
Flags: needinfo?(enndeakin)
Comment 2•9 years ago
|
||
Cocoa probably. The place to start would be nsScreenManagerCocoa::ScreenForRect.
Flags: needinfo?(enndeakin)
Reporter | ||
Comment 3•9 years ago
|
||
Appears to also be doing it with text input suggestions.
Comment 4•9 years ago
|
||
Hey spohl, is there a process for getting this triaged by the macdev folk? Or are we done here, and you'll get to it when you get to it?
Flags: needinfo?(spohl.mozilla.bugs)
Comment 5•9 years ago
|
||
As the module owner of widget/cocoa[1], Markus may be best suited to answer your question here. Personally, I've been tasked to work on things outside of cocoa and I haven't been able to triage much myself lately (unfortunately).
[1] https://wiki.mozilla.org/Modules/All#Widget_-_OS_X
Flags: needinfo?(spohl.mozilla.bugs) → needinfo?(mstange)
Comment 6•9 years ago
|
||
Well, the bug is in the right component. Getting it fixed is another matter...
Jonathan, do you want to take a look at this one?
Flags: needinfo?(mstange) → needinfo?(jfkthame)
Assignee | ||
Comment 7•9 years ago
|
||
Ryan, could you test some older releases to see whether this issue has existed ever since we initially landed hidpi support (in bug 674373 and followups, beginning at Firefox 18), or was there a time when it used to work and then we've (re-)broken it?
Flags: needinfo?(jfkthame) → needinfo?(rfeeley)
Reporter | ||
Comment 8•9 years ago
|
||
I've never seen this bug before and have been using Nightly for over a year.
Flags: needinfo?(rfeeley)
Assignee | ||
Comment 9•9 years ago
|
||
Ah, so it's likely to be a regression from bug 890156, then. (That bug merged to m-c for the 2016-01-14 nightly, I believe; if you could confirm that's when it regressed, that would be helpful - thanks!)
Comment 10•9 years ago
|
||
Hey rfeeley - here are builds to try:
Jan 13th Nightly: https://ftp.mozilla.org/pub/firefox/nightly/2016/01/2016-01-13-13-59-47-mozilla-central/firefox-46.0a1.en-US.mac.dmg
Jan 14th Nightly: https://ftp.mozilla.org/pub/firefox/nightly/2016/01/2016-01-14-03-02-46-mozilla-central/firefox-46.0a1.en-US.mac.dmg
If the bug is in the 14th Nightly, and _not_ in the 13th, then that's pretty compelling evidence that bug 890156 is the culprit.
Flags: needinfo?(rfeeley)
Reporter | ||
Comment 11•9 years ago
|
||
Your suspicions are correct! The issue does not appear in Jan 13th Nightly but does appear in Jan 14th Nightly.
Flags: needinfo?(rfeeley) → needinfo?(mconley)
Comment 12•9 years ago
|
||
Outstanding - thank you! I'm going to mark this blocking bug 890156 for now. jfkthame - is there any other information you need from rfeeley?
Blocks: 890156
Flags: needinfo?(mconley) → needinfo?(jfkthame)
Comment 14•9 years ago
|
||
This is reproducible on Linux as well.
Component: Widget: Cocoa → Widget
OS: Mac OS X → All
Hardware: x86 → All
Keywords: regression
Version: unspecified → 46 Branch
Assignee | ||
Comment 16•9 years ago
|
||
I couldn't seem to reproduce this exact scenario, but I've definitely seen similar cases where a popup appears on the wrong screen.
Could you please test with the try build from https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e49e0b8585f (when it's ready), and let me know if that helps? Thanks.
Flags: needinfo?(jfkthame) → needinfo?(rfeeley)
Comment 17•9 years ago
|
||
What about on Windows? See bug #1246815 which was marked as a dupe. How can I download that build? I went to the URL and I don't see a way to download.
Comment 18•9 years ago
|
||
OS X build is here: http://archive.mozilla.org/pub/firefox/try-builds/jkew@mozilla.com-2e49e0b8585f8f84131e3c8d1b9c0c7066ec6cc8/try-macosx64/firefox-47.0a1.en-US.mac.dmg
Windows build is here: http://archive.mozilla.org/pub/firefox/try-builds/jkew@mozilla.com-2e49e0b8585f8f84131e3c8d1b9c0c7066ec6cc8/try-win32/firefox-47.0a1.en-US.win32.zip
Comment 19•9 years ago
|
||
Thanks. I tested that build and the bug does not appear. Windows 7 x64 120 dpi, two monitors horizontal, firefox on left monitor.
Reporter | ||
Comment 20•9 years ago
|
||
OS X build resolves the issue for me. Great work Mike!
Flags: needinfo?(rfeeley)
Comment 21•9 years ago
|
||
This is 100% jfkthame work here, but I'll happily take the free credit. ;)
Assignee | ||
Comment 22•9 years ago
|
||
Attachment #8719021 -
Flags: review?(VYV03354)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Comment 23•9 years ago
|
||
Comment on attachment 8719021 [details] [diff] [review]
Parameters to ScreenForRect need to be passed as desktop pixels, not device pixels
Review of attachment 8719021 [details] [diff] [review]:
-----------------------------------------------------------------
::: layout/xul/nsMenuPopupFrame.cpp
@@ +1555,5 @@
> // anchor is located.
> + DesktopToLayoutDeviceScale scale =
> + PresContext()->DeviceContext()->GetDesktopToDeviceScale();
> + DesktopRect rect =
> + (mInContentShell ? aRootScreenRect : aAnchorRect) / scale;
Again, is this correct on mixed DPI monitors?
Comment 24•9 years ago
|
||
http://archive.mozilla.org/pub/firefox/try-builds/jkew@mozilla.com-2e49e0b8585f8f84131e3c8d1b9c0c7066ec6cc8/try-linux64/ this build doesn't fix the bug for me.
Comment 25•9 years ago
|
||
I've set scaling to 1,62 for the first monitor (2880x1620) and 1 for the second monitor (1920x1200).
Assignee | ||
Comment 26•9 years ago
|
||
(In reply to Masatoshi Kimura [:emk] from comment #23)
> Comment on attachment 8719021 [details] [diff] [review]
> Parameters to ScreenForRect need to be passed as desktop pixels, not device
> pixels
>
> Review of attachment 8719021 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: layout/xul/nsMenuPopupFrame.cpp
> @@ +1555,5 @@
> > // anchor is located.
> > + DesktopToLayoutDeviceScale scale =
> > + PresContext()->DeviceContext()->GetDesktopToDeviceScale();
> > + DesktopRect rect =
> > + (mInContentShell ? aRootScreenRect : aAnchorRect) / scale;
>
> Again, is this correct on mixed DPI monitors?
As far as I can tell, aRootScreenRect and aAnchorRect are always rects in the same device-pixel units, so it should be OK to apply the same presContext's scale to either of them.
(If there are ever circumstances where this isn't true, we could presumably handle it by passing the appropriate presContext to this method along with each of the rects, so that each of them can be scaled using the correct factor. But I don't currently believe this is necessary.)
(In reply to Marco Castelluccio [:marco] from comment #25)
> I've set scaling to 1,62 for the first monitor (2880x1620) and 1 for the
> second monitor (1920x1200).
I think there are problems with non-integer scale factors in the Linux widget backend, because somewhere in that code we try to "snap" the devicePixelRatio to an integer, but in other parts of the code we may not be aware of that. I'll try to get a mixed-resolution Linux system set up so as to look into this further...
Comment 27•9 years ago
|
||
(In reply to Jonathan Kew (:jfkthame) from comment #26)
> I think there are problems with non-integer scale factors in the Linux
> widget backend, because somewhere in that code we try to "snap" the
> devicePixelRatio to an integer, but in other parts of the code we may not be
> aware of that. I'll try to get a mixed-resolution Linux system set up so as
> to look into this further...
Note that this wasn't a problem before bug 890156.
Updated•9 years ago
|
Attachment #8719021 -
Flags: review?(VYV03354) → review+
Assignee | ||
Comment 28•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf6afee1a737301d1df572dc99a941a3008167b9
Bug 1240533 - Parameters to ScreenForRect need to be passed as desktop pixels, not device pixels. r=emk
Comment 29•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Comment 30•9 years ago
|
||
This is still not fixed for me, should I reopen this or reopen bug 1240749?
Flags: needinfo?(jfkthame)
Assignee | ||
Comment 31•9 years ago
|
||
Let's re-open bug 1240749; we should un-dupe it as it's a Linux-specific issue at this point -- thanks.
Flags: needinfo?(jfkthame)
Comment 33•9 years ago
|
||
rfeeley - I think you said you were still seeing this on Nightly?
Flags: needinfo?(rfeeley)
Reporter | ||
Comment 34•9 years ago
|
||
I restarted and am now on 47.0a1 (2016-02-22) and it's not doing it anymore!
Flags: needinfo?(rfeeley)
You need to log in
before you can comment on or make changes to this bug.
Description
•