Assertion failed: parallel system is too tall

[phet-steele]

Happens on load (master)

Error: Assertion failed: parallel system is too tall
    at window.assertions.assertFunction (https://bayes.colorado.edu/continuous-testing/snapshot-1508945010642/assert/js/assert.js:22:13)
    at new SystemsScreenView (https://bayes.colorado.edu/continuous-testing/snapshot-1508945010642/hookes-law/js/systems/view/SystemsScreenView.js?bust=1508946234969:70:15)
    at SystemsScreen.createView (https://bayes.colorado.edu/continuous-testing/snapshot-1508945010642/hookes-law/js/systems/SystemsScreen.js?bust=1508946234969:35:34)
    at SystemsScreen.initializeView (https://bayes.colorado.edu/continuous-testing/snapshot-1508945010642/joist/js/Screen.js?bust=1508946234969:223:25)
    at Array.<anonymous> (https://bayes.colorado.edu/continuous-testing/snapshot-1508945010642/joist/js/Sim.js?bust=1508946234969:677:18)
    at https://bayes.colorado.edu/continuous-testing/snapshot-1508945010642/joist/js/Sim.js?bust=1508946234969:685:27

[pixelzoom]

There have been no changes to this sim for a long time, so I suspect that a common code change is the culprit. I first tested with my working copy, which was pulled yesterday - no problem. I then ran pull-all.sh and retested, and reproduced the problem.

pull-all.sh changed these common-code repos in my working copy: axon dot joist scenery scenery-phet sun

One of them is the culprit.

[pixelzoom]

Here's a screenshot that shows what the assertion is detecting - the bottom-left control panel is now outside the layoutBounds.

[pixelzoom]

I asked about this on the Slack developer channel. @zepumph pointed me to a11y changes in HSlider (in sun).

[pixelzoom]

The breaking change was to HSlider in https://github.com/phetsims/sun/commit/f309a64160ae1802ba57a1924cc4b5c25ded6c19. The nature of the change and reason for the break was discussed in Slack:

Chris Malley [11:15 AM] 
why would a11y change the control panel size?
Michael Kauzmann [11:16 AM] 
@jessegreenberg and I updated that way that HSlider's focusHighlight works, so that it is actually a node in an HSlider, therefore the bounds are calculated around it, and the focusHighlight goes outside the old bounds.
Chris Malley [11:17 AM] 
not cool.
Michael Kauzmann [11:17 AM] 
I'll revert and we will figure something else out.
Chris Malley [11:18 AM] 
anytime you change default options, or especially do something that can affect bounds, in common code, a full regression test is needed.
Michael Kauzmann [11:18 AM] 
Sounds good. Sorry about that!
Jesse Greenberg [11:19 AM] 
Yes, sorry about that, we will come up with something else.
Michael Kauzmann [11:19 AM] 
pull sun and confirm fix?
Chris Malley [11:20 AM] 
Ah… So this affected the bounds of all HSliders?
Michael Kauzmann [11:20 AM] 
yes
Chris Malley [11:20 AM] 
yikes
Michael Kauzmann [11:20 AM] 
in retrospect very dangerous indeed.

[pixelzoom]

Apparently reverting HSlider didn't fix it. The control panels are now less tall, but still taller than they were as some point yesterday.

[pixelzoom]

Breaking changes were also made to NumberControl. Relevant Slack discussion:

Michael Kauzmann [11:25 AM] 
We added a node to NumberControl too. Reverting
Chris Malley [11:26 AM] 
Was this pattern applied to any other controls?  I want to make sure we’re not fixing 1 of N problems.
Michael Kauzmann [11:27 AM] 
I don't believe so. This was a very custom case, most of the time focusHighlight is handled in an overlay, and has nothing to do with the node.

[zepumph]

Issue that breaking changes were made for: https://github.com/phetsims/scenery-phet/issues/341

[pixelzoom]

Verified that the assertion failure is gone, everything is inside layoutBounds, and the layout now matches the published version:

[pixelzoom]

I’ve noted this issue in the commits that reverted HSlider and NumberControl.

@phet-steele please verify after pulling sun and scenery-phet.

[phet-steele]

Verified!