296036 - Fix event coordinate handling

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

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

nsGUIEvents passed through the view, frame and DOM event system (e.g. in
GetFrameForPoint and HandleEvent) modify their .point member on the fly and the
interpretation of that point is really convoluted. We need to fix this so that
the invariants are simple and universally observed.

Comment 1

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

Here's a strawman proposal:

-- event->refpoint remains as it is --- always in device pixels relative to the
given widget in nsGUIEvent::widget

-- remove event->point completely. Pass an nsPoint around explicitly as a
parameter alongside the event, where necessary. When calling a method on a frame
or view, or where a frame or view is passed alongside, then the point will
always be relative to the origin of that frame or view.

Comment 2

[Boris Zbarsky [:bzbarsky]]

I think I'm ok with passing the point along explicitly... (and we can sprinkle
about asserts about it and refpoint being equal once you convert them to the
same coordinate system).

Comment 3

[Eli Friedman]

Attachment: Patch v1

Comment 4

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

Basically this looks good and I think it shows we're on the right track, it gets
rid of a lot of grungy and error-prone code.

+      TranslateEventCoords(aEvent->refPoint, mLastClickPoint);

I think that's a bug ... refPoint has to be translated to the current frame here.

+	gAlphaBlendChecked = !gAlphaBlendChecked;

This looks like something else...

Things this patch doesn't address (which could probably be done as separate
patches):
-- GetFrameForPoint/GetFrameForPointUsing should take a point in the frame's
coordinate system, not in its parent's coordinate system
-- GetEventCoordinatesForNearestView should not need to exist. Everywhere it's
needed, we should modify the code that takes view-relative points so it takes
frame-relative points instead.
-- Defining client event points in DOM events.

Comment 5

[Eli Friedman]

Attachment: Patch v2

This version addresses the comments.

Comment 6

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

This is 1.9 material and I'll review it then.

Comment 7

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

Comment on attachment 189733 [details] [diff] [review]
Patch v2

please re-request review after the branch has happened

Comment 8

[Eli Friedman]

Attachment: Patch v3

Comment 9

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

In GetLayerPoint:
+  mPresContext->EventStateManager()->GetEventTarget(&targetFrame);

This isn't really right, since it assumes this event is the currently processed
event, which may not be true. I think you should just return mClientPoint and be
done with it.

+      nsPoint widgetToView;
+      nsIView* baseView = nsIView::GetViewFor(mouseEvent->widget);
+      baseView->GetNearestWidget(&widgetToView);
+      float pixelsToTwips;
+      pixelsToTwips = aPresContext->DeviceContext()->DevUnitsToTwips();
+      nsPoint refPointTwips(NSIntPixelsToTwips(aEvent->refPoint.x, pixelsToTwips),
+                            NSIntPixelsToTwips(aEvent->refPoint.y, pixelsToTwips));
+      nsPoint eventPoint = refPointTwips + widgetToView -
aView->GetOffsetTo(baseView);
       if (parentWidget &&
-        (aView->GetBounds() - aView->GetPosition()).Contains(aEvent->point)) {
+          aView->GetBounds().Contains(eventPoint)) {

How about moving this to a new method nsPoint
nsLayoutUtils::TranslateWidgetToView(nsIWidget* aWidget, nsIntPoint aPt,
nsIView* aView)? Note that your code is slightly incorrect; you need
aView->GetBounds() - aView->GetPosition() because aView->GetBounds returns x,y
coordinates relative to aView's parent. GetEventCooridinatesRelativeTo could use
this new method.

[nsIntPoint == nsPoint for now, but later we might change nsPoint to be floats
in which case widget-relative pixel coordinates would remain ints.]

In nsLayoutUtils::GetEventCoordinatesRelativeTo, we should probably assert that
the widget returned by GetNearestWidget is equal to GUIEvent->widget.

nsLayoutUtils::GetEventCoordinatesRelativeTo
+  aFrame->GetOffsetFromView(viewToFrame, &frameView);

This method is evil. Please don't call it. Especially not here where
GetClosestView is the right thing.

nsLayoutUtils::GetEventCoordinatesRelativeTo
+  return mousePt + widgetToView - frameView->GetOffsetTo(view) - viewToFrame;

Shouldn't this be "+ viewToFrame"?

+/**
+   * Get the coordinates of a given native mouse event, relative to the nearest 
+   * view for a given frame.

Please add
  * The "nearest view" is the view returned by nsFrame::GetOffsetFromView.
  * XXX this is extremely BOGUS because "nearest view" is a mess; every
  * use of this method is really a bug!

+  nsPoint eventPoint;
+

Can't this be declared below, where it's assigned?

+        if (mRect.Contains(nsLayoutUtils::GetEventCoordinatesRelativeTo(aEvent,
+                                                                        this)))

This isn't correct, because mRect is relative to the frame's parent. Try passing
GetParent() instead of this.

 nsPoint
 nsSliderFrame::EventPointInOurCoords(nsEvent* aEvent)
 {
-  // aEvent->point is in the coordinate system of the view returned by
-  // GetOffsetFromView.
-  nsPoint eventPoint = aEvent->point;
-  nsPoint viewOffset;
-  nsIView* dummy;
-  GetOffsetFromView(viewOffset, &dummy);
-  return eventPoint - viewOffset;
+  return nsLayoutUtils::GetEventCoordinatesRelativeTo(aEvent, this);
 }

Maybe you should just remove this method and replace its callers with this function.

             nsPoint rootOffset(0, 0);
             for (nsView *v = baseView; v != mRootView; v = v->GetParent())
               v->ConvertToParentCoords(&rootOffset.x, &rootOffset.y);
-            // aEvent->point is relative to the widget, i.e. the view top-left,
-            // so we need to add the offset to the view origin
-            rootOffset += baseView->GetDimensions().TopLeft();
-            mMouseLocation.MoveTo(NSTwipsToIntPixels(rootOffset.x, t2p) +
-                                    aEvent->point.x,
-                                  NSTwipsToIntPixels(rootOffset.y, t2p) +
-                                    aEvent->point.y);
+
+            if (aEvent->widget) {
+              nsRect widgetRect(aEvent->refPoint, nsSize(0,0));
+              nsRect screenRect;
+              aEvent->widget->WidgetToScreen(widgetRect, screenRect);
+              mRootView->GetWidget()->ScreenToWidget(screenRect, widgetRect);
+              mMouseLocation = widgetRect.TopLeft();
+            }
+
Why are you doing this? Can't you just convert the widget point to aEvent's
widget's view coordinates and then call GetOffsetTo to get it into mRootView's
coordinates, then convert back to pixels? (It's safe to assume that mRootView's
origin coincides with its widget's origin)

Apart from those comments, it looks great.

Comment 10

[Eli Friedman]

(In reply to comment #9)
> In GetLayerPoint:
> +  mPresContext->EventStateManager()->GetEventTarget(&targetFrame);
> 
> This isn't really right, since it assumes this event is the currently processed
> event, which may not be true. I think you should just return mClientPoint and be
> done with it.

The issue is that some websites use layerX and layerY expecting them to be in
terms of the nearest positioned element, so changing the behavior would break
sites.  I thought calling that method was OK because rangeOffset and rangeParent
already call it.  Anyway, as near as I can tell, in Gecko, Javascript can only
access the currently processed event anyway (whether that's correct is an
entirely different issue; Opera does allows saving events between dispatches). 
Overall, what I wrote works, and the whole thing's a mess that's outside the
scope of this patch.

> +      nsPoint widgetToView;
> +      nsIView* baseView = nsIView::GetViewFor(mouseEvent->widget);
> +      baseView->GetNearestWidget(&widgetToView);
> +      float pixelsToTwips;
> +      pixelsToTwips = aPresContext->DeviceContext()->DevUnitsToTwips();
> +      nsPoint refPointTwips(NSIntPixelsToTwips(aEvent->refPoint.x,
pixelsToTwips),
> +                            NSIntPixelsToTwips(aEvent->refPoint.y,
pixelsToTwips));
> +      nsPoint eventPoint = refPointTwips + widgetToView -
> aView->GetOffsetTo(baseView);
>        if (parentWidget &&
> -        (aView->GetBounds() - aView->GetPosition()).Contains(aEvent->point)) {
> +          aView->GetBounds().Contains(eventPoint)) {
> 
> How about moving this to a new method nsPoint
> nsLayoutUtils::TranslateWidgetToView(nsIWidget* aWidget, nsIntPoint aPt,
> nsIView* aView)? Note that your code is slightly incorrect; you need
> aView->GetBounds() - aView->GetPosition() because aView->GetBounds returns x,y
> coordinates relative to aView's parent. GetEventCooridinatesRelativeTo could use
> this new method.
> 
Done.

> 
> In nsLayoutUtils::GetEventCoordinatesRelativeTo, we should probably assert that
> the widget returned by GetNearestWidget is equal to GUIEvent->widget.
> 
Done (in split off method).

> nsLayoutUtils::GetEventCoordinatesRelativeTo
> +  aFrame->GetOffsetFromView(viewToFrame, &frameView);
> 
> This method is evil. Please don't call it. Especially not here where
> GetClosestView is the right thing.

GetClosestView actually doesn't work in this situation.  Replacing the call to
GetOffsetFromView with GetClosestView breaks scrolling.

> 
> nsLayoutUtils::GetEventCoordinatesRelativeTo
> +  return mousePt + widgetToView - frameView->GetOffsetTo(view) - viewToFrame;
> 
> Shouldn't this be "+ viewToFrame"?

The function returns a positive offset calculated by addding up the positions of
the ancestor frames.  Therefore, what I'm doing is correct (from widget to root
view to frame view to frame).  Changed the variable name to make that clearer.

> 
> +/**
> +   * Get the coordinates of a given native mouse event, relative to the nearest 
> +   * view for a given frame.
> 
> Please add
>   * The "nearest view" is the view returned by nsFrame::GetOffsetFromView.
>   * XXX this is extremely BOGUS because "nearest view" is a mess; every
>   * use of this method is really a bug!
> 
Done.
> +  nsPoint eventPoint;
> +
> 
> Can't this be declared below, where it's assigned?

Fixed.

> 
> +        if (mRect.Contains(nsLayoutUtils::GetEventCoordinatesRelativeTo(aEvent,
> +                                                                        this)))
> 
> This isn't correct, because mRect is relative to the frame's parent. Try passing
> GetParent() instead of this.

Fixed.

> 
>  nsPoint
>  nsSliderFrame::EventPointInOurCoords(nsEvent* aEvent)
>  {
> -  // aEvent->point is in the coordinate system of the view returned by
> -  // GetOffsetFromView.
> -  nsPoint eventPoint = aEvent->point;
> -  nsPoint viewOffset;
> -  nsIView* dummy;
> -  GetOffsetFromView(viewOffset, &dummy);
> -  return eventPoint - viewOffset;
> +  return nsLayoutUtils::GetEventCoordinatesRelativeTo(aEvent, this);
>  }
> 
> Maybe you should just remove this method and replace its callers with this
function.

Done.

> 
>              nsPoint rootOffset(0, 0);
>              for (nsView *v = baseView; v != mRootView; v = v->GetParent())
>                v->ConvertToParentCoords(&rootOffset.x, &rootOffset.y);
> -            // aEvent->point is relative to the widget, i.e. the view top-left,
> -            // so we need to add the offset to the view origin
> -            rootOffset += baseView->GetDimensions().TopLeft();
> -            mMouseLocation.MoveTo(NSTwipsToIntPixels(rootOffset.x, t2p) +
> -                                    aEvent->point.x,
> -                                  NSTwipsToIntPixels(rootOffset.y, t2p) +
> -                                    aEvent->point.y);
> +
> +            if (aEvent->widget) {
> +              nsRect widgetRect(aEvent->refPoint, nsSize(0,0));
> +              nsRect screenRect;
> +              aEvent->widget->WidgetToScreen(widgetRect, screenRect);
> +              mRootView->GetWidget()->ScreenToWidget(screenRect, widgetRect);
> +              mMouseLocation = widgetRect.TopLeft();
> +            }
> +
> Why are you doing this? Can't you just convert the widget point to aEvent's
> widget's view coordinates and then call GetOffsetTo to get it into mRootView's
> coordinates, then convert back to pixels? (It's safe to assume that mRootView's
> origin coincides with its widget's origin)

Fixed.  I removed my change there; I forgot that that the current code actually
refers to the correct point.

New patch coming up.

Comment 11

[Eli Friedman]

Attachment: patch v4

Comment 12

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

(In reply to comment #10)
> The issue is that some websites use layerX and layerY expecting them to be in
> terms of the nearest positioned element, so changing the behavior would break
> sites.  I thought calling that method was OK because rangeOffset and
> rangeParent already call it.  Anyway, as near as I can tell, in Gecko,
> Javascript can only access the currently processed event anyway (whether
> that's correct is an entirely different issue; Opera does allows saving events
> between dispatches).  Overall, what I wrote works, and the whole thing's a
> mess that's outside the scope of this patch.

Fine.

> > nsLayoutUtils::GetEventCoordinatesRelativeTo
> > +  aFrame->GetOffsetFromView(viewToFrame, &frameView);
> > 
> > This method is evil. Please don't call it. Especially not here where
> > GetClosestView is the right thing.
> 
> GetClosestView actually doesn't work in this situation.  Replacing the call to
> GetOffsetFromView with GetClosestView breaks scrolling.

Then something else is wrong because GetClosestView() should be the right thing.
For scrolled frames, e.g., a scrolled 400x400px block scrolled down 100px in a
window of 500x200px, assuming 15px-wide scrollbars on the right and bottom and
no borders/padding, we should have the following situation:
  nsView (nsGfxScrollFrame)
    nsScrollPortView (0,0,485,200) (no frame)
      nsView (0,-100,485,400) (nsBlockFrame (0,0,485,400))

Now suppose an event occurs in scrollport view's widget at (0,100).
GetClosestView on the blockframe should return the innermost view and an offset
of (0,0). In TranslateWidgetToView, innermostView->GetOffsetTo(scrollportview)
should return (0,100) so the point should be translated to (0,100) - (0,100) =
(0,0) which should be correct... What part of this picture does not correspond
to reality? :-)

Otherwise, excellent.

Comment 13

[Eli Friedman]

Attachment: Patch v5

Stupid mistake on my part.  GetFrameForPoint is really annoying.  Changed to
GetClosestView and fixed nsPresShell.cpp.

Comment 14

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

(In reply to comment #13)
> Stupid mistake on my part.  GetFrameForPoint is really annoying.

Yeah. let's fix that :-)

Comment 15

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

Comment on attachment 193410 [details] [diff] [review]
Patch v5

+  nsPoint frameToView;
+  frameView = aFrame->GetClosestView(&frameToView);

Put these on one line. Super! I'll land it.

Comment 16

[Eli Friedman]

Attachment: Patch v6

Changed.

Comment 17

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

I just realized that this patch only fixes Windows. We need to fix widget code
on all the other platforms too.

Comment 18

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

It looks like we just need wholesale /point/refPoint/ changes, so I've made them
and I'll check in (and watch Tinderboxen vigorously!)

Comment 19

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

checked in. Praying.

Comment 20

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

Looking at
http://tinderbox.mozilla.org/bonsai/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/layout/generic&command=DIFF_FRAMESET&file=nsHTMLFrame.cpp&rev1=1.149&rev2=1.150&root=/cvsroot
I just noticed that there's some cruft there that computes offset and view that
is no longer needed.

Comment 21

[Joey Minta]

This patch seems to break onpopupshowing event coordinates.  Prior to checkin,
an onpopupshowing event had clientX and clientY properties that were generally
not 0.  Now they are always 0.

event.screenX and event.screenY are also 0 for these, but this problem dates
back to at least FF 1.0.x

Testcase to follow.

Comment 22

[Joey Minta]

Attachment: onpopupshowing testcase

testcase for onpopupshowing event coordinates

Comment 23

[Martijn Wargers (dead)]

Joey Minta, please file a new bug for it, and make it blocking this bug.

Comment 24

[Boris Zbarsky [:bzbarsky]]

File regressions as new bugs.  Do NOT reopen the original bug for regressions.