It's possible to set dropper.positionProperty outside range

[Nancy-Salpepi]

Test device
MacBoo Air (m1 chip)

Operating System
13.1

Browser
safari

Problem description
For phetsims/qa#872 and phetsims/qa#873, it is possible to move the dropper outside of range. Once I click on the dropper, it snaps back into place.

Steps to reproduce
Here is an example from pH Scale Basics:

1. Go to phScaleBasics.macroScreen.model.dropper.positionProperty
2. Press Get Value
3. Change the Value of Y to 150
4. Click on the dropper

Visuals

dropperlocation.mp4

[arouinfar]

Thanks @Nancy-Salpepi.

This is also true for pHMeter.probe.positionProperty. Normally, the pH probe is bounded to the sim's layoutBounds. You can see this when running the sim with ?dev and dragging the probe to the edge of the screen. The probe will stop when it reaches the edge of the layoutBounds (red line). If you use setValue to set the position outside of this boundary and then click on the probe, it will jump back to the edge of the layoutBounds. It seems like the layoutBounds do not depend at all on the window size, so maybe we can enforce a range on the positionProperty? Even if we cannot, I am not overly concerned. The sim will not crash if the positionProperty is outside of the visible bounds of the sim, and it can always be retrieved by returning to the previous position or using Reset All.

In the case of the pH probe, I think it is valuable for clients to be able to reposition it with an API command. This would allow clients to move the probe into/out of the beaker in their wrapper. I do not think the same holds for dropper.positionProperty. There is no pedagogical value in being able to move the dropper, and we can add this issue to the list of reasons why having a movable dropper is more trouble than its worth.

@pixelzoom here are my recommendations

• Set dropper.positionProperty to phetioReadOnly: true. There is no reason for clients to have API control over this.
• If possible, limit pHMeter.probe.positionProperty to the layoutBounds.

[pixelzoom]

• Set dropper.positionProperty to phetioReadOnly: true. There is no reason for clients to have API control over this.

Easy. But I'd much rather make the dropper have a static position, to put an end to the problems that dropper.positionProperty has caused. @arouinfar how do you want to proceed?

• If possible, limit pHMeter.probe.positionProperty to the layoutBounds.

Not going to happen -- it's too much work, and does not address this problem generally. As I said recently for Beer's Law Lab... I'm of the opinion that when there are drag constraints, positionProperty should be phetioReadOnly: true, and the position should be customized by using the sims. Setting positionProperty via an API call is not something that anyone has asked for, and trying to constrain the position to some bounds is a programming (and documentation) "can of worms". @arouinfar do you want me to change pHMeter.probe.positionProperty to phetioReadOnly: true, or do you want to leave it as is and document the drag bounds in the Client Guide?

[pixelzoom]

Another thing to note about dropper.positionProperty is that only the x coordiante of the position should be changed, because dragging is constrained to be horizontal. So someone setting dropper.positionProperty through the PhET-iO API would need to know that only x can be changed, and they would need to know the allowable range of x.

[pixelzoom]

My recommendations are:

• Remove the ability to move the dropper. If that's OK, then please go to Studio, place the dropper where you'd like it to appear, and tell me the value of dropper.positionProperty.

• Make no code changes for pHMeter.probe.positionProperty. If you feel that's it's necessary (which I do not), document that pointProperty.value.x is constrained to [0, 1100], and y is constrained to [0, 700]. Note that this sim has non-default layoutBounds, so it's possible that those constraints could change in the future -- which is a good reason NOT to document them outside of the code.

[arouinfar]

Remove the ability to move the dropper. If that's OK, then please go to Studio, place the dropper where you'd like it to appear, and tell me the value of dropper.positionProperty.

Yes, let's do it. We can use the default initial position { "x": 700, "y": 265 } for the permanent position.

Make no code changes for pHMeter.probe.positionProperty. If you feel that's it's necessary (which I do not), document that pointProperty.value.x is constrained to [0, 1100], and y is constrained to [0, 700]. Note that this sim has non-default layoutBounds, so it's possible that those constraints could change in the future -- which is a good reason NOT to document them outside of the code.

Sounds reasonable to me. I see value in clients having control of the probe position, but I don't think we need to add documentation that is bound to get stale. Clients can easily get the coordinates from Studio for use in their wrappers later.

[arouinfar]

For future reference, here's a list of the many issues related to having a movable dropper.

• dropper: disable dragging without disabling button #141
• how to disable input listeners scenery#1041
• How does inputEnabledProperty apply to dropperNode? #215
• CT: Assertion failed: Attempted to attach to an already attached pointer #250
• Dropper does not need to be movable for alt input. #252
• Disabling Dropper dragListener in version 1.5 results in disabled Dropper Button in version 1.6 #263

There are probably more that I missed, but this is a pretty good overview of complexities.

[pixelzoom]

Since #271 went so poorly due to migration problems, I'm waiting to move forward on this issue until all migration-related issues are resolved and patched.

[pixelzoom]

I'll address this in master, @samreid @zepumph will cherry-pick.

[pixelzoom]

Addressed in master in the above commits. Summary:

• deleted model.dropper.positionProperty and view.dropperNode.dragListener
• added rules to ph-scale-migration-rules.ts and ph-scale-basics-migration-rules.ts
• regenerated ph-scale-phet-io-api.json and ph-scale-basics-phet-io-api.json

Ready for next steps by @samreid @zepumph.

[samreid]

@zepumph and I cherry picked the changes and confirmed they work well. Would be good for @Nancy-Salpepi to double check following the directions in the top comment.

[zepumph]

Position is not draggable in ph-scale and basics 1.6.0-rc.2. Ready to close after QA verification.

[Nancy-Salpepi]

The dropper is no longer draggable in pH Scale and pH Scale Basics rc.3 (model.dropper.positionProperty and view.dropperNode.dragListener have been removed from the tree). Closing!

[KatieWoe]

Should the position property still be visible due to this from Amy?

Sounds reasonable to me. I see value in clients having control of the probe position, but I don't think we need to add documentation that is bound to get stale. Clients can easily get the coordinates from Studio for use in their wrappers later.

If I'm misunderstanding please close again.

[arouinfar]

Thanks for checking @KatieWoe. The comment you quoted was in reference to phScale.macroScreen.model.pHMeter.probe.positionProperty (not the dropper). I've confirmed it still exists in rc.3, closing.