search

phetsims / resistance-in-a-wire

"Resistance in a Wire" is an educational simulation in HTML5, by PhET Interactive Simulations.
http://phet.colorado.edu/en/simulation/resistance-in-a-wire
GNU General Public License v3.0
1 stars 4 forks source link

Max width of keyboardNav dialog #203

Closed KatieWoe closed 5 years ago

KatieWoe commented 5 years ago

Test device: Dell Operating System: Win 8.1 and 10 Browser: Chrome Problem description: For https://github.com/phetsims/QA/issues/239 The keyboardNav dialog can become large enough to slightly exceed bounds. This is most easily seen by using stringTest=long and dev query parameters together. It can also be seen by resizing the window of the sim. It does not seriously exceed bounds, and the X is still entirely in frame. Steps to reproduce:

  1. Set dev and stringTest=long
  2. Make variables small to make bounds of dialog easier to see
  3. Open keyboardnav dialog
  4. Resize window so it is taller than wide

Screenshots: outsidedev

Troubleshooting information (do not edit):

Name: 12345678901234567890123456789012345678901234567890 URL: https://phet-dev.colorado.edu/html/resistance-in-a-wire/1.6.0-rc.2/phet/resistance-in-a-wire_en_phet.html?dev&stringTest=long Version: 1.6.0-rc.2 2018-12-13 16:36: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/71.0.3578.98 Safari/537.36 Language: en-US Window: 1536x732 Pixel Ratio: 2.5/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: 4096 Texture: size: 16384 imageUnits: 16 (vertex: 16, combined: 32) Max viewport: 16384x16384 OES_texture_float: true Dependencies JSON: {"assert":{"sha":"928741cf","branch":"HEAD"},"axon":{"sha":"de77d4b5","branch":"HEAD"},"brand":{"sha":"1fd6682e","branch":"HEAD"},"chipper":{"sha":"fa2fbadf","branch":"HEAD"},"dot":{"sha":"bbbd8526","branch":"HEAD"},"joist":{"sha":"82521d0c","branch":"HEAD"},"kite":{"sha":"380cef53","branch":"HEAD"},"phet-core":{"sha":"1b90ac2f","branch":"HEAD"},"phet-io":{"sha":"38d7b161","branch":"HEAD"},"phet-io-wrapper-classroom-activity":{"sha":"246085c1","branch":"HEAD"},"phet-io-wrapper-hookes-law-energy":{"sha":"7479b0ec","branch":"HEAD"},"phet-io-wrapper-lab-book":{"sha":"c46f7839","branch":"HEAD"},"phet-io-wrappers":{"sha":"a6bc62ca","branch":"HEAD"},"phetcommon":{"sha":"cd63d89a","branch":"HEAD"},"query-string-machine":{"sha":"06ed6276","branch":"HEAD"},"resistance-in-a-wire":{"sha":"c996a2e3","branch":"HEAD"},"scenery":{"sha":"9953c5f7","branch":"HEAD"},"scenery-phet":{"sha":"15ed6d54","branch":"HEAD"},"sherpa":{"sha":"2cd50500","branch":"HEAD"},"sun":{"sha":"9ee72759","branch":"HEAD"},"tambo":{"sha":"65315b32","branch":"HEAD"},"tandem":{"sha":"ed8f8f1d","branch":"HEAD"}}
arouinfar commented 5 years ago

Nice catch @KatieWoe.

Seems like the dialog itself probably needs a maxWidth. I think we could let it get pretty wide, so long as it has a few px padding between the dialog and the layoutBounds, so perhaps 1004 px would be appropriate?

jbphet commented 5 years ago

I did a "quick and dirty" fix for this where I limited the width of the content node in the RIAW sim code. However, it seems like it would be better to do this generally so that KeyboardHelpDialog is automatically limited to the dev bounds. Assigning to @jessegreenberg, since he was the original author of KeyboardHelpDialog, to determine if this is viable.

jbphet commented 5 years ago

Note to self: Whether we keep the quick fix described in the previous comment or integrate a more general solution, it will need to be propagated to the 1.6 release branch.

jessegreenberg commented 5 years ago

Thanks @jbphet the above commit looks reasonable to me. It seems like most PhET dialogs assume that their content is limited to dev bounds. This may be larger than just the KeyboardHelpDialog and I made the above issue to investigate.

jessegreenberg commented 5 years ago

Update: We have a couple of options in https://github.com/phetsims/joist/issues/546 and we are narrowing in on a general solution for this. It is pending review. @jbphet we will let you know when this is ready.

jessegreenberg commented 5 years ago

OK, we have a general fix for this in https://github.com/phetsims/joist/issues/546 and this is the joist commit to pull into the release: https://github.com/phetsims/joist/commit/428b7a2d985fd7918e0dbf2c9025d34b724db9db.

No longer on hold, @jbphet back to you, but let me know if I can help.

jbphet commented 5 years ago

I've moved this to the 1.6 branch so the fix should appear in v1.6.0-rc.3.

KatieWoe commented 5 years ago

This looks good 1.6.0-rc.3

jessegreenberg commented 5 years ago

In https://github.com/phetsims/sun/issues/435 we decided that there wasn't a general solution for this, and the layoutStrategy added to KeyboardHelpDialog was removed. maxWidth should be set on the HelpContent text instead. Reopening to do so.

jessegreenberg commented 5 years ago

I added nested options to SliderControlsHelpContent and GeneralNavigationHelpContent and used in the above commit, so the dialog is limited to the dev bounds.

This has aligned the sim with the decision in https://github.com/phetsims/sun/issues/435, but I do not think any further changes need to be propagated to the release branch, so closing.