problems with screens query parameter

[loganbraywork]

Test device

Windows 10 laptop

Operating System

Windows 10 v.1903

Browser

Chrome v. 84.0.4147.89

Problem description

For phetsims/qa#515

When using the multiple screens query parameter, it is possible to create a home screen where none previously existed when using the command screens=2,1. A box does come up saying screen 2 is invalid, but a home screen appears with Macro appearing twice.

Steps to reproduce

1. Enter Query parameter ?screens=2,1

Visuals

Troubleshooting information:
!!!!! DO NOT EDIT !!!!!
Name: ‪pH Scale: Basics‬
URL: https://phet-dev.colorado.edu/html/ph-scale-basics/1.4.0-rc.1/phet/ph-scale-basics_all_phet.html
Version: 1.4.0-rc.1 2020-07-09 22:22:32 UTC
Features missing: touch
Flags: pixelRatioScaling
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/84.0.4147.89 Safari/537.36
Language: en-US
Window: 1433x706
Pixel Ratio: 1.3406250476837158/1
WebGL: WebGL 1.0 (OpenGL ES 2.0 Chromium)
GLSL: WebGL GLSL ES 1.0 (OpenGL ES GLSL ES 1.0 Chromium)
Vendor: WebKit (WebKit WebGL)
Vertex: attribs: 16 varying: 30 uniform: 4095
Texture: size: 16384 imageUnits: 16 (vertex: 16, combined: 32)
Max viewport: 32767x32767
OES_texture_float: true
Dependencies JSON: {}

[pixelzoom]

Reproduced in 1.4.0-rc.1 and master.

I see the same behavior in master with Concentration, but not with RIAW. They are both 1-screen sims, so I'm not sure what's causing different behavior. Perhaps its somehow related to the fact the pH Scale: Basics and Concentration are both subsets of some other sim, but I can imagine why that would be relevant.

Assigning to @chrisklus, since I believe he worked on this feature. High priority because this sim is in RC testing.

The fix will need to be patched into the 1.4 branches for ph-scale and ph-scale-basics.

[samreid]

In the commit, we fall back to nominal screens if there is an error, without appending to it on subsequent loops. It's working well in my testing.

@loganbraywork can you please test on phettest? Please run without assertions so it will not crash during startup.

@pixelzoom or @zepumph are you available to review this? It is just a few lines of code.

[pixelzoom]

forEach is an inappropriate iterator to be using in selectScreen.js, and this clearly has not been tested well. For example, running ph-scale-basicis with ?screens=3,4,5, I see the same error message 3x with totally bogus values:

Please use a for loop that is exited when allScreensValid becomes false. I.e.:

let allScreensValid = true;
for ( let i = 0; i < screensQueryParameter.length && allScreensValid; i++ ) {
   ...
}

[pixelzoom]

Self assigning. I'll fix this.

[pixelzoom]

@samreid please review the above commits.

Test cases:

// These will start without error
ph-scale-basics_en.html?screens=1
ph-scale_en.html?screens=1,2,3
ph-scale_en.html?screens=2
ph-scale_en.html?screens=2,3

// These contain errors, display warning dialog, and revert to default.
ph-scale-basics_en.html?screens=2,1
ph-scale_en.html?screens=1,4

[pixelzoom]

This issue also points to insufficient unit tests in selectScreensTests.js. I don't have time to address that, but it should be addressed. Tracking in phetsims/joist#643.

[pixelzoom]

phetsims/joist#644 is closed and changes have been cherry-picked.

phetsims/query-string-machine#42 is still blocking.

[pixelzoom]

phetsims/query-string-machine#42 is resolved and has been cherry-picked.

Ready for testing in next RC.

[pixelzoom]

This is irrelevant for ph-scale-basics, because it has 1 screen.

To verify in ph-scale, run the sim with these query parameters and verify that you get an "Invalid Query Parameters" dialog that correctly identifies the bad query parameter and its value:

• ?screens=4,1
• ?screens=-1
• ?screens=Macro,Micro

Also check a few valid values, like:

• ?screens=2
• ?screens=3,2

[KatieWoe]

No basics to test, but it looks ok on Win 10 Chrome in 1.5.0-dev.1. @pixelzoom if that is enough, feel free to close
Edit: disregard my comment

[brooklynlash]

On Chromebook on master, I only get an invalid query parameter screen with the Macro, Micro, and the valid ones work. The other two (-1, 4,1) are stuck on the phet loading screen and never load.

[pixelzoom]

I'm seeing all results as expected in master, on macOS.

@brooklynlash I'm not sure that I understand your report. Are you referring to the tests in #171 (comment), and saying that you're getting these results?

• ?screens=4,1 - "Invalid Query Parameters" dialog is NOT displayed, sim stuck on Loading screen
• ?screens=-1 - "Invalid Query Parameters" dialog is NOT displayed, sim stuck on Loading screen
• ?screens=Macro,Micro - "Invalid Query Parameters" dialog is displayed
• ?screens=2 - sim starts as expected
• ?screens=3,2 - sim starts as expected

Are you running with any other query parameters, or just screens?

[brooklynlash]

Yes, this is what I meant, and that was the only query parameter running. I can test also on Win10 and see if there is a discrepancy.

[brooklynlash]

Oh, I figured out the issue. Had the same issue on master on Win10, but then took out the brand=phet parameter and worked as expected.

[pixelzoom]

Great, thanks @brooklynlash. Closing.