GrabDragInteraction: Code Review

[samreid]

While working on GrabDragInteraction usage in Buoyancy, @zepumph and I wanted to take a closer look to see if we would like to improve anything as part of this work.

[samreid]

Unit test is failing and is also in *.js.

scenery-phet: 
F
Module Failed: GrabDragInteraction
  Test Failed: GrabDragInteraction defaults
    Assertion Failed: default to grabbable

    at testDefaultGrabbable (http://localhost:63989/chipper/dist/js/scenery-phet/js/accessibility/GrabDragInteractionTests.js:45:14)
    at Object.<anonymous> (http://localhost:63989/chipper/dist/js/scenery-phet/js/accessibility/GrabDragInteractionTests.js:55:3)
    at runTest (http://localhost:63989/sherpa/lib/qunit-2.20.0.js:2697:30)
    at Test.run (http://localhost:63989/sherpa/lib/qunit-2.20.0.js:2680:5)
    at http://localhost:63989/sherpa/lib/qunit-2.20.0.js:2951:11
    at processTaskQueue (http://localhost:63989/sherpa/lib/qunit-2.20.0.js:2271:22)    
    Assertion Failed: default to grabbable

[samreid]

This is a straightforward refactoring that seems nice but I wasn't sure how to test it:

```diff Subject: [PATCH] Add REVIEW comment, see https://github.com/phetsims/scenery-phet/issues/869 --- Index: js/accessibility/GrabDragInteraction.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/accessibility/GrabDragInteraction.ts b/js/accessibility/GrabDragInteraction.ts --- a/js/accessibility/GrabDragInteraction.ts (revision b9a8d160e096bf263e6823da1ade22d297748eac) +++ b/js/accessibility/GrabDragInteraction.ts (date 1724884683824) @@ -709,13 +709,16 @@ // You can override this with onGrabbable() if necessary. this.node.setPDOMAttribute( 'aria-roledescription', this.supportsGestureDescription ? movableStringProperty : buttonStringProperty ); - if ( this.shouldAddAriaDescription( this.grabDragModel.grabDragCueModel.numberOfGrabs ) ) { + const shouldDescribe = this.shouldAddAriaDescription( this.grabDragModel.grabDragCueModel.numberOfGrabs ); + const isDescribed = this.node.hasAriaDescribedbyAssociation( this.descriptionAssociationObject ); + + if ( !isDescribed && shouldDescribe ) { // this node is aria-describedby its own description content, so that the description is read automatically // when found by the user - !this.node.hasAriaDescribedbyAssociation( this.descriptionAssociationObject ) && this.node.addAriaDescribedbyAssociation( this.descriptionAssociationObject ); + this.node.addAriaDescribedbyAssociation( this.descriptionAssociationObject ); } - else if ( this.node.hasAriaDescribedbyAssociation( this.descriptionAssociationObject ) ) { + else if ( isDescribed && !shouldDescribe ) { this.node.removeAriaDescribedbyAssociation( this.descriptionAssociationObject ); }

[zepumph]

In https://github.com/phetsims/scenery-phet/issues/868#issuecomment-2316114484 @jessegreenberg also recommended using Properties for the "showGrab/DragCue()" options.

[zepumph]

We will keep going through TODO items together, lots of really great work came from today, and we fixed at least 3 bugs, and made this class much more maintainable.

[zepumph]

Down to 27 TODO!

[zepumph]

All TODOs have been accounted for. Thanks @samreid for finding all these good changes to be done. I also discussed these changes generally with JG a few minutes ago, and we want to have him do a spot check before closing everything out. Thanks all!

[jessegreenberg]

This is really great work, nicely done! Thank you all for taking it on. To review, I read through GrabDragModel, GrabDragInteraction, tested BASE and Friction, GrabDragInteractionTests, and the simple usage in demoGrabDragInteraction. It all worked well.

• For demoGrabDragInteraction, the interaction cue looks like it is placed at the origin of the rectangle. Is that how it has always been? I can't remember if that is the expected default or if it should appear below the Node and centered horizontally.
• Should we note that GrabDragModel is scenery-phet internal?
• I think interactionStateProperty should be disposed in GrabDragModel.

Thanks again for your work here!

[zepumph]

• For demoGrabDragInteraction, the interaction cue looks like it is placed at the origin of the rectangle. Is that how it has always been? I can't remember if that is the expected default or if it should appear below the Node and centered horizontally.

Looks like it has always been like that: https://phet-dev.colorado.edu/html/scenery-phet/1.0.0-dev.21/phet/scenery-phet_all_phet.html

Closing. Thanks all for everything here.