Interrupting touch dragHandlers by setting a sim inactive breaks the drag

[phet-steele]

This can be done in a phet-io "Active" or "Instance Proxies" wrapper, hence the phet-io label, but more importantly this is a breaking feature for our Legends of Learning work with build-an-atom (see phetsims/tasks/issues/851). Also, it appears to only be a touch input problem.

Given the purpose of LoL, it is not unreasonable to imagine that a teacher would "pause" a sim as a student is dragging an object around. This scenario can be reproduced using the "Pause in 5 seconds" button:

1. Follow the steps in Test Build an Atom 1.2.6-rc.2 tasks#851 to start the sim in the LoL environment.
2. Touch "Pause in 5 seconds"
3. Within the next 5 seconds, start dragging a particle around. Don't let go before the sim is set inactive.
4. Once the sim is inactive, touch "Resume".

What you were dragging will be stuck, unable to return to the bucket, and unable to be grabbed again. Reset All will fix this by returning the particle to its bucket. Beyond that, the particle will still have problems when retrieved from the bucket again. The only true fix is a page refresh.

Tagging @samreid and @jessegreenberg to be aware that this a major issue for LoL, and tagging @jonathanolson preemptively since this might require a scenery change. Seen on iPad Air 2 iOS 10.3.2.

[jonathanolson]

Instead of properly handling interruption, is it sufficient to queue up input events when paused, then play them back?

[samreid]

That sounds like it would be good enough to me. I can imagine cases where odd things would happen when the events catch up, but I'm not too excited about retrofitting 30+ old sims to handle event dropping.

[samreid]

@phet-steele mentioned that Build an Atom testing is getting close enough to completion that we can start looking into this issue. @jonathanolson can we schedule a time to work on this?

[jonathanolson]

The workaround consists of:

Changes to Sim:

• don't set display.interactive
• Connect activeProperty to display._input's activeProperty

Changes to Input:

• Add activeProperty
• When activeProperty changes to true, fireBatchedEvents()
• Check for activeProperty where batchDOMEvents is checked currently.

[samreid]

@jonathanolson and I worked together on adding an interrupt feature to the legacy scenery events. It would be great if @pixelzoom is available to review this. @ariel-phet can you check whether that is possible and help prioritize? If @pixelzoom won't be available to review this soon, can you recommend another reviewer?

[pixelzoom]

Self assigning per @ariel-phet . 30 minutes tops to review, but I'll need an iPad, which I don't have with me today.

[pixelzoom]

@samreid Are the steps that @phet-steele described in #619 (comment) still sufficient for testing this? If not, please provide details.

[pixelzoom]

I asked about whether I needed an iPad to test this, and @phet-steele provided some info via Skype:

[6/13/17, 1:01:11 PM] Steele Dalton: don’t need multitouch if you use the “Pause in 5 seconds” button. However, you still need a touch screen
[6/13/17, 1:03:42 PM] Chris Malley: So that I understand... I need a touch screen because the problem occurs only for touch events?
[6/13/17, 1:04:07 PM] Steele Dalton: as far as I know, yes

But looking at the commits about, I don't see anything obvious to indicate that this is a touch-only problem, or that the fix applies to touch only.

So... I'm going to defer reviewing this until someone (@samreid? @jonathanolson?) clarifies what the problem actually was, what I'm supposed to review, and how this should be tested.

[samreid]

The steps described by @phet-steele in #619 (comment) are one way to test the changes. Joist/Sim.js also uses the input interruption feature when switching screens, so you could test it by using an iPad, starting a drag sequence then switching screens while still dragging.

@jonathanolson also suggested that @phet-steele may be able to look up some older multi-touch bug reports that may be addressed by this.

[jonathanolson]

But looking at the commits about, I don't see anything obvious to indicate that this is a touch-only problem, or that the fix applies to touch only.

It's not exclusively a touch-only issue, but it almost always can only be triggered by touch. The only reason in this case that it can be triggered with a mouse is because the test harness allows a delayed action (pause in 5 seconds), which allows it to be triggered. Usually if it's just a sim on its own, you can't trigger delayed actions like that, and thus for most cases it is touch-only.

If you're testing anything except for the legends of learning test harness, you would presumably need a device capable of multitouch.

[samreid]

@pixelzoom this problem pertains to a large class of multi-touch problems, the main issues is not only to test that it works but more generally to investigate the implementation strategy we chose for interruptable input events using legacy scenery listeners. We were hoping to get a code review from you and for you to evaluate the strategy and think about corner cases it might fail on.

[phet-steele]

The only reason in this case that it can be triggered with a mouse is because the test harness allows a delayed action (pause in 5 seconds)

Using a mouse does not show the same symptoms. The controlled object can be recovered by moving the cursor back into the sim. Touch, on the other hand, is un-recoverable. This is the reason I deemed it "touch only".

Mouse:

[jonathanolson]

I'll grab people to discuss during the next core hours.

[jonathanolson]

Current best option:

When paused:

• Interrupt listeners
• Remove all Touch pointers from the pointers array (sending exit events as necessary)
• Fully ignore all DOM input events (nice and simple)

When unpaused:

• Don't need to do anything

Other changes:

• If an unknown touch comes in (a touch ID that we don't have recorded, because we ignored events or removed it earlier), we just ignore that event. Require a user to release a finger and re-press (if their finger was down during the paused section).
• Optionally, dynamically create touches on things like touchMove, so that the user would just need to move a finger after pausing, rather than re-press.

[jonathanolson]

Implemented fix. @phet-steele, can you verify that the "sticking" of particles now doesn't happen in the harness with pausing?

[phet-steele]

Talked with @jonathanolson yesterday. Most browsers agree with this fix....except Edge. JO can probably speak to this more, but there are some cases in Edge where the sim breaks when we do not use event.preventDefault. It looks like there are some user gestures that work when the harness is paused (like swipe right to go to the previous page, but stop halfway and swipe back to the left to stay on the current page) that causes problems in Edge. Other browsers are fine and work as intended.

[jonathanolson]

Reproducing with touch on a Surface Pro 3. Local urls (http://10.10.X.X) and file:// aren't working in Edge with the harness, so I'll be SCPing test versions to spot.

[jonathanolson]

@ariel-phet, I've spent enough time on this where I'd prefer to bail on handling the Edge-specific nature for Legends of Learning, and potentially sometime look at the larger Edge bugginess (see details in #645).

Currently I'd guess 2 more days of my time on this would have a 40% chance of resolving this issue.

[ariel-phet]

@jonathanolson - I think anyone using Edge "deserves" to get some strange behavior :).

Considering our usage stats (with chrome as by far the dominant browser), I am fine with you bailing.