Add grab interaction to the ruler

[zepumph]

Related to #138, I think we should add grab/drag interaction before adding shortcuts.

[zepumph]

This was a very simple change to implement, and I got it working with only one small bug. There is a bug that this introduced. It seems like when I added the grab/drag interaction, the modelViewTransform was off a bit. On first drag, it would jump to a point a bit down and to the left, like that was its origin, then on reset the ruler goes back there, now where it starts at.

After investigating further I began wondering why ISLCRulerNode is set up using composition. All listeners and other pieces are set on this, the ISLCRulerNode, this includes bother drag listeners, and PDOM options. But then why is the child RulerNode having the center set on it like:

    // @public - ruler node is never destroyed, no listener disposal necessary
    model.rulerPositionProperty.link( function( value ) {
      ruler.center = modelViewTransform.modelToViewPosition( value );
    } );

I would have expected to see inheritance used instead because basically ISLCRulerNode is just a fancy RulerNode. After making somewhat of a conversion in that direction, it seems like the "origin" position in the view is now even more off than it was.

Here is a patch for the whole switcharoo where I am trying to use RulerNode.call

Index: js/view/ISLCRulerNode.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/view/ISLCRulerNode.js	(revision 83f7c99bbff2a728517b838912757b392a536bcf)
+++ js/view/ISLCRulerNode.js	(date 1547597181740)
@@ -13,6 +13,7 @@
   var AccessiblePeer = require( 'SCENERY/accessibility/AccessiblePeer' );
   var Bounds2 = require( 'DOT/Bounds2' );
   var FocusHighlightFromNode = require( 'SCENERY/accessibility/FocusHighlightFromNode' );
+  var GrabDragInteraction = require( 'SCENERY_PHET/accessibility/GrabDragInteraction' );
   var inherit = require( 'PHET_CORE/inherit' );
   var inverseSquareLawCommon = require( 'INVERSE_SQUARE_LAW_COMMON/inverseSquareLawCommon' );
   var ISLCA11yStrings = require( 'INVERSE_SQUARE_LAW_COMMON/ISLCA11yStrings' );
@@ -27,6 +28,8 @@
 
   // strings
   var unitsCentimetersString = require( 'string!INVERSE_SQUARE_LAW_COMMON/units.centimeters' );
+
+  // a11y strings
   var rulerHelpTextString = ISLCA11yStrings.rulerHelpText.value;
   var rulerLabelString = ISLCA11yStrings.rulerLabel.value;
   var moveInFourDirectionsString = ISLCA11yStrings.moveInFourDirections.value;
@@ -35,6 +38,7 @@
   var RULER_WIDTH = 500;
   var RULER_HEIGHT = 35;
   var RULER_INSET = 10;
