The default bug view has changed. See this FAQ.

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

RESOLVED FIXED

Status

()

Core
Widget: Cocoa
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: Håkan Waara, Assigned: Håkan Waara)

Tracking

({fixed1.8.0.12, fixed1.8.1.4})

Trunk
PowerPC
Mac OS X
fixed1.8.0.12, fixed1.8.1.4
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Assignee)

Description

11 years ago
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.
(Assignee)

Comment 1

11 years ago
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.
> 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.
Assignee: nobody → joshmoz
Component: Layout → Widget: Cocoa
Flags: blocking1.9?
QA Contact: layout → cocoa
Blocks: 325336
(Assignee)

Comment 3

11 years ago
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.
(Assignee)

Comment 4

11 years ago
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.
(Assignee)

Comment 5

11 years ago
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?
(Assignee)

Comment 6

11 years ago
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.
Attachment #236067 - Flags: review?(joshmoz)
(Assignee)

Comment 7

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

See previous comment for review aid.
Attachment #236067 - Flags: review?(joshmoz) → review?(mark)

Comment 8

11 years ago
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
Attachment #236067 - Flags: review?(mark) → review+
(Assignee)

Comment 9

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

Checked in with review comment addressed.
Attachment #236067 - Attachment description: Cleanup patch (no behavior change) → Cleanup patch (checked in)
(Assignee)

Comment 10

11 years ago
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.
Attachment #235395 - Attachment is obsolete: true
(Assignee)

Comment 11

11 years ago
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!
(Assignee)

Comment 12

11 years ago
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!
Attachment #239487 - Flags: review?(joshmoz)
(Assignee)

Updated

11 years ago
Blocks: 352329
(Assignee)

Comment 13

11 years ago
Created attachment 239493 [details] [diff] [review]
Better proposed patch

I changed a var name in the last minute; this actually compiles.
Attachment #239487 - Attachment is obsolete: true
Attachment #239493 - Flags: review?(joshmoz)
Attachment #239487 - Flags: review?(joshmoz)

Updated

11 years ago
Attachment #239493 - Flags: review?(joshmoz) → review+

Updated

11 years ago
Attachment #239493 - Flags: superreview?(mikepinkerton)
Comment on attachment 239493 [details] [diff] [review]
Better proposed patch

sr=pink
Attachment #239493 - Flags: superreview?(mikepinkerton) → superreview+
(Assignee)

Updated

11 years ago
Assignee: joshmoz → hwaara
(Assignee)

Comment 15

11 years ago
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.
Attachment #239517 - Flags: review?(surkov.alexander)

Comment 16

11 years ago
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.
Attachment #239517 - Flags: review?(surkov.alexander) → review+
(Assignee)

Comment 17

11 years ago
Should be all fixed now. Patches checked in to trunk.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 18

10 years ago
For information, I came across a coordinate management related issue. See bug 370106.
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).
Attachment #256847 - Flags: review?(joshmoz)

Updated

10 years ago
Attachment #256847 - Flags: review?(joshmoz) → review?(stuart.morgan)

Comment 20

10 years ago
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

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

Found it (bug 353964). r=me
Attachment #256847 - Flags: review?(stuart.morgan) → review+
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.
Attachment #256847 - Flags: approval1.8.1.3?

Updated

10 years ago
Duplicate of this bug: 369998

Comment 24

10 years ago
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.

Updated

10 years ago
Duplicate of this bug: 374305
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).
Attachment #256847 - Flags: approval1.8.0.12?

Updated

10 years ago
Duplicate of this bug: 375422
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
Attachment #256847 - Flags: approval1.8.1.4?
Attachment #256847 - Flags: approval1.8.1.4+
Attachment #256847 - Flags: approval1.8.0.12?
Attachment #256847 - Flags: approval1.8.0.12+

Comment 29

10 years ago
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
Flags: blocking1.9?
Keywords: fixed1.8.0.12, fixed1.8.1.4
You need to log in before you can comment on or make changes to this bug.