phetsims / number-compare

"Number Compare" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
0 stars 0 forks source link

Return button can be mostly offscreen #29

Closed Nancy-Salpepi closed 1 year ago

Nancy-Salpepi commented 1 year ago

Test device MacBook Air M1 chip

Operating System 13.2.1

Browser Safari

Problem description For https://github.com/phetsims/qa/issues/917, if I set up a ten frame on the left side of the screen and add an object, the return button is mostly offscreen.

Visuals

Screenshot 2023-03-16 at 9 14 48 AM
Troubleshooting information: !!!!! DO NOT EDIT !!!!! Name: ‪Number Compare‬ URL: https://phet-dev.colorado.edu/html/number-compare/1.0.0-dev.6/phet/number-compare_all_phet.html Version: 1.0.0-dev.6 2023-03-15 01:30:22 UTC Features missing: applicationcache, applicationcache, touch Flags: pixelRatioScaling User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/16.3 Safari/605.1.15 Language: en-US Window: 1203x654 Pixel Ratio: 2/1 WebGL: WebGL 1.0 GLSL: WebGL GLSL ES 1.0 (1.0) Vendor: WebKit (WebKit WebGL) Vertex: attribs: 16 varying: 30 uniform: 1024 Texture: size: 16384 imageUnits: 16 (vertex: 16, combined: 32) Max viewport: 16384x16384 OES_texture_float: true Dependencies JSON: {}
amanda-phet commented 1 year ago

I think this is a weird situation because the button isn't always there, and if we change the drag bounds so that the user can't drag this button off-screen, then ten frames would never be able to exist close to the left edge. But if you drag an empty ten frame, you can get closer. This seems strange to me as well, and if we start coming up with logic that involves bouncing something back in the play area or other edge cases, this becomes a huge thing. image

Given the nature of this sim's timeline and resources, I would prefer to defer this until we have a client who wants a specific behavior.

@chrisklus will spend 30 minutes investigating the options and report back.

chrisklus commented 1 year ago

@marlitas @amanda-phet and I worked on this issue. This is tricky because we are constraining the position of the ten frame by its model bounds, which does not include the return button, and there is only return button in the view. We have an idea to use a constant from the size of the return button to change the constraining function in the ten frame's model. BUT now I'm realizing that would be importing a view thing into a model file which isn't great. Patch below to show where this constant would be factored into the model constraint.

```diff Index: js/lab/model/TenFrame.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/lab/model/TenFrame.ts b/js/lab/model/TenFrame.ts --- a/js/lab/model/TenFrame.ts (revision ca4f7ef4a637f299101e2edce92b3cf6db6eb7ad) +++ b/js/lab/model/TenFrame.ts (date 1679515340485) @@ -132,7 +132,7 @@ * Determine how this ten frame's origin can be placed in the provided bounds. */ public getOriginBounds( viewBounds: Bounds2 ): Bounds2 { - this.originBounds.minX = viewBounds.left - this.localBounds.left; + this.originBounds.minX = viewBounds.left - this.localBounds.left + 36; this.originBounds.minY = viewBounds.top - this.localBounds.top; this.originBounds.maxX = viewBounds.right - this.localBounds.right; this.originBounds.maxY = viewBounds.bottom - this.localBounds.bottom; ```
chrisklus commented 1 year ago

@zepumph and I handled this in the above commit. We made space for the return button when dragging whether it is visible or not. We were also able to do this in the view so we didn't need to import any constants to get the width of the return button.

Back over to @Nancy-Salpepi to test on phettest, thanks! Feel free to close if looking good.

Nancy-Salpepi commented 1 year ago

This looks good initially, but if I make my window size smaller, it moves out of view again.

https://user-images.githubusercontent.com/87318828/227626451-b714e3a5-feea-4341-9744-1fa7c5160934.mp4

zepumph commented 1 year ago

My guess is that this is correct, since this is the kind of sim that expands its controls to the edge of the screen instead of keeping to the dev bounds.

This is the same for a sim like projectile-motion: image

image

Over to @chrisklus to close if all is well.

chrisklus commented 1 year ago

I think @zepumph may have missed what @Nancy-Salpepi was demonstrating in https://github.com/phetsims/number-compare/issues/29#issuecomment-1483338438. That still looks incorrect to me and inconsistent with the drag behavior that was fixed in earlier commits. Leaving self-assigned to fix.

zepumph commented 1 year ago

Ahh yes, thanks. Sorry @Nancy-Salpepi, I was not paying close enough attention. So rude. I'll take a look now.

zepumph commented 1 year ago

Ok. @chrisklus and I weren't handling the general change when the play area drag bounds changed, and instead only the dragging logic. I set things up so all dragBounds changes go through the Node so that the return button bounds can be offset. @Nancy-Salpepi this should be fixed now. Please review.

Nancy-Salpepi commented 1 year ago

OK! Check this one off the list too!