Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should fireOnHold be set to True for the Number Spinners? #320

Closed
Tracked by #1121
Nancy-Salpepi opened this issue Jul 10, 2024 · 9 comments
Closed
Tracked by #1121

Should fireOnHold be set to True for the Number Spinners? #320

Nancy-Salpepi opened this issue Jul 10, 2024 · 9 comments
Labels
status:ready-for-review type:question Further information is requested

Comments

@Nancy-Salpepi
Copy link

Test device
MacBook Air M1 chip and Dell

Operating System
macOS14.5 and Win11

Browser
Safari and Chrome

Problem description
For phetsims/qa#1105, I currently have to continually press the increment/decrement buttons with the mouse to change the Number of Cups/People/Balls, which is different behavior than in the prototype and with the keyboard.

@amanda-phet do you like the current mouse behavior or should it be changed?

@Nancy-Salpepi Nancy-Salpepi added the type:question Further information is requested label Jul 10, 2024
@marlitas
Copy link
Contributor

I think this might have been a multi-touch issue... @jbphet can confirm

@amanda-phet
Copy link
Contributor

I just chatted with @jbphet about this and it does sound like it was maybe a multi-touch issue, and I vaguely remember now saying it's fine if you can't press and hold because it's only 7 clicks max. However, he will look into it tomorrow and if it's not too hard to fix we'll allow fireOnHold!

@jbphet
Copy link
Contributor

jbphet commented Jul 18, 2024

@amanda-phet and I discussed this in person and decided that making the "Kick" button work for press-and-hold should also be included in the scope of this issue.

@jbphet
Copy link
Contributor

jbphet commented Jul 18, 2024

Okay, the spinners for number of cups/plates/balls now works with the press-and-hold behavior. I removed the interruptSubtreeInput calls that were preventing this from working and added some more targeted ones. I tested the basic, mouse-only cases as well as some multi-touch cases and fixed up any problems I found. I think this is now ready for code review, and we should definitely leave it open and have QA test it as an "issue to verify" in the next round of testing.

@jbphet jbphet assigned marlitas and unassigned jbphet Jul 18, 2024
@marlitas
Copy link
Contributor

Sending this back to @jbphet since we discovered that the kick button is still getting interrupted during standup.

@jbphet
Copy link
Contributor

jbphet commented Jul 19, 2024

Yes indeed, I messed this up at the last moment. I added some code that was intended to interrupt input on the "Kick" button if the user tried to interact with it and the "Number of Balls" spinner at the same time. My code was flawed though, and I won't get into the details as to why, but I've now backed it out.

The code that was added for the other direction, meaning where interaction with the "Kick" button interrupts input to the "Number of Balls" spinner, seems to be working fine. I've tried testing a number of multi-touch scenarios where I tried to interact with both controls at the same time and wasn't able to create any problematic behavior in the sim, so I'm now thinking that this is sufficient to allow fire-and-hold to work on both of these controls without introducing multi-touch vulnerabilities.

@marlitas - please have another look and see if the behavior now looks good and if the code changes seem reasonable.

@jbphet jbphet assigned marlitas and unassigned jbphet Jul 19, 2024
@marlitas
Copy link
Contributor

I looked at the code and this seems to be handling situations effectively. I also tested a few scenarios on my phone and it's looking good. This is ready to close on my end.

Over to @Nancy-Salpepi or @KatieWoe to confirm the behavior is looking good on main. Feel free to close!

@KatieWoe
Copy link

Things look ok on main to me.

@Nancy-Salpepi
Copy link
Author

Looks good in rc.1! Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:ready-for-review type:question Further information is requested
Projects
None yet
Development

No branches or pull requests

5 participants