Grabbed tool should pop to the front

[samreid]

From #434

When grabbing a tool, it should go to the front (in z-ordering) relative to other grabbable tools. This will not be fixed for 2.0.

[samreid]

Measuring tape now moves to the front in the mentioned issue.

[samreid]

Implemented and would be nice to have some QA testing for this feature.

[brooklynlash]

Works in dev.24

[KatieWoe]

I noticed in 1.1.0 rc.3 that this works in most cases, but if the stopwatch is behind another tool, and you press one of its buttons, it does not jump to the top. It does if you grab a non-button area.

[samreid]

There is a natural place we could add a moveToFront listener in the restart button. But the play pause button is a BooleanRectangularToggleButton and has no apparent places to listen for press. We could listen to the play/pause property and move to front when it changes, but then it would also move to front when it changes without a button press. @jonathanolson and @pixelzoom I noticed you have commits in StopwatchNode, can you please recommend how to proceed?

[jonathanolson]

What is the desired behavior? I'm still very uneasy about having a default "moveToFront" inside a component (but I guess you can always wrap it with a Node if desired?)

But the play pause button is a BooleanRectangularToggleButton and has no apparent places to listen for press.

That seems like something that might be nice to have for this and other reasons.

I think the better approach is to NOT have moveToFront baked into StopwatchNode (especially just in the dragTarget!!!), but instead a client can add a listener to it that moves it to the front when interacted with.

[samreid]

I discussed with @arouinfar and we agreed that it's not a blocking publication issue that clicking the buttons doesn't bring the stopwatch to the front. Since waves intro 1.1.0-rc.3 moves the stopwatch node to front for all except for the buttons, I'll remove this issue from the milestone.

I think the better approach is to NOT have moveToFront baked into StopwatchNode (especially just in the dragTarget!!!), but instead a client can add a listener to it that moves it to the front when interacted with.

That seems like a nontrivial issue which would probably need to be discussed with the dev team, and may go against parts of the decision in phetsims/scenery-phet#608 (comment). Up to @jonathanolson if we should open an issue for that.

[jonathanolson]

Or, presumably we'd want all StopwatchNodes to moveToFront with the buttons, so StopwatchNode could create a listener on ALL of itself?

[samreid]

This patch seems to be working OK, is it what you had in mind?

Index: js/StopwatchNode.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/StopwatchNode.js b/js/StopwatchNode.js
--- a/js/StopwatchNode.js	(revision 18007999e489b1f4709ac13fb18695329d80b9d4)
+++ b/js/StopwatchNode.js	(date 1611963761694)
@@ -159,7 +159,7 @@
     const stopwatchVisibleListener = visible => {
       this.visible = visible;
       if ( visible ) {
-        this.moveToFront();
+        // this.moveToFront();
       }
       else {
 
@@ -205,7 +205,7 @@
       // The StopwatchNode always calls moveToFront on drag start
       const startOption = dragListenerOptions.start;
       dragListenerOptions.start = () => {
-        this.moveToFront();
+        // this.moveToFront();
         startOption && startOption();
       };
 
@@ -238,6 +238,10 @@
     // @private
     this.numberDisplay = numberDisplay;
 
+    this.addInputListener( {
+      down: () => this.moveToFront()
+    } );
+
     // support for binder documentation, stripped out in builds and only runs when ?binder is specified
     assert && phet.chipper.queryParameters.binder && InstanceRegistry.registerDataURL( 'scenery-phet', 'StopwatchNode', this );
   }
@@ -293,7 +297,7 @@
    * @param {Object} [options]
    * @returns {function(time:number):string} - see NumberDisplay options.numberFormatter
    */
-  static createRichTextNumberFormatter( options ){
+  static createRichTextNumberFormatter( options ) {
 
     options = merge( {
 

[samreid]

I noticed the patch doesn't pop it to the front when dragging out of the toolbox though.

[jonathanolson]

I noticed the patch doesn't pop it to the front when dragging out of the toolbox though.

Presumably we'd want to handle touch-snag and the toolbox pattern! Having a DragListener with attach:false would handle the first, however the second would almost require information from the actual DragListener being triggered, right?

[samreid]

I'll move the latter part of the conversation to a new issue in scenery-phet.

[samreid]

Since this issue is non-blocking for Waves Intro, I'm unassigning. We can continue in phetsims/scenery-phet#659 as appropriate