306222 - Clean up nsIFrame::GetContentAndOffsetsFromPoint

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This function has many problems:

1) It's completely undocumented
2) It takes a point that is either in coordinates relative to the view returned
GetClosestView, or GetOffsetFromView --- it seems confused on that
3) It's not 100% clear what the return values mean

We should replace it with something that's actually specified.

For PeekOffset we'll be defining a triple (nsIContent*, offset,
associateWithPrevContent) to identify caret/selection positions (see bug
300131). It seems like we could reuse that here. Then we'd have something like
  nsCaretPosition GetCaretPositionFromPoint(const nsPoint& pt);
where the point is in frame coordinates. This would return where the caret
should jump to if the user clicks at that point in the given frame, *assuming
the point is not in a child frame*. So you might want to call GetFrameForPoint
before calling this. For content that automatically selects when clicked on, it
would return the prev-edge of the selection.

HOWEVER it might be useful to first fix GetContentAndOffsetsFromPoint to take
frame coordinates before redefining it completely this way.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

nsTextFrame::GetPosition(Slowly) needs similar treatment at the same time.

Comment 2

[Eli Friedman]

(In reply to comment #0)
> This would return where the caret
> should jump to if the user clicks at that point in the given frame, *assuming
> the point is not in a child frame*. So you might want to call GetFrameForPoint
> before calling this.

Fine, I guess.  I don't see how that helps, though; there still needs to be
handling for things like clicking between lines where you want content within
the lines.

> For content that automatically selects when clicked on, it
> would return the prev-edge of the selection.

Currently, I believe content that automatically selects when clicked on is
handled by code like that at
http://lxr.mozilla.org/seamonkey/source/layout/generic/nsFrame.cpp#1364.
I believe the current version of GetContentAndOffsetsFromPoint uses the
aContentOffsetEnd to deal with this case.  I'll try to understand that code a
bit better sometime.

I'll try to get to the conversion to frame coords part sometime soon.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #2)
> Fine, I guess.  I don't see how that helps, though; there still needs to be
> handling for things like clicking between lines where you want content within
> the lines.

Right, that's exactly what GetCaretPositionFromPoint would be used for.

> > For content that automatically selects when clicked on, it
> > would return the prev-edge of the selection.
> 
> Currently, I believe content that automatically selects when clicked on is
> handled by code like that at
> http://lxr.mozilla.org/seamonkey/source/layout/generic/nsFrame.cpp#1364.
> I believe the current version of GetContentAndOffsetsFromPoint uses the
> aContentOffsetEnd to deal with this case.  I'll try to understand that code a
> bit better sometime.

Actually I think that's mostly here:
http://lxr.mozilla.org/seamonkey/source/layout/generic/nsFrame.cpp#1373
Which seems like the right way to do it.

> I'll try to get to the conversion to frame coords part sometime soon.

Super!

Comment 4

[Eli Friedman]

Attachment: Patch v1

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

-    nsPoint pt = nsLayoutUtils::GetEventCoordinatesForNearestView(aEvent, this);
+    nsPoint pt = nsLayoutUtils::GetEventCoordinatesRelativeTo(aEvent, this);
     frameselection->HandleDrag(aPresContext, this, pt);

Why are you doing this? You're not changing the interpretation of aPoint in
HandleDrag, are you? Or is that an existing bug that you're fixing now?

+      nsPoint newPoint = aPoint - closestFrame->GetPosition();

I think you should use closestFrame->GetOffsetTo(this) here. Otherwise when
"this" is a scrollframe and closestFrame is the scrolled frame, you'll lose.

+  nsRect thisRect = GetRect() - GetPosition();

Another way of doing this is nsRect(nsPoint(0, 0), GetSize());

In nsTextFrame, you can remove the code that sets "origin" completely.

Please add comments in nsIFrame.h and nsTextFrame.h explaining that the point
parameters are now relative to "this" frame's origin.

-  // XXX The client point storage was moved to the DOM event, so now this can't
-  // work.  A real fix would require fixing the mess that is popup coordinates
-  // in layout.  For now, don't bother setting the point.
-  //event.point.x = aX;
-  //event.point.y = aY;
+  // XXX This is messed up: it needs to account for widgets.
+  event.widget = GetClosestView()->GetNearestWidget(&event.refPoint);
+  event.refPoint.x = aX;
+  event.refPoint.y = aY;

Looks like you're fixing the popup coordinate bug here... generally it's better
to fix unrelated bugs with separate patches, but I won't force the issue. But
here, do you mean to set event.refPoint and then immediately overwrite it with
aX,aY? Or should we be adding aX,aY?

Other than that, it looks spectacular. It's great to see all the code getting
simpler.

Comment 6

[Eli Friedman]

(In reply to comment #5)
> -    nsPoint pt = nsLayoutUtils::GetEventCoordinatesForNearestView(aEvent, this);
> +    nsPoint pt = nsLayoutUtils::GetEventCoordinatesRelativeTo(aEvent, this);
>      frameselection->HandleDrag(aPresContext, this, pt);
> 
> Why are you doing this? You're not changing the interpretation of aPoint in
> HandleDrag, are you? Or is that an existing bug that you're fixing now?

Actually, I am changing the interpretation of aPoint for
nsSelection::HandleDrag; the only other caller is
http://lxr.mozilla.org/seamonkey/source/layout/generic/nsSelection.cpp#618, and
I believe that caller is also passing frame coords.  The method doesn't need any
changes because with the change to GetContentAndOffsetsFromPoint, it and its
child method ConstrainFrameAndPointToAnchorSubtree interpret the point relative
to the frame.

> 
> +      nsPoint newPoint = aPoint - closestFrame->GetPosition();
> 
> I think you should use closestFrame->GetOffsetTo(this) here. Otherwise when
> "this" is a scrollframe and closestFrame is the scrolled frame, you'll lose.

That's handled by
http://lxr.mozilla.org/seamonkey/source/layout/generic/nsGfxScrollFrame.cpp#1410
  .  In light of that, would you like me to change the code you cited and/or
GetChildContentAndOffsetsFromPoint?

> 
> +  nsRect thisRect = GetRect() - GetPosition();
> 
> Another way of doing this is nsRect(nsPoint(0, 0), GetSize());
> 
> In nsTextFrame, you can remove the code that sets "origin" completely.
>
> Please add comments in nsIFrame.h and nsTextFrame.h explaining that the point
> parameters are now relative to "this" frame's origin.

Will do (for all 3).  I guess I'll also add a comment for
nsSelection::HandleDrag as well.

> -  // XXX The client point storage was moved to the DOM event, so now this can't
> -  // work.  A real fix would require fixing the mess that is popup coordinates
> -  // in layout.  For now, don't bother setting the point.
> -  //event.point.x = aX;
> -  //event.point.y = aY;
> +  // XXX This is messed up: it needs to account for widgets.
> +  event.widget = GetClosestView()->GetNearestWidget(&event.refPoint);
> +  event.refPoint.x = aX;
> +  event.refPoint.y = aY;
> Looks like you're fixing the popup coordinate bug here... generally it's better
> to fix unrelated bugs with separate patches, but I won't force the issue. But
> here, do you mean to set event.refPoint and then immediately overwrite it with
> aX,aY? Or should we be adding aX,aY?
> 
> Other than that, it looks spectacular. It's great to see all the code getting
> simpler.

I didn't mean for that to slip in.  I'll address your comment in the bug 
filed for that.

I should have a new patch by tomorrow.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #6)
> Actually, I am changing the interpretation of aPoint for
> nsSelection::HandleDrag; the only other caller is
> http://lxr.mozilla.org/seamonkey/source/layout/generic/nsSelection.cpp#618,
> and I believe that caller is also passing frame coords.  The method doesn't
> need any changes because with the change to GetContentAndOffsetsFromPoint, it
> and its child method ConstrainFrameAndPointToAnchorSubtree interpret the point
> relative to the frame.

Great. Better document that in nsIFrameSelection.h.

> > +      nsPoint newPoint = aPoint - closestFrame->GetPosition();
> > 
> > I think you should use closestFrame->GetOffsetTo(this) here. Otherwise when
> > "this" is a scrollframe and closestFrame is the scrolled frame, you'll lose.
> 
> That's handled by
> http://lxr.mozilla.org/seamonkey/source/layout/generic/nsGfxScrollFrame.cpp#1410
>   .  In light of that, would you like me to change the code you cited and/or
> GetChildContentAndOffsetsFromPoint?

I think we should fix that here and remove the nsGfxScrollFrame override.

> I should have a new patch by tomorrow.

This is great work.

Comment 8

[Eli Friedman]

Attachment: Patch v2

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked in! Thanks!

Comment 10

[Ivan Vadovic]

Is GetContentAndOffsetsFromPoint or something similar available to javascript? 

Comment 11

[Eli Friedman]

This isn't really the right place for questions, but there is rangeOffset and
rangeParent for DOM events; see
http://www.faqts.com/knowledge_base/view.phtml/aid/33674 for a little more info.
 If you have more questions, you can email me.