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

exitEvents should Fire for all touch like Pointers #1156

Closed
3 tasks done
jessegreenberg opened this issue Feb 16, 2021 · 17 comments
Closed
3 tasks done

exitEvents should Fire for all touch like Pointers #1156

jessegreenberg opened this issue Feb 16, 2021 · 17 comments

Comments

@jessegreenberg
Copy link
Contributor

jessegreenberg commented Feb 16, 2021

From phetsims/ratio-and-proportion#352 we found that Input.exitEvents is currently not called for Pointers of type Pen. One result of this is PressListener overPointers are not updated, So things may still think they have a Pointer over them when they do not.

For example, see dragging a stylus across the navigation bar screen buttons - they look highlighted even when the stylus leaves the button.
https://user-images.githubusercontent.com/60749003/108002416-cbd93580-6fac-11eb-9388-c629864243dc.gif

In a meeting with @jonathanolson and @zepumph we identified that these exit events are specifically prevented for all pointers other than Touch

    // touch pointers are transient, so fire exit/out to the trail afterwards
    if ( pointer instanceof Touch ) {
      this.exitEvents( pointer, event, eventTrail, 0, true );
    }

We came up with a game plan to address this:

  • Add to Pointer a method called isTouchLike, to be ovderridden by subtypes - returns true for Touch and Pen
  • Go through and review usages of pointer instanceof Touch and probably replace with isTouchLike.
  • Confirm with designers that touch areas can be used for pen input (stylus)

This has likely been an issue for some time, so I don't know if it is blocking or needs to be cherry-picked into existing RCs. Or if it needs to go out to all sims in a batch maintenance release. Adding blocking label until we learn one way or another.

@zepumph
Copy link
Member

zepumph commented Feb 16, 2021

@arouinfar, do you think that buggy hover behavior like the video in phetsims/ratio-and-proportion#352 should block publication? If so should this be a fix that get's propagated to older versions of the sim? Presumably there would be the potential for incorrect pen hover states in sims for quite some time. We may decide that this is not worth an MR, and instead just fix this on master and for sims moving forward. We think that it is best for a designer to determine this.

@zepumph
Copy link
Member

zepumph commented Feb 17, 2021

I will proceed with the fix on master now.

zepumph added a commit to phetsims/circuit-construction-kit-common that referenced this issue Feb 17, 2021
zepumph added a commit to phetsims/expression-exchange that referenced this issue Feb 17, 2021
zepumph added a commit to phetsims/charges-and-fields that referenced this issue Feb 17, 2021
zepumph added a commit to phetsims/fractions-common that referenced this issue Feb 17, 2021
@zepumph
Copy link
Member

zepumph commented Feb 17, 2021

  • Add to Pointer a method called isTouchLike, to be ovderridden by subtypes - returns true for Touch and Pen
  • Go through and review usages of pointer instanceof Touch and probably replace with isTouchLike.

In the above commits I did these two items. My biggest worries were with ClosestDragListener, and the changes in SimpleDragHandler. @jonathanolson can you please review those two spots. Do my changes make sense to you?

Confirm with designers that touch areas can be used for pen input (stylus)

709262c#diff-19425a8432045348468723394f3f65c8511637a0a5e8dbbded246fb6e718a854R1976 is influenced by this decision, and will need to be changed in pen areas are different from touch areas. @arouinfar will you please tell us if you think pen areas can be the same as touch areas?

  • Should we do more to prohibit usages of instanceof Touch, like a lint rule/warning, because most likely it wants to include Pen in the logic.

@jessegreenberg will you please to a general review and try out this logic with your pen/iPad system? Please make sure that phetsims/ratio-and-proportion#352 is fixed and nothing else seems wrong.

In summary of jobs for this issue:

  • @jonathanolson to look at the specific changes in the above two files mentioned
  • @arouinfar to answer question about pen/touch areas AND if this needs to have an MR.
  • @jessegreenberg to general review and test with his pen.

Thanks all!

@zepumph
Copy link
Member

zepumph commented Feb 17, 2021

isTouchLike() doesn't support a null pointer like instanceof used to, so I committed a fix above.

@arouinfar
Copy link

@arouinfar to answer question about pen/touch areas AND if this needs to have an MR.

Confirm with designers that touch areas can be used for pen input (stylus)

This doesn't seem like something that needs a MR. I don't think we officially support stylus use, and this isn't a sim-breaking issue. Using the touch areas for pen input also sounds reasonable to me.

@jessegreenberg
Copy link
Contributor Author

Changes look good, I don't see any more usages of instanceof Touch in the project. I tested on a iPad with a stylus and the original issue is gone. However, there are cases where I cannot activate a HomeScreenButton with a stylus anymore. I am not sure if it is because of this issue or something else.

@jessegreenberg
Copy link
Contributor Author

The bug was specific to logic in HomeScreenButton for the custom touch over behavior. Should be fixed in the above commit by doing the same work with penover as well. Back to @zepumph to review this change but otherwise things look good.

@jonathanolson
Copy link
Contributor

Should pen input initiate touch snag in these cases if we are considering pen and touch to both be "touch like"? @jonathanolson what do you think?

Touch areas seem like a different feature than touch-snag. I think this is a good question for the designers.

@jessegreenberg
Copy link
Contributor Author

@arouinfar do you have any thoughts about this, should touch-snag happen when using a pen/stylus?

@arouinfar
Copy link

A stylus isn't an officially supported device or something we QA test on. I don't have an opinion about whether it should have touch-snag or not. My recommendation would be to do whatever requires the least amount of dev time/effort.

@jessegreenberg
Copy link
Contributor Author

OK sounds good, between @arouinfar and @jonathanolson comments I think that means the answer to

earching the project for touch[a-z]+: yielded usages in CoinTermCreatorNode and SimpleDragHandler. Should we fix those?

is that we should not change these.

@jonathanolson reassigning back to you to review changes to SimpleDragHandler and ClosestDragListener (from #1156 (comment))

@jonathanolson
Copy link
Contributor

The changes above look good.

Should we do more to prohibit usages of instanceof Touch, like a lint rule/warning, because most likely it wants to include Pen in the logic.

I'm fine without a lint rule here, but @zepumph what do you think?

zepumph added a commit to phetsims/chipper that referenced this issue Feb 26, 2021
@zepumph
Copy link
Member

zepumph commented Feb 26, 2021

Recently @arouinfar said we don't support pen, and with that in mind, we likely won't devote many resources to testing on Pen. Any small thing we can do to improve Pen experience without incurring more cost sounds like a win to me. Is the above commit ok with you? Feel free to close.

@zepumph zepumph assigned jonathanolson and unassigned zepumph Feb 26, 2021
@arouinfar
Copy link

Any small thing we can do to improve Pen experience without incurring more cost sounds like a win to me.

Agreed!

@jonathanolson
Copy link
Contributor

The commit looks good, closing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants