350018 - nsIFrame code returns coordinates with (0, 0) in bottom-left on mac

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Håkan Waara]

In the Cocoa toolkit on mac, the origo point is based in the bottom-left in all kind of drawing and positioning.

Now, when you interact with Mozilla code, you're used to do the conversion from top-left to bottom-most coordinate systems.

What surprised me, when I sat debugging just now, is that a few core Gecko APIs inside cross-platform code, actually return cocoa rects on mac.

I'm not sure if this is a bug or feature, but I'd think that the cross-platform code should behave the same on all platforms in this sense.

For example:

nsIFrame::GetScreenRect()
nsIFrame::GetScreenRectExternal()
(I don't know if there are more)

If this is expected behavior, I think a comment is at least warranted. Other XP-code (like nsAccessibility, where this is used) depend on it.

Comment 1

[Håkan Waara]

BTW, the reason they return cocoa rects on mac, is because those APIs query the nsIWidget, which queries cocoa widgets, which will do its thing.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> I'm not sure if this is a bug or feature

Bug, imo.

> is because those APIs query the nsIWidget

Which should return Gecko-coordinate rects.  If it doesn't, it's buggy.

That said, I think the basic issue is just that http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/widget/src/cocoa/nsChildView.mm&rev=1.166#1766 flips the rect 3 times instead of twice.  It does ConvertGeckoToFlippedCocoaRect, then flips the cocoa rect, then does ConvertFlippedCocoaToGeckoRect.  Looks like a regression introduced by bug 325336.  That middle chunk of code can be removed, I think.

Comment 3

[Håkan Waara]

ChildView is a bit schizophrenic, because it passes anyone who asks it cocoa coordinates, but it returns YES for |-(BOOL)isFlipped| as if it would give you a topmost coordinate.  This is causing me problems with 3rd party tools that think they have to adjust the coordinates they get because the view is flipped.

Comment 4

[Håkan Waara]

Attachment: Initial proposed patch

Here's the bulk of a proposed fix. I'm not sure it applies because it's hand-edited. Here's what it does:

* Create a new helper FlipCocoaScreenCoordinate() that will convert a bottomleft screen point into a topleft screen point. We can use this in a bunch of places.

* Rename some of the helpers to clarify their meaning. Right now, they do no conversion, only casting, so the name could be better. Specifically:
  o ConvertGeckoToFlippedCocoaRect -> GeckoRectToNSRect()
  o ConvertFlippedCocoaToGeckoRect -> NSRectToGeckoRect()

* In WidgetToScreen(), I remove one conversion step, so the return coords are topleft ("flipped cocoa coordinates").

It seems to work OK so far, but the initial window is a bit smaller than usual, and context menus show up at the wrong place.

Comment 5

[Håkan Waara]

To sum up a bit, all the code really changes is to remove one conversion from a topleft rect to a bottomleft rect in nsChildView::ScreenToWidget.

Josh/Mento: Do you know which other code that depends on this call returning a cocoa rect?

Comment 6

[Håkan Waara]

Attachment: Cleanup patch (checked in)

To simplify, here's a simple cleanup patch. It does the two first points of comment 4 (rename, and create a new helper to share some code).

The method is still returning coordinates like before, so no "real" change has been made.

Comment 7

[Håkan Waara]

Comment on attachment 236067 [details] [diff] [review]
Cleanup patch (checked in)

See previous comment for review aid.

Comment 8

[Josh Aas]

Comment on attachment 236067 [details] [diff] [review]
Cleanup patch (checked in)

+  if ([[NSScreen screens] count] > 0)   // paranoia
+  {

Put the brace on the same line as the test

Comment 9

[Håkan Waara]

Comment on attachment 236067 [details] [diff] [review]
Cleanup patch (checked in)

Checked in with review comment addressed.

Comment 10

[Håkan Waara]

Comment on attachment 235395 [details] [diff] [review]
Initial proposed patch

This first patch did the right thing, but regresses context menu positioning, which assumes that we return cocoa coordinates.

Comment 11

[Håkan Waara]

I think I finally figured this bug out. 

Basically, what's fooling us, is that when we're using NSView's convertRect:toView:nil to convert our rect to a window-coord rect, it's not accounting for us being a flipped (top-left coord) view.

I'll explain it with a simple code example.

// let's convert a rect with origin (0, 0) and any |width| and 
// |height| to window coordinates.

NSRect convertedRect = 
  [ourView convertRect:NSMakeRect(0, 0, width, height) toView:nil];

The (0, 0) origin of our original rect is assumed to be in bottom-left coordinates, whilst we meant (0, 0) in *top-left* coordinates.

After this, we turn the rect's window-point into a screen-point (which is also in bottom-left coords).

We now have a rect in bottom-left screen coords. So we convert the rect's origin into top-left coords.

All fine and dandy, however, it doesn't work if the rect has a height! Because the top-left origin is still based on the fact that our localRect -> windowRect conversion in the beginning thought that (0, 0) was in bottom-left coordinates.

So, in order for the rect's top-left corner to be (0, 0), we have to subtract the rect's height.

This last step is what we've omitted before. And usually WidgetToScreen() is used for converting rects with a width, height of 1,1 so we haven't noticed it -- until nsFrame::GetScreenRect() tries to convert real rectangles with a size.

Sorry for the long explanation, this is not easy to explain without a whiteboard!

Comment 12

[Håkan Waara]

Attachment: Proposed patch

Patch following the explanation above. Context-menus appear at the right places, and nsFrame::GetScreenRect() works as expected!

Comment 13

[Håkan Waara]

Attachment: Better proposed patch

I changed a var name in the last minute; this actually compiles.

Comment 14

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 239493 [details] [diff] [review]
Better proposed patch

sr=pink

Comment 15

[Håkan Waara]

Attachment: Patch to accessible/ code that is needed now that this is fixed

Now that this bug is fixed, I need to revert a hack in my code.

Surkov, Cocoa uses the bottom-left as it's origo (0, 0) (and Y-value increases when you go up), so in order to return the correct position, I need to subtract the rectangle's height.

Comment 16

[alexander :surkov (:asurkov)]

Comment on attachment 239517 [details] [diff] [review]
Patch to accessible/ code that is needed now that this is fixed

>+    // Check for "role skew"; the role constants in nsIAccessible.idl need to match the ones
>+    // in nsRoleMap.h.
>+    NS_ASSERTION([AXRoles[nsIAccessible::ROLE_LAST_ENTRY] isEqualToString:@"ROLE_LAST_ENTRY"], "Role skew in the role map!");
>   }

nit: I'm not sure but probably you should say: "OS X role skew .." like it does msaa/atk code.

>-  // convert from cocoa's coordinate system to gecko's.
>+  // convert from cocoa's coordinate system to gecko's. according to the docs
>+  // the point we're given is guaranteed to be bottom-left screen coordinates.

nit: should be 'The point ...'

>+  // The coords we get from Gecko are top-left screen coordinates.
>+  // Cocoa wants us to return bottom-left screen coordinates.
>+  // this involves two steps:

nit: Should be 'This invloves ...'.

The code looks good.

Comment 17

[Håkan Waara]

Should be all fixed now. Patches checked in to trunk.

Comment 18

[Sylvain Pasche]

For information, I came across a coordinate management related issue. See bug 370106.

Comment 19

[jhp (no longer active)]

Attachment: 1.8.1 branch patch

Just ran into this bug when using XULRunner 1.8.1.x (need to use Cocoa widgets in order to embed XR in Java/SWT).  This patch to the 1.8.1 branch just takes the functional change (no cleanup).

Comment 20

[Stuart Morgan]

I think this messed Camino when it landed on trunk; let me dig that up before reviewing this so 1.1 doesn't get screwed.

Comment 21

[Stuart Morgan]

Comment on attachment 256847 [details] [diff] [review]
1.8.1 branch patch

Found it (bug 353964). r=me

Comment 22

[jhp (no longer active)]

Comment on attachment 256847 [details] [diff] [review]
1.8.1 branch patch

Asking for 1.8.1.3 approval.  This only affects Cocoa widgets, which aren't built by default on the 1.8 branch.  The Camino related change will be taken care of later, in bug 353964.

Comment 24

[Stuart Morgan]

This turns out to be the cause of a problem rendering Picasa web albums, which is a fairly high-profile compatibility problem. As a result, we (Camino) would very much like to see this on the branch.

Comment 26

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 256847 [details] [diff] [review]
1.8.1 branch patch

We (Camino) would really like to get this patch on the 1.8.0 branch as well to fix the same major compat issue for our 10.2 users (the 1.8.0 branch is the last place Camino supports 10.2).

Comment 28

[Daniel Veditz [:dveditz]]

Comment on attachment 256847 [details] [diff] [review]
1.8.1 branch patch

approved for 1.8.0.12/1.8.1.4, a=dveditz for release-drivers

Comment 29

[froodian (Ian Leue)]

Checked in on MOZILLA_1_8_BRANCH and MOZILLA_1_8_0_BRANCH.  (also clearing the blocking1.9? flag, since this landed on trunk long ago)

/cvsroot/mozilla/widget/src/cocoa/nsChildView.mm,v  <--  nsChildView.mm
new revision: 1.120.2.15; previous revision: 1.120.2.14
/cvsroot/mozilla/widget/src/cocoa/nsChildView.mm,v  <--  nsChildView.mm
new revision: 1.120.2.13.2.1; previous revision: 1.120.2.13