Closed Bug 1240533 Opened 4 years ago Closed 4 years ago

Context menu sometimes appears on secondary screen

Categories

(Core :: Widget, defect)

46 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: rfeeley, Assigned: jfkthame)

References

()

Details

(Keywords: regression)

Attachments

(2 files)

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.
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)
Cocoa probably. The place to start would be nsScreenManagerCocoa::ScreenForRect.
Flags: needinfo?(enndeakin)
Appears to also be doing it with text input suggestions.
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)
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)
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)
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)
I've never seen this bug before and have been using Nightly for over a year.
Flags: needinfo?(rfeeley)
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!)
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)
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)
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)
Duplicate of this bug: 1240749
This is reproducible on Linux as well.
Component: Widget: Cocoa → Widget
OS: Mac OS X → All
Hardware: x86 → All
Duplicate of this bug: 1246815
Keywords: regression
Version: unspecified → 46 Branch
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)
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.
Thanks. I tested that build and the bug does not appear. Windows 7 x64 120 dpi, two monitors horizontal, firefox on left monitor.
OS X build resolves the issue for me. Great work Mike!
Flags: needinfo?(rfeeley)
This is 100% jfkthame work here, but I'll happily take the free credit. ;)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
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?
I've set scaling to 1,62 for the first monitor (2880x1620) and 1 for the second monitor (1920x1200).
(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...
(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.
Attachment #8719021 - Flags: review?(VYV03354) → review+
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
https://hg.mozilla.org/mozilla-central/rev/bf6afee1a737
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
This is still not fixed for me, should I reopen this or reopen bug 1240749?
Flags: needinfo?(jfkthame)
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)
Duplicate of this bug: 1244129
rfeeley - I think you said you were still seeing this on Nightly?
Flags: needinfo?(rfeeley)
I restarted and am now on 47.0a1 (2016-02-22) and it's not doing it anymore!
Flags: needinfo?(rfeeley)
Outstanding. Thanks rfeeley.
Status: RESOLVED → VERIFIED
Duplicate of this bug: 1244957
You need to log in before you can comment on or make changes to this bug.