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

Disabling Dropper dragListener in version 1.5 results in disabled Dropper Button in version 1.6 #263

Closed
Nancy-Salpepi opened this issue Dec 21, 2022 · 6 comments

Comments

@Nancy-Salpepi
Copy link

Nancy-Salpepi commented Dec 21, 2022

Test device
MacBook Air (m1 chip)

Operating System
13.0.1

Browser
safari 16.1

Problem description
For phetsims/qa#866 and phetsims/qa#867, in Studio version 1.5 if I set view.dropperNode.dragListener.enabledProperty to false --the dropper can't be moved from side to side, but the button still works and liquid can be dispensed. Loading this version into Studio 1.6, results in the dropper button being disabled as well. I see there are changes to that portion of the tree, but wanted to make sure that was the intended outcome OR if in this case, it should have reverted to the default setting?

Screenshot 2022-12-21 at 9 03 14 AM

Steps to reproduce
In pH Scale:

  1. In Studio version 1.5, select "all" for PhET-iO elements
  2. On either the Macro or Micro screens, set view.dropperNode.dragListener.enabledProperty to false
  3. Save version
  4. Upload into Studio version 1.6
  5. Try to dispense liquid
@pixelzoom
Copy link
Contributor

pixelzoom commented Dec 21, 2022

What, another problem with dropper interactions? How suprising :)

There's no pedagogical reason for making the dropper movable. We did it to make interacting with the dropper move "fun". But it has caused nothing but problems (and continuing cost) in both the Java and HTML5 versions. An incomplete list of problems includes: How to keep the drag handler from swallowing button events, how to hold down the button while dragging the dropper, how to design alt input (we punted), ... and now, how to workaround PhET-iO problems.

So before I incur yet-another cost related to this... Can we please revisit whether the dropper needs to be movable? @arouinfar thoughts?

@pixelzoom
Copy link
Contributor

Also note that in #252 (alt input), we decided that moving the dropper via alt input is not necessary. In PHDropperNode.ts:

    // NOTE: Moving the dropper via the keyboard is not necessary.
    // See https://github.com/phetsims/ph-scale/issues/252

If it's not necessary for keyboard, then it's presumably not necessary for mouse & touch either.

pixelzoom added a commit to phetsims/scenery-phet that referenced this issue Dec 21, 2022
@pixelzoom
Copy link
Contributor

pixelzoom commented Dec 21, 2022

Reproduced in master.

Investigating more, and re-reading @Nancy-Salpepi's report:

in Studio version 1.5 if I set view.dropperNode.dragListener.enabledProperty to false --the dropper can't be moved from side to side, but the button still works and liquid can be dispensed.

This smells like a bad migration rule, and here it is:

    new TandemFragmentRenamed( 'dropperNode.dragListener.enabledProperty', 'dropperNode.inputEnabledProperty' ),

The original Property disabled only the button's dragListener. This rule maps that to disabling all input for the entire dropper, including the push button. This definitely needs to be addressed, regardless of whether the dropper remains movable. @samreid @zepumph FYI.

Also note that if do we remove dragging from the dropper, that will change the PhET-iO API, and require one or more migration rules. So maybe we want to leave it as is.

@pixelzoom
Copy link
Contributor

pixelzoom commented Dec 21, 2022

@arouinfar nevermind. We're keeping the dropper movable. I can't bear to deal with making it unmovable at this point in the release process. If you'd like to reconsider that in a future release, please create an issue.

In the above commits, I instrumented dropperNode.dragListener.enabledProperty (which appears in 2 places in the Studio tree), so that element is present in both the 1.5 and 1.6 API. Then I deleted the bogus TandemFragmentRenamed rule from the migration-rules.ts files.

This example illustrates several pitfalls of migration rules:
(1) Rules may succeed but still totally break the sim. Testing is required.
(2) "Common" rules are not necessarily applicable to all situations, and should be individually considered, not bulk/blindy copy-pasted.
(3) Having developers who are not familiar with the sim write migration rules is liable to break things in subtle ways.

@pixelzoom
Copy link
Contributor

@Nancy-Salpepi ready for review in master. If it looks OK, please close.

To test:

  • dropperNode.dragListener.enabledProperty should enabled/disable only dragging for the dropper
  • dropperNode.inputEnabledProperty should enable/disable all input for the dropper

There are 2 occurrences of each of the above PhET-iO elements in ph-scale, 1 occurence in ph-scale-basics.

@Nancy-Salpepi
Copy link
Author

Looks good in all occurrences in master. 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

3 participants