CT failure with PressListener integration into RoundButtonView

[jonathanolson]

Noted in phetsims/sun#336:

ph-scale : xss-fuzz : load
Uncaught Error: Assertion failed: If a custom isPressedProperty is provided, it must be a Property that is false initially
Error: Assertion failed: If a custom isPressedProperty is provided, it must be a Property that is false initially
    at window.assertions.assertFunction (https://bayes.colorado.edu/continuous-testing/snapshot-1520276037206/assert/js/assert.js:22:13)
    at new PressListener (https://bayes.colorado.edu/continuous-testing/snapshot-1520276037206/scenery/js/listeners/PressListener.js?bust=1520276650810:124:15)
    at RoundMomentaryButton.RoundButtonView [as constructor] (https://bayes.colorado.edu/continuous-testing/snapshot-1520276037206/sun/js/buttons/RoundButtonView.js?bust=1520276650810:91:28)
    at new RoundMomentaryButton (https://bayes.colorado.edu/continuous-testing/snapshot-1520276037206/sun/js/buttons/RoundMomentaryButton.js?bust=1520276650810:36:21)
    at PHDropperNode.EyeDropperNode [as constructor] (https://bayes.colorado.edu/continuous-testing/snapshot-1520276037206/scenery-phet/js/EyeDropperNode.js?bust=1520276650810:86:18)
    at new PHDropperNode (https://bayes.colorado.edu/continuous-testing/snapshot-1520276037206/ph-scale/js/common/view/PHDropperNode.js?bust=1520276650810:29:20)
    at new MacroScreenView (https://bayes.colorado.edu/continuous-testing/snapshot-1520276037206/ph-scale/js/macro/view/MacroScreenView.js?bust=1520276650810:52:23)
    at MacroScreen.createView (https://bayes.colorado.edu/continuous-testing/snapshot-1520276037206/ph-scale/js/macro/MacroScreen.js?bust=1520276650810:44:34)
    at MacroScreen.initializeView (https://bayes.colorado.edu/continuous-testing/snapshot-1520276037206/joist/js/Screen.js?bust=1520276650810:242:25)
    at Array.<anonymous> (https://bayes.colorado.edu/continuous-testing/snapshot-1520276037206/joist/js/Sim.js?bust=1520276650810:678:18)
Approximately 3/5/2018, 11:53:57 AM

[jonathanolson]

The easiest way to add a workaround to this failure would cause a slight difference in sim behavior: the dropper button would not appear to be pressed while the dropper is dispensing the initial liquid.

See https://www.colorado.edu/physics/phet/dev/html/ph-scale/1.3.0-dev.3/phet/ph-scale_en_phet.html for the behavior with the workaround, and https://www.colorado.edu/physics/phet/dev/html/ph-scale/1.2.11/ph-scale_en.html for reference behavior.

@ariel-phet, is this an acceptable change, or should we look into modifying our buttons so they can be synthetically pressed with no actual press?

[jonathanolson]

Internally, having a PressListener start as "pressed" leaves it in an invalid state, since any call to things like interrupt/release would try to remove its listener from a pointer (which would be a null reference).

[pixelzoom]

I don't think this issue belongs in ph-scale. It's a general issue with the implementation of PressListener.

Autofill on startup is a required feature of the “Macro” screen in ph-scale. A further requirement is that the momentary button on the eyedropper must be pressed while the dropper is dispensing fluid. See MacroModel startAutoFill, which sets this.dropper.dispensingProperty.set( true ); And dispensingProperty corresponds to the state of the momentary button on the eyedropper.

Other sims are likely to have similar issue, where the state of buttons (and other UI components that can be "pressed") is supposed to reflect the state of the model, regardless of whether the user is interacting with the button. So I don't agree that with the assumption that the initial state of isPressedProperty will be false.

[pixelzoom]

@jonathanolson proposed:

the dropper button would not appear to be pressed while the dropper is dispensing the initial liquid.

No - this is not an acceptable change. It affects not only this sim, but the ability of all sims to have buttons (or anything use that uses PressListener) that reflect the state of the model. The state of buttons should reflect the state of the model, not the state of user interaction.

[jonathanolson]

I'll be looking into it, but I disagree about the concept of the PressListener being a view. It allows you to pass in a Property to be used as a model, but it was meant to be fully in-charge of that model state.

If a more general change is required, it should probably be a separation of the "view" property from the press state of the PressListener.

I will look into the ability to have synthetic external control of any properties passed to PressListener.

[pixelzoom]

I'll be looking into it, but I disagree about the concept of the PressListener being a view. It allows you to pass in a Property to be used as a model, but it was meant to be fully in-charge of that model state.

Requiring PressListener to be "fully in-charge" of isPressedProperty violates the entire spirit of MVC. No one has exclusive control of the model in MVC. And I seriously doubt that this is necessary in PressListener or sun buttons.

[jonathanolson]

Requiring PressListener to be the only controller of isPressedProperty violates the entire spirit of MVC.

If this is true, presumably we shouldn't allow passing in properties to be used. PressListener can just always create isPressedProperty, can mark it read-only (so clients shouldn't use it), and you can link to it? Same for things like DragListener, where instead of the convenience of being able to pass in your isUserControlled property, we should instead have multiple properties and link one of their states to the other?

[pixelzoom]

Why is PressListener even involved in this case, when there is no user interaction? An InputListener should be responsible for handling user interaction. It should not be responsible for the look of the button, and should certainly have no knowledge of the model.

[jonathanolson]

Are you available to join a zoom call about this? It's taking up a lot of time back-and-forth to write down explanations.

It should not be responsible for the look of the button, and should certainly have no knowledge of the model.

ph-scale is passing in a model property to be used as the "tracking whether it is pressed" by the input listener.

[pixelzoom]

PressListener also seems to be responsible for a lot more than "pressing" . I see several things that have nothing to do with pressing: isOverProperty, isHoveringProperty, isHighlightedProperty.

[pixelzoom]

I might be available for a Zoom call later today. Depends on where I'm at with EqEx.

[jonathanolson]

PressListener also seems to be responsible for a lot more than "pressing" . I see several things that have nothing to do with pressing: isOverProperty, isHoveringProperty, isHighlightedProperty.

I'm happy to discuss other names.

[ariel-phet]

I am going to tag this for dev-meeting currently, since it seems like there are some things worth discussing. Feel free to untag if a resolution is reached before then.

[ariel-phet]

@pixelzoom - I think pH scale predates me being on every design team...but I am a bit surprised by the autofill honestly -- not suggesting we change it necessarily, but one of our conventions has been to in general never begin sim with an animation. I believe this paradigm started with Energy Skate Park, when in the original the skater was already on the track and moving. Every other sim I can think of is "begun" with some user interaction. Maybe this autofilling has a reason, but it seems the sim could just start out with 1/2 a liter (sims like molarity, concentration begin this way). So, it seems like there are larger issues around MVC patterns and such in this issue, but I am unclear on the autofill rationale for this sim.

[pixelzoom]

To clarify why I said "this is not an acceptable change" in #71 (comment) ...

First, this change would need to be run past the design team. The current behavior is present in both the Java and HTML5 versions. It was arrived at after discussion, and I recall deciding to keep it during the HTML5 redesign. I personally don't care whether the vessel autofills on startup or not. But if it does autofill, then the dropper behavior (including its button state) should make sense (which is does not in the proposed 1.3.0-dev.3 demonstration). I'm also not excited to revisit the design issue, since this was not a problem until PressListener was integrated.

More importantly... The behavior of ph-scale could indeed be changed to make the CT failure go away. But it does not explain why this new constraint is necessary/desirable, or address the limitations that this constraint will impose on UX design.

[pixelzoom]

Relevant bits from pH Scale HTML5 design document are shown below. Whenever you change solutions in this sim, the beaker auto-fills to 0.50L. So the idea was that it should also auto-fill with the initial solution on startup.

"P" in the "Reason" column stands for "Pedagogical": Changes made to meet the learning goals of the sim, or to improve that student sense-making in interviews around those learning goals, or to better meet with discipline specific practices and conventions

---

Identical to pH scale basics, except that beaker animates auto-fill to 0.50 L upon startup and when solution type is switched.

Feature	Reason	Detailed changes & Rationale
Auto-fill beaker (0.50 L)	P	Students seemed to focus heavily on rates of change of graph values without attending to the amount of solution added from the dropper prior to dilution. Starting with a standard amount allows for more standardized comparisons by both students and teachers. Animation makes it clear what changes when solutions change.

[pixelzoom]

Also note... The beaker auto-fills every time the solution is changed, not just at startup. And the proposal in #71 (comment) prevents the dropper button from looking "pressed" in all cases, not just at startup. So this has very little to do with whether it's appropriate to auto-fill the beaker with the initial solution.

[ariel-phet]

@pixelzoom thanks much for clarifications. I did not realize about he autofill when changing solutions. That behavior makes sense. So even if we did not autofill on startup, this autofill behavior would still be required, good to know. Then I definitely agree that this constraint is not desirable and we should not limit our UX with this constraint if possible.

[ariel-phet]

@jonathanolson kicking back to you. Feel free to punt until dev meeting if you feel that is appropriate and more discussion is warranted.

[jonathanolson]

First, this change would need to be run past the design team. The current behavior is present in both the Java and HTML5 versions. It was arrived at after discussion, and I recall deciding to keep it during the HTML5 redesign.

Keeping the behavior sounds best. I was curious if no one would care about the change.

It sounds like we'll need to decouple the sun buttons' "button model is pressed" (downProperty) from PressListener's "is the user actually pressing" (currently isPressedProperty).

I'd have a few questions about how flexible would this need to be (possibly relevant for the design team):

• If you set downProperty to false during an actual user press, would this interrupt the actual user press (so they don't receive the "pressing a button" cursor, etc.)?
• If you set downProperty to true, would that need to prevent an actual user press from happening? (synthetic "press" set on the model, THEN user tries to press the button. Does this start an actual press? If the user releases before a synthetic "release" would happen, does it actually become unselected?) Dropper behavior seems to be "let the actual press happen, and actual release sets downProperty to false" (try clicking the dropper while it is autofilling).
• @samreid, for phet-io is it OK for a "model" event that starts a press to be paired with a "user" event that releases a press? Is it OK to have a "model" press followed by a "user" press? Is it OK for a "model" press to have no corresponding "model" release? If the downProperty is true at the start, would we want a "model" press to be triggered on listener creation (otherwise you will have a "release" event before any "press", is that OK?). We should use FireListener instead of SUN/buttons/ButtonListener sun#336 may be relevant.

I see a few current approaches to this:

1. Have the sun buttons handle both properties (downProperty and isPressedProperty), including setting up the forwarding/behavior (so that isPressedProperty going to true may, depending on the above questions, trigger downProperty being set to true). This sounds preferential to me, as I don't like the general pattern of separation, and it seems only necessary for things that use ButtonModel.
2. Don't allow passing in things like isPressedProperty to PressListener. PressListener creates and exposes its own property, and you can link to it if you want to know about the "actual" press information. Can also be done with (1).
3. Force (some of) the public properties of PressListener to be arbitrarily writable by outside clients. Most likely, this will end up in the implementation having to have separate "public" and "private" versions of each Property. This is what I believe was recommended by @pixelzoom. I'm generally against this unless I hear that lots of custom buttons/etc. will need "synthetic" presses/releases with whatever common behavior we describe.
4. Something else?

@pixelzoom, @samreid and @jbphet, thoughts?

[samreid]

If you set downProperty to false during an actual user press, would this interrupt the actual user press (so they don't receive the "pressing a button" cursor, etc.)?

Yes, the model should be able to programmatically trigger the release of the button. I can imagine this happening when certain conditions in the model are met (such as the solution being saturated, etc). I suspect that should change the mouse cursor back to normal (though I'm not sure how important that is).

If you set downProperty to true, would that need to prevent an actual user press from happening?

I'm not sure, I think we could construct cases either way (sometimes the mouse should be able to "take over" from the model, but sometimes the model should remain in control).

@samreid, for phet-io is it OK for a "model" event that starts a press to be paired with a "user" event that releases a press?

It would be unconventional to pair a model start with a user end, but I think as long as researchers know that's possible, they could parse it correctly.

If the downProperty is true at the start, would we want a "model" press to be triggered on listener creation (otherwise you will have a "release" event before any "press", is that OK?)

So far we haven't signified property initial values on the data stream (thinking we can get them from the state stream), but maybe one day we'll have to investigate that. It would be odd for a release before a press, but as long as researchers know that's possible, I'm sure they could deal with it.

I see a few current approaches to this:

To clarify, it seems to me that isPressedProperty is referring to whether the item was pressed by a mouse/touch (and not pressed by the model). I suspect one day we'll run into corner cases where a sim will need to know that value, so it may need to be public whether declared in the button or in the pressListener. I read through the approaches, but I'm not sure of the best implementation.

[jessegreenberg]

Hmm, not ready for review. This change caused problems with CT, ill investigate.

[jessegreenberg]

Same change added back, the issue was with disposing Multilink and moved to phetsims/axon#192. Back to @jonathanolson for review.

[pixelzoom]

What's the status of this issue? Labeling for developer meeting.

[jonathanolson]

I'm reviewing now.

[jonathanolson]

So it appears there were cases where createListener() was called multiple times on a single button model, but do we need to handle the case where we need to remove any? (Hopefully don't need to handle it)

Changes look good to me.

[jonathanolson]

Also had an above commit that did a bit of cleanup, can you do a brief review?

[jessegreenberg]

Refactoring in phetsims/sun@b0cbd63 looks nice, thanks.

So it appears there were cases where createListener() was called multiple times on a single button model,

No, the problem in phetsims/axon#192 was that listeners on dependencies of a multilink could still be called after the multilink had been disposed. Not that createListener was called multiple times on a button model. Since that is the case, can this be closed?

[pixelzoom]

Assigning back to myself to test ph-scale.

[pixelzoom]

ph-scale tested, look good, closing.