392395 - Cocoa/Gecko rect (and point?) conversion appears to be wrong

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Stuart Morgan]

All the rect and point conversions in Cocoa Widget appear to assume that Cocoa's 0,0 is in the lower left of the bounding rect of all screens and that Gecko's is in the upper left of the bounding box of all screens. However, both are actually in their respective corners of the screen with the menu bar (which I will call the primary screen to differentiate it from NSScreen's |mainScreen|, which is not the same thing).

Comment 1

[Stuart Morgan]

Attachment: suggest logic

This is an untested patch for what I think is the correct logic.

However, there are several places where CocoaScreenCoordsHeight() is used directly to flip a point, which would be equally wrong (I suspect that CocoaScreenCoordsHeight won't be necessary given correct logic everywhere, but I haven't looked in detail at all it's uses). Those need to be audited and likely fixed--They should probably be folded into point-flipping utility functions in nsCocoaUtils to match the rect flipping, to make it easier to adjust this later if there are other issues.

Comment 3

[Stuart Morgan]

Attachment: fix

This is the complete version, changing all the uses of screen height. I've tested that this not only fixes the Camino regression in bug 384657, but gives Gecko screen coords that look correct on a variety of unusual monitor layouts.

I haven't tested Firefox, so someone should test the Firefox window pieces, but I'm now much more convinced that this is correct, so any issues it causes would be bugs in assumptions made by other code.

Comment 4

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 276855 [details] [diff] [review]
fix

> All the rect and point conversions in Cocoa Widget appear to assume
> that Cocoa's 0,0 is in the lower left of the bounding rect of all
> screens and that Gecko's is in the upper left of the bounding box of
> all screens.

Yes, this is correct wrt to my CocoaScreenCoordsHeight() in
nsCocoaUtils.mm.  (I didn't realize you were saying this when we
talked yesterday in #macdev.)  This is how I read Apple's very vague
documentation.

> However, both are actually in their respective corners of the screen
> with the menu bar (which I will call the primary screen to
> differentiate it from NSScreen's |mainScreen|, which is not the same
> thing).

I didn't know this ... but I'll defer to you.  I have zero experience
working with more than one monitor :-(

If you're right about this, then your patch looks fine to me.

But I suggest you get also get someone to review who has access to
multiple monitors and can test on Minefield.  I think Colin fits the
bill.

Comment 5

[Stuart Morgan]

Comment on attachment 276855 [details] [diff] [review]
fix

(In reply to comment #4)
> > However, both are actually in their respective corners of the screen
> > with the menu bar (which I will call the primary screen to
> > differentiate it from NSScreen's |mainScreen|, which is not the same
> > thing).
> 
> I didn't know this ... but I'll defer to you.

It's definitely true for Cocoa (they unfortunately use the term "main" in the description to mean the screen with the menu bar), and I was told in #gfx yesterday that that's how Gecko works.

Colin, can you take this for a spin in Minefield? You'll particularly want to test monitors where the tops and bottoms of the two are not vertically aligned.

Comment 6

[Colin Barrett [:cbarrett]]

That's perfect -- I have a multimonitor setup at work, should be pretty easy to set something like that up.

I'll get to this on Monday.

Comment 7

[Colin Barrett [:cbarrett]]

Comment on attachment 276855 [details] [diff] [review]
fix

I tested it on my multimonitor setup at work. I tried a number of different tasks, like opening multiple new windows on different screens, opening up popups from select elements, showing tooltips, dragging images.

Everything seemed to work fine.

Comment 8

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 276855 [details] [diff] [review]
fix

sr=pink

for testing, did you try the case where there are two monitors stacked on top of each other? I'd also make sure to try selects on both monitors if you haven't already.

Comment 9

[Colin Barrett [:cbarrett]]

I tried selects on both monitors already. I haven't tried the case where the monitors are stacked on top of each other.

Comment 10

[Colin Barrett [:cbarrett]]

Just tested both ways you could stack monitors on top of each other. Opened tooltips and selects, seemed to work fine.

Comment 11

[Stuart Morgan]

Landed.