76297 - need some easy way to get the screen coordinates of a xul element

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Joe Hewitt (gone)]

I really need a way to get the screen coordinates of a xul element.  The reason
I need it is because I just discovered that when you capture a mouse event
inside of a popup, the event.clientX/Y properties will actually like and give
you screen coordinates instead of client coordinates.  I need to turn them back
into clinet coordinates, and I need to be able to say something like -
box.boxObject.screenY

I thought there would be an easy way to do this, but I can't find one.  All I
could find was nsIWidget::WidgetToScreen, but since not all xul elements have a
widget this is not very useful.  Fortunately, this is useful for me, as I only
need to know the coordinates of a popup for this application.  I hacked screenX
and screenY into nsIBoxObject in my tree, but maybe that's not the best way to
do this?

Comment 1

[Peter Trudelle]

->hyatt

Comment 2

[Joe Hewitt (gone)]

Attachment: patch to fix

Comment 3

[Joe Hewitt (gone)]

taking

Comment 4

[Brendan Eich [:brendan]]

The patch leaks all over:

+    rv = frame->GetView(presContext, &view);
+    if (view) {
+      rv = view->GetWidget(widget);
+      if (widget)
+        break;
+    }

Use an nsCOMPtr, or NS_RELEASE after calling view->GetWidget and before the if
(widget) break; -- but really, use nsCOMPtr.

For widget too, which is leaked if found.  The control flow here:

+  if (widget) {
+    // add the widget's screen coordinates to the offset we've counted
+    nsRect oldBox(0,0,0,0);
+    widget->WidgetToScreen(oldBox, aRect);
+    aRect.x += offsetX;
+    aRect.y += offsetY;
+  } else
+    return NS_ERROR_FAILURE;
+
+  return NS_OK;

is capricious: why put the return NS_ERROR_FAILURE in the else clause, but the
final return not in the then clause?  Of course, then you'd have an instance of
the else-after-return non-sequitur.  Best to cast out failure cases with early
returns:

+  if (!widget)
+    return NS_ERROR_FAILURE;
+
+  // add the widget's screen coordinates to the offset we've counted
+  nsRect oldBox(0,0,0,0);
+  widget->WidgetToScreen(oldBox, aRect);
+  aRect.x += offsetX;
+  aRect.y += offsetY;
+  return NS_OK;

supposing, again, that widget is nsCOMPtr<nsIWidget>.

Same goes for doc, except that the use of a reference to a pointer in
nsIContent::GetDocument requires special handling:

+  nsIDocument* doc;
+  mContent->GetDocument(doc);

should be

+  nsCOMPtr<nsIDocument> doc;
+  mContent->GetDocument(*getter_AddRefs(doc));

/be

Comment 5

[Joe Hewitt (gone)]

Attachment: patch, take 2

Comment 6

[Brendan Eich [:brendan]]

Still leaking the view if found:

+    nsIView* view;
+    rv = frame->GetView(presContext, &view);
+    if (view) {
+      rv = view->GetWidget(*getter_AddRefs(widget));
+      if (widget)
+        break;
+    }

That was my first comment ("but really, use nsCOMPtr").

Can the content have lost its document, via DOM manipulations, yet still have a
script ask for its screenX or Y?  If so, you should null-check doc right after
getting it.

/be

Comment 7

[Joe Hewitt (gone)]

I am confused by your suggestion to use nsCOMPtr on nsIView, because it seems
that views are not ref counted... or at least, that's what the compiler told me
when I tried to do an nsCOMPtr<nsIView>.  Am I missing something here?

Comment 8

[Brendan Eich [:brendan]]

Sorry, I'm wrong: views, like frames, are not ref-counted.  Evil, but I forgot
views shared that badness with frames.

Is the doc null check necessary?  Waterson would know (hyatt too, I bet).

/be

Comment 9

[hangas]

Themes Triage Team Marking nsbeta1+

Comment 10

[Joe Hewitt (gone)]

Judging by the implementation of GetOffsetRect, right above GetScreenRect, I'd
say I need to be null checkin doc and presShell.  I modified the patch so that
it looks similar to GetOffsetRect.

Comment 11

[Joe Hewitt (gone)]

Attachment: patch, take 3

Comment 12

[David Hyatt]

[s]r=hyatt

Comment 13

[Brendan Eich [:brendan]]

r/sr=[e-mail].

/be

Comment 14

[Joe Hewitt (gone)]

fixed