+  var RULER_MAJOR_TICK_WIDTH = 50;
 
   /**
    * @param {ISLCModel} model
@@ -47,54 +51,49 @@
   function ISLCRulerNode( model, screenHeight, modelViewTransform, tandem, options ) {
 
     options = _.extend( {
-      snapToNearest: null,
+      snapToNearest: 0,
       majorTickLabels: [ '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '10' ],
       unitString: unitsCentimetersString,
       backgroundFill: '#ddd',
-      rulerInset: RULER_INSET,
+
+      insetsWidth: RULER_INSET,
+      minorTicksPerMajorTick: 4,
+      majorTickFont: new PhetFont( 12 ),
+      unitsFont: new PhetFont( 10 ),
+      unitsSpacing: 3,
+      tandem: tandem,
+
+      cursor: 'pointer',
+      cssTransform: true,
 
       // a11y
       moveOnHoldDelay: 750,
-      tagName: 'div'
+      focusHighlightLayerable: true,
     }, options );
 
+
+    assert && assert( options.tagName === undefined, 'RulerNode sets its own tagName' );
+
     var majorTickLabels = options.majorTickLabels;
     var rulerUnitString = options.unitString;
+    var self = this;
 
-    Node.call( this, {
-      cursor: 'pointer',
-      cssTransform: true,
-      tandem: tandem,
-      tagName: 'div',
-      focusable: true,
-      focusHighlightLayerable: true
-    } );
 
-    var ruler = new RulerNode(
+    RulerNode.call( this,
       RULER_WIDTH,
       RULER_HEIGHT,
-      50,
+      RULER_MAJOR_TICK_WIDTH,
       majorTickLabels,
       rulerUnitString,
-      {
-        backgroundFill: options.backgroundFill,
-        insetsWidth: options.rulerInset,
-        minorTicksPerMajorTick: 4,
-        majorTickFont: new PhetFont( 12 ),
-        snapToNearest: options.snapToNearest ? options.snapToNearest : 0,
-        unitsFont: new PhetFont( 10 ),
-        unitsSpacing: 3,
-        tandem: tandem.createTandem( 'ruler' )
-      }
+      options
     );
-    this.addChild( ruler );
 
-    ruler.mouseArea = Shape.rectangle( 0, 0, ruler.bounds.width, RULER_HEIGHT );
-    ruler.touchArea = ruler.mouseArea;
+    this.mouseArea = Shape.rectangle( 0, 0, this.bounds.width, RULER_HEIGHT );
+    this.touchArea = this.mouseArea;
 
     // @public - ruler node is never destroyed, no listener disposal necessary
     model.rulerPositionProperty.link( function( value ) {
-      ruler.center = modelViewTransform.modelToViewPosition( value );
+      self.center = modelViewTransform.modelToViewPosition( value );
     } );
 
     // ruler drag bounds (in model coordinate frame) - assumes a single point scale inverted Y mapping
@@ -127,11 +126,16 @@
       }
     } ) );
 
+    // TODO: delete me!
+    this.model = model;
+    window.ruler = this;
+    console.log( model.rulerPositionProperty.get() );
+
     // a11y - custom, layerable focus highlight
-    var focusHighlight = new FocusHighlightFromNode( ruler, { useLocalBounds: true } );
-    this.setFocusHighlight( focusHighlight);
+    var focusHighlight = new FocusHighlightFromNode( this, { useLocalBounds: true } );
+    this.setFocusHighlight( focusHighlight );
 
-    ruler.addChild( focusHighlight );
+    this.addChild( focusHighlight );
 
     // @private (a11y) - supports keyboard interaction, private so it can be stepped
     var keyboardDragDelta = modelViewTransform.modelToViewDeltaX( options.snapToNearest );
@@ -149,26 +153,56 @@
       // as key is held down
       drag: function() {
         if ( options.snapToNearest ) {
+          console.log( model.rulerPositionProperty.get() );
           var xModel = model.rulerPositionProperty.get().x;
           var snappedX = Util.roundSymmetric( xModel / options.snapToNearest ) * options.snapToNearest;
           model.rulerPositionProperty.set( new Vector2( snappedX, model.rulerPositionProperty.get().y ) );
         }
       }
     } );
-    this.addInputListener( keyboardDragListener );
+
+    this.a11yGrabDragInteraction = new GrabDragInteraction( this, {
+      objectToGrabString: rulerLabelString,
 
-    this.accessibleName = rulerLabelString;
-    this.helpText = rulerHelpTextString;
-    this.ariaRole = 'application';
-    this.setAccessibleAttribute( 'aria-roledescription', moveInFourDirectionsString );
-    this.addAriaDescribedbyAssociation( {
-      otherNode: this,
-      otherElementName: AccessiblePeer.DESCRIPTION_SIBLING,
-      thisElementName: AccessiblePeer.PRIMARY_SIBLING
+      // Empirically determined values to place the cue above the book.
+      grabCueOptions: {
+        x: 60,
+        y: -55
+      },
+      grabbableOptions: {
+        appendDescription: true,
+        helpText: rulerHelpTextString
+        // focusHighlight: focusHighlightRect
+      },
+
+      draggableOptions: {},
+      onDraggable: () => {
+        this.setAccessibleAttribute( 'aria-roledescription', moveInFourDirectionsString );
+        this.addAriaDescribedbyAssociation( {
+          otherNode: this,
+          otherElementName: AccessiblePeer.DESCRIPTION_SIBLING,
+          thisElementName: AccessiblePeer.PRIMARY_SIBLING
+        } );
+      },
+      onGrabbable: () => {
+        if ( this.hasAccessibleAttribute( 'aria-roledescription' ) ) {
+          this.removeAccessibleAttribute( 'aria-roledescription' );
+        }
+        this.ariaDescribedbyAssociations = [];
+      },
+
+      // dragCueNode: arrows,
+
+      listenersForDrag: [ keyboardDragListener ]
     } );
   }
 
   inverseSquareLawCommon.register( 'ISLCRulerNode', ISLCRulerNode );
 
-  return inherit( Node, ISLCRulerNode );
+  return inherit( Node, ISLCRulerNode, {
+
+    reset: function() {
+      this.a11yGrabDragInteraction.reset();
+    }
+  } );
 } );

I think for now I will revert and try to just fix the minor change in adding the GrabDragInteraction.

[zepumph]

I tracked down the bug. The RulerNode was being set in the view via its center, but when adding the grab/drag interaction there are a few nodes added to the focusHighlight passed in. Normally this wouldn't matter, but since this was using focusHighlightLayerable, the focusHighlight was a Node that was actually added as a child to the RulerNode. Thus the center and bounds changed because of the "grab cue" node that GrabDragInteraction.js tacks on.

The fix I made is less than ideal, but I couldn't figure out a better way currently. I'm removing the focusHighlight, setting the center (based on just the RulerNode) and then readding the focusHighlight as a child again each time the ruler's position Property changes.

Again this is less than ideal. @jessegreenberg would you mind reviewing this and seeing if you can find a better way around this. I'm also happy to talk to better explain the hacky hack I hacked into master right now (even though it is just 2 lines).

[terracoda]

@zepumph, I verified the keyboard interactions for grabbing and moving the ruler. This seems to be working nicely.

There are additional details (specific help text and alerts) in the design doc that will support the ruler interaction.

I will close this issue and open a new one about help text and alerts, and point you to the correct place in the design doc.

However, further work ruler interaction can likely be de-prioritized because there is no ruler in the BASICS sim.

[zepumph]

Sounds good @terracoda. I think it is still important for @jessegreenberg to have a look at the implementation, and some of their oddities, re-opening.

[zepumph]

@jessegreenberg here is light ping on review here, since we are working on the Grab/Drag interaction again, as well as this sim. It would be nice to know what you think about this usage.

[terracoda]

@zepumph, FYI I changed the label of the interaction and design context responses for grab and release. Please see updates in the design doc.

This likely doesn't affect much of the custom interaction part, though.

[terracoda]

Verifying mobile A11y gesture for
https://phet-dev.colorado.edu/html/gravity-force-lab/2.2.0-dev.16/phet/gravity-force-lab_en_phet.html

• Voice Over highlight doesn't move with the ruler
• If pink highlight gets activated on any control (ruler, spheres, check boxes, etc.), it seems difficult for it to go away.
• Force alert when changing mass gets always chopped off... "As mass gets bigger, force vectors get b-"
• Force alert when changing position always gets chopped off... I think it is because Voice Over's highlight is slow to react to the change in position "2.5 meters from mass 2. Force vect-"

The ruler sounds good:

• Measure distance ruler. Movable. If needed, grab ruler to measure distance between spheres. Double tap and hold to drab ruler. Lift finger to release.

But it can get stuck

• Ruler gets stuck behind the Mass Controls. I can't seem to drag it out from behind the mass controls. When I grab it in that location, and try to drag it, I end up changing the mass of mass 2 instead of moving the ruler.

[terracoda]

Verifying Keyboard for
https://phet-dev.colorado.edu/html/gravity-force-lab/2.2.0-dev.16/phet/gravity-force-lab_en_phet.html

• focus sounding fine (Measure Distance Ruler, button)
• on grab, hear "Ruler, move in four directions". Should be "Ruler, movable" or "Grabbed Ruler, movable"
• initial response is good - provides J+C hint.
• grab and drag working
• shortcuts working
• ruler position regions heard on grab are correct

Coming along nicely @zepumph. Waiting to test anything further.

[zepumph]

I created #194 through #197 to investigate the above bugs found with gesture mobile a11y. It didn't seem like there were any actionable steps to take for keyboard.

From here I would recommend creating new issues for further testing and results, and letting this issue be a review of the grab drag implementation added 8 months ago.

[terracoda]

@zepumph, the keyboard is working great. And I just posted #201 for gesture in case that wasn't already covered.

Close this issue when you feel the need.

[terracoda]

I guess we will close when the related issues are completed?

[jessegreenberg]

@zepumph I really apologize that it took me so long to review this.

The particular point of concern in #140 (comment) (issue around focusHighlightLayerable) was resolved in phetsims/inverse-square-law-common@d24287c so that is no longer a problem.

So I am reviewing the GrabDragInteraction usage in ISLCRulerNode. I only noticed one thing. The x value here seems specific to gravity-force-lab, I think it was chosen so that the ruler is between the two masses on start up

        // Empirically determined values to place the cue above the ruler.
        grabCueOptions: {
          x: 135,
          y: -45
        },

Should the cue be center-bottom aligned with the ruler by default and these values be moved to GFL?

[zepumph]

The x value here seems specific to gravity-force-lab

I think that the x value looks fine in both sims do you feel differently?

[jessegreenberg]

It looks a bit off center to me in that capture, but it is most noticeable in the "Atomic" screen.

Here it is horizontally centered on the in GFL which also looks better in my opinion:

[zepumph]

How does that look?

[terracoda]

Looks good to me :-)

[zepumph]

@jessegreenberg anything else here?

[jessegreenberg]

Thanks for the ping - looks good!