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

Max width of keyboardNav dialog #203

Closed
KatieWoe opened this issue Dec 17, 2018 · 10 comments
Closed

Max width of keyboardNav dialog #203

KatieWoe opened this issue Dec 17, 2018 · 10 comments

Comments

@KatieWoe
Copy link
Contributor

Test device:
Dell
Operating System:
Win 8.1 and 10
Browser:
Chrome
Problem description:
For phetsims/qa#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
Copy link

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?

@arouinfar arouinfar removed their assignment Dec 17, 2018
@jbphet
Copy link
Contributor

jbphet commented Dec 18, 2018

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 jbphet removed their assignment Dec 18, 2018
@jbphet
Copy link
Contributor

jbphet commented Dec 18, 2018

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
Copy link
Contributor

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
Copy link
Contributor

Update: We have a couple of options in phetsims/joist#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
Copy link
Contributor

OK, we have a general fix for this in phetsims/joist#546 and this is the joist commit to pull into the release: phetsims/joist@428b7a2.

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

@jbphet
Copy link
Contributor

jbphet commented Dec 27, 2018

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

@KatieWoe
Copy link
Contributor Author

KatieWoe commented Jan 3, 2019

This looks good 1.6.0-rc.3

@jessegreenberg
Copy link
Contributor

In phetsims/sun#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
Copy link
Contributor

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 phetsims/sun#435, but I do not think any further changes need to be propagated to the release branch, so 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

4 participants