DragListener bug when computing localPoint when using "applyOffset" option

[zepumph]

The title of the issue is my best guess of what this bug is, though I'm not sure. While working on phetsims/projectile-motion#183, @jessegreenberg @jonathanolson and I found that there was a bug with the DragListener in TracerNode.js

Basically when applying the below patch, dragging the tracer out of the toolbox will bring warp the tracerNode to the origin of the screenView (i.e. top left of layout bounds).

We think that this has to do with the following two lines of code in DragListener.applyParentOffset

    parentPoint.subtract( this.localToParentPoint( scratchVector2A.set( this._localPoint ) ) );
    parentPoint.add( this.localToParentPoint( scratchVector2A.setXY( 0, 0 ) ) );

Somehow, even though the Tracer's initial position (in model) is 10,10, and the toolbox is far from any origin, after these two lines, parentPoint ends up being 0,0 in view coords.

Another piece of data is that as we enter these two lines of code, this._localPoint has the same values as this._parentPoint. I am unable to comment any further on this issue, as I don't have enough knowledge about DragListener to suggest how that may or may not be buggy. Over to you @jonathanolson for when you finish up DragListener work. Tagging @ariel-phet as well to note one more issue for that work load.

Index: js/common/view/TracerNode.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/common/view/TracerNode.js	(revision 73d8e7e17b370ac2417c24aabf11200a71a5fd2e)
+++ js/common/view/TracerNode.js	(date 1573253277967)
@@ -156,8 +156,8 @@
       locationProperty: tracer.positionProperty,
       modelViewTransform: transformProperty.get(),
       dragBoundsProperty: dragBoundsProperty,
-      offsetLocation: () => DRAG_OFFSET,
-      applyOffset: false, // ignore where the pointer pressed in relation to the TracerNode origin
+      // offsetLocation: () => DRAG_OFFSET,
+      applyOffset: true, // ignore where the pointer pressed in relation to the TracerNode origin
       start: () => self.isUserControlledProperty.set( true ),
       end: () => self.isUserControlledProperty.set( false ),
       tandem: options.tandem.createTandem( 'dragListener' )

[zepumph]

@jonathan I ran into this again while converting a MovableDragHandler over in phetsims/gravity-force-lab#191 in ISLCRulerNode. Gravity Force Lab would like to use the applyOffset feature, and hopefully we can publish it in the next month or so. I'm going to assign @ariel-phet to help prioritize this bug.

I added this TODO in ISLCRulerNode:
applyOffset: false, // TODO: we want to be able to apply offset here, but can't because of https://github.com/phetsims/scenery/issues/1014

[zepumph]

Note that in GFL, this is just a DragListener, it isn't being forwarded from anything.

[zepumph]

This bug is blocking phetsims/gravity-force-lab#191 (publication of GFL), so @ariel-phet recommended marking as high priority.

[zepumph]

Steps to reproduce this problem (in projectile-motion):

• In TracerNode
• Comment out options to DragListener on lines 159/160:

     // offsetLocation: () => DRAG_OFFSET,
     // applyOffset: true, // ignore where the pointer pressed in relation to the TracerNode origin

• Load the sim and drag the blue tracer node out of the tool box
• On mouse down you will find that the tracer node jumps to the origin of the screen ( 0,0 of the layout bounds). Note that the center of the tracer node (0,0) is at the center of the crosshairs.

[jonathanolson]

The problem in both of these cases was presumably the core difference in how MovableDragHandler and DragListener handle offsets. I've added an option to use the MovableDragHandler-style offsets (based in the parent coordinate frame, and requiring a locationProperty). The DragListener style I believe is superior for a range of cases (except for where we can't translate/transform one node), so I believe it's best to include both as options.

Notably, if we're porting from MovableDragHandler, this option may be helpful to include (if the drag offset seems buggy). Is that worth documenting somewhere (in MovableDragHandler?)

@zepumph can you review my changes? I'm available to discuss anything about these.

[zepumph]

This is looking very nice! I added doc directly to that option, since I feel like I wouldn't likely look at MoveableDragHandler if I was converting to use DragListener. What do you think!.
I see this as totally fixed in GFL. I just applied a bit of cleanup over there.

In Projectile Motion I think we are close, but it took the addition of useParentOffset to realize that I don't actually want the offsetLocation option. In the published sim this is only occurring when pulling it out of the toolbox. So I cleaned that up too and documented.

I don't really trust my documentation much. Please review and see if I grasp this bug well enough to have documented it accurately. Otherwise I think we are good to close.

[jonathanolson]

All of the changes above look great, thanks!