search

phetsims / twixt

Animation library for interactive HTML5 graphics
MIT License
1 stars 3 forks source link

OpacityTo shouldn't modify visibility of target Node #4

Closed pixelzoom closed 6 years ago

pixelzoom commented 6 years ago

In Slack developer channel, @jonathanolson said that OpacityTo was the root cause of https://github.com/phetsims/capacitor-lab-basics/issues/228, specifically how it makes the target Node visible on start. See line 52:

50      .onStart( function() {
51        node.opacity = parameters.opacity;
52        node.visible = true;
53        options.onStart && options.onStart();
54      } )

I don't understand the capacitor-lab-basics issue, or why you would run OpacityTo using an invisible Node. That said...

OpacityTo is used in these sims:

capacitor-lab-basics equality-explorer function-builder

I identified at least one case (function-builder FBScreenView) that is relying on this behavior. I'm going to defensively fix that, and any others that I can identify.

pixelzoom commented 6 years ago

I adjusted function-builder, and verified that equality-explorer isn't relying on OpacityTo to make the target Node visible. Before removing line 52 (node.visible = true) someone needs to verify that this doesn't cause problems for capacitor-lab-basics. Since phetsims/capacitor-lab-basics#228 motivated this issue and is assigned to @jonathanolson, I'm assigning this issue to him to complete.

pixelzoom commented 6 years ago

Labeling as high priority since we should change OpacityTo before addition usages are created that might rely on node.visible = true.

pixelzoom commented 6 years ago

OpacityTo is now used in equality-explorer, which is soon to be published. So as I recommended in https://github.com/phetsims/twixt/issues/4#issuecomment-361776968, I have removed node.visible = true from OpacityTo, so that I'm not creating more dependencies on this quirk.

This may or may not break capacitor-lab, so it would be good for @jonathanolson to verify with "top" priority.

jonathanolson commented 6 years ago

Looks good in CLB, thanks!