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

Pickability and handling listeners #1150

Open
jonathanolson opened this issue Jan 28, 2021 · 2 comments
Open

Pickability and handling listeners #1150

jonathanolson opened this issue Jan 28, 2021 · 2 comments
Assignees

Comments

@jonathanolson
Copy link
Contributor

Raised from phetsims/sun#676.

During dev meeting, it would be good to discuss the current approach to pickability, I'll give a refresher for how it works, and what we can do.

This has appeared as a problem for components where:

  1. We develop a component that has default pickability on the foreground. Because there are no input listeners covering the whole component, background subcomponents get input events.
  2. A listener is added to the entire component, so now the foreground intercepts the events and seems "buggy"

Having the non-interactive foreground set to pickable:false prevents this.

@zepumph
Copy link
Member

zepumph commented Feb 25, 2021

Note that @jonathanolson and I looked at some pickable usages in our project over in #1116 (comment) trying to find spots that should be using Node.inputEnabledProperty instead. Here are our notes:

There are 303 usages of pickable:false (and more with pickable = false, many of which likely are just trying to prevent input, and not represent a transparent-to-input Node

  • @jonathanolson's thoughts: You definitely want to set pickable on "foreground" components.

  • "Do I need to pick things through it"

  • "Do I want to prevent hit test calculation on children.

  • pickable: false was largely used correctly, on items that were never going to get input, but .pickable = is more dynamic. AreaBuilderGameView.challengeLayer sets pickable dynamically (between false and null) where it could likely get away with setting inputEnabled instead.

  • canStartPress: was another spot that demonstrated that you COULD potentially use Node.setInputEnabled (so long as all listeners on that Node would apply, since canStartPress is just for a single input listener. I.E. ChargedParticleNode.js

  • Likely the only work to be done here (retroactively change usages) is on PhET-iO sims using instrumented Pickable.

@pixelzoom
Copy link
Contributor

pixelzoom commented Feb 25, 2021

2/25/21 dev meeting:

@jonathanolson summarized how pickable works. He mentioned the possibility that pickable could be converted to a boolean. But we'd need to do some work to set pickable: false for things that are currently pickable: null.

@jbphet preferred leaving things the way they are, based on the work that would be required. Wondered if that's naive.

Consensus:

  • leave pickable as is (3 values)
  • no query parameter to add input listeners to root or ScreenViews
  • could do snapshot comparison on sun and scenery-phet demos, compare with and without a query parameter that adds input listeners
  • @jonathanolson will manually do snapshot comparison once to identify problems with common-code components. He'll add a query parameter (flag) called rootInputListener.

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

No branches or pull requests

3 participants