Set fireOnHold to true

[marlitas]

This will track the commit for allowing keyboard listener to fire when a key is held down.

[marlitas]

This was a straightforward code change, but I do want to make sure it didn't introduce any craziness in behavior or interactions we're not thinking of so I would like to keep this open for the next QA test.

Over to @catherinecarter first.

[catherinecarter]

Seems to be working perfectly when I tested it. Nice suggestion, @Nancy-Salpepi!

[marlitas]

I'll unassign @Nancy-Salpepi for now, and just flag this as an issue that we'll want to reference in the next dev test.

[Nancy-Salpepi]

For phetsims/qa#985:
When pressing down the arrow key to move a card, the card speeds up drastically when it gets closer to either end. This looks especially odd when zoomed in.
Here is the non-zoomed version:

fireonholdcards.mp4

[marlitas]

I would love to get @catherinecarter's opinion on the priority of this problem. The interactions in the cards are rather complicated especially when it comes to keyboard interaction. @catherinecarter how much time would you like me to spend investigating this? I have a feeling this could go down a rabbit hole and I don't know how big of a concern it is right now.

If it is a large concern, the other option is to remove auto-firing when a keypress is held down for the cards.

[catherinecarter]

I agree that this is distracting and seems strange that the card would gain speed when moving towards the end. Not sure why that's happening.

I think spending maybe 15 minutes investigating would make sense, but not much more than that. I worry that changing anything in the cards could inspire a domino effect and create more issues down the road, especially since the code is complicated as is.

If earlier in the process, I think spending more time would make sense to fix it, but since we're trying to get this published, I'm ok accepting the behavior as is if it takes longer than 15 minutes or would create more problems by fixing.

Does this sound reasonable? Thanks!

[marlitas]

@catherinecarter, very reasonable. I'll take a look today and report back.

[KatieWoe]

Noting that the sped up movement does not seem to happen when moving soccerballs.

[marlitas]

My first area of investigation was to see how frequently swapCards() was firing. This seems to be at a consistent rate when fireOnHold is activated.

[marlitas]

My next theory was that because the amount of displacedCards goes down the closer we get to the end, the less we have to iterate over and animate. However logging did not seem to indicate this was causing the hold up at the beginning/ speed up at the end.

And having displacedCards go directly home instead of animateToHomeCell confirmed that this is not the hold up.

displacedCards.forEach( ( card, index ) => {
          card.model.indexProperty.value = displacedIndexes[ index ];
          model.setAtHomeCell( card.model );
          console.log( Date.now() - lastDate, 'displaceCard Animating' );
        } );

[marlitas]

I removed animation from the focused card and that did indeed remove the speed ramping effect.

I created a query parameter ?removeFocusedCardAnimation, that when included will show the behavior that fixes the bug (the focused card will immediately snap to it's homeCell rather than animating to it). The difference looks pretty subtle to me, but I recommend that @catherinecarter take a look and see which she prefers.

This would be good for @Nancy-Salpepi to test and see if there's anything odd from that change as well.

If we would rather remove the animation to fix this bug, let me know and I can remove the query parameter and make setAtHomeCell the default.

Also sending over to @samreid since he did a lot of the animation work here.

[Nancy-Salpepi]

@marlitas with the query parameter the screen no longer pans when moving the card with alt input.

[marlitas]

Ahh that's because I bypassed the animationEndedEmitter. Manually calling the panZoomListener fixes it. That's pushed up.

[marlitas]

We met today and decided we liked it! We're going to remove the animation for the focused card.

[marlitas]

Done above! This was actually really nice because we got to remove some code complexity with animatedPanZoomListener here. I like it.

Over to @Nancy-Salpepi to check on main. Was relatively straightforward so shouldn't need a code review.

[Nancy-Salpepi]

Looks good to me! Closing.