Last Comment Bug 350018 - nsIFrame code returns coordinates with (0, 0) in bottom-left on mac
: nsIFrame code returns coordinates with (0, 0) in bottom-left on mac
: fixed1.8.0.12, fixed1.8.1.4
Product: Core
Classification: Components
Component: Widget: Cocoa (show other bugs)
: Trunk
: PowerPC Mac OS X
: -- normal (vote)
: ---
Assigned To: Håkan Waara
: Markus Stange [:mstange]
: 369998 374305 375422 (view as bug list)
Depends on:
Blocks: 325336 352329
  Show dependency treegraph
Reported: 2006-08-24 06:38 PDT by Håkan Waara
Modified: 2007-03-27 16:32 PDT (History)
12 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Initial proposed patch (13.83 KB, patch)
2006-08-25 07:25 PDT, Håkan Waara
no flags Details | Diff | Splinter Review
Cleanup patch (checked in) (14.11 KB, patch)
2006-08-30 05:05 PDT, Håkan Waara
jaas: review+
Details | Diff | Splinter Review
Proposed patch (2.53 KB, patch)
2006-09-21 03:07 PDT, Håkan Waara
no flags Details | Diff | Splinter Review
Better proposed patch (2.54 KB, patch)
2006-09-21 04:18 PDT, Håkan Waara
jaas: review+
mikepinkerton: superreview+
Details | Diff | Splinter Review
Patch to accessible/ code that is needed now that this is fixed (2.21 KB, patch)
2006-09-21 09:50 PDT, Håkan Waara
surkov.alexander: review+
Details | Diff | Splinter Review
1.8.1 branch patch (1.07 KB, patch)
2007-02-28 15:31 PST, jhp (no longer active)
stuart.morgan+bugzilla: review+
dveditz: approval1.8.1.4+
dveditz: approval1.8.0.12+
Details | Diff | Splinter Review

Description Håkan Waara 2006-08-24 06:38:03 PDT
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:

(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 2006-08-24 06:39:24 PDT
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 [:bz] (still a bit busy) 2006-08-24 10:14:57 PDT
> 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 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 2006-08-25 06:19:14 PDT
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 2006-08-25 07:25:15 PDT
Created attachment 235395 [details] [diff] [review]
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 2006-08-25 08:53:56 PDT
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 2006-08-30 05:05:17 PDT
Created attachment 236067 [details] [diff] [review]
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 2006-09-05 08:22:18 PDT
Comment on attachment 236067 [details] [diff] [review]
Cleanup patch (checked in)

See previous comment for review aid.
Comment 8 Josh Aas 2006-09-07 11:33:15 PDT
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 2006-09-07 12:05:49 PDT
Comment on attachment 236067 [details] [diff] [review]
Cleanup patch (checked in)

Checked in with review comment addressed.
Comment 10 Håkan Waara 2006-09-07 12:07:18 PDT
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 2006-09-21 02:56:46 PDT
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 2006-09-21 03:07:20 PDT
Created attachment 239487 [details] [diff] [review]
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 2006-09-21 04:18:59 PDT
Created attachment 239493 [details] [diff] [review]
Better proposed patch

I changed a var name in the last minute; this actually compiles.
Comment 14 Mike Pinkerton (not reading bugmail) 2006-09-21 07:21:08 PDT
Comment on attachment 239493 [details] [diff] [review]
Better proposed patch

Comment 15 Håkan Waara 2006-09-21 09:50:42 PDT
Created attachment 239517 [details] [diff] [review]
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 2006-09-22 03:59:19 PDT
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 2006-09-22 04:06:39 PDT
Should be all fixed now. Patches checked in to trunk.
Comment 18 Sylvain Pasche 2007-02-14 16:17:47 PST
For information, I came across a coordinate management related issue. See bug 370106.
Comment 19 jhp (no longer active) 2007-02-28 15:31:17 PST
Created attachment 256847 [details] [diff] [review]
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 2007-02-28 21:12:00 PST
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 2007-02-28 21:19:19 PST
Comment on attachment 256847 [details] [diff] [review]
1.8.1 branch patch

Found it (bug 353964). r=me
Comment 22 jhp (no longer active) 2007-03-01 07:57:20 PST
Comment on attachment 256847 [details] [diff] [review]
1.8.1 branch patch

Asking for 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 23 Stuart Morgan 2007-03-09 20:57:29 PST
*** Bug 369998 has been marked as a duplicate of this bug. ***
Comment 24 Stuart Morgan 2007-03-09 20:59:54 PST
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 25 Chris Lawson (gone) 2007-03-17 08:40:25 PDT
*** Bug 374305 has been marked as a duplicate of this bug. ***
Comment 26 Smokey Ardisson (offline for a while; not following bugs - do not email) 2007-03-21 13:59:41 PDT
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 27 froodian (Ian Leue) 2007-03-26 10:10:48 PDT
*** Bug 375422 has been marked as a duplicate of this bug. ***
Comment 28 Daniel Veditz [:dveditz] 2007-03-27 15:57:28 PDT
Comment on attachment 256847 [details] [diff] [review]
1.8.1 branch patch

approved for, a=dveditz for release-drivers
Comment 29 froodian (Ian Leue) 2007-03-27 16:32:20 PDT
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/,v  <--
new revision:; previous revision:
/cvsroot/mozilla/widget/src/cocoa/,v  <--
new revision:; previous revision:

Note You need to log in before you can comment on or make changes to this bug.