phetsims / circuit-construction-kit-common

"Circuit Construction Kit: Basics" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
10 stars 10 forks source link

Value location interferes with ability to select circuit element #845

Closed Nancy-Salpepi closed 1 year ago

Nancy-Salpepi commented 2 years ago

Test device iPad Air 2 Operating System 15.2

Browser safari

Problem description For https://github.com/phetsims/qa/issues/772

On either screen, when the values checkbox is selected and the voltage is negative, the voltage value appears in the middle of the battery. This makes it difficult to select the battery--especially on the iPad (but also with the trackpad on my mac). Is it possible to change the location of the voltage value to below the battery or keep it above the battery like with positive values?

Steps to reproduce

  1. On the iPad, make a circuit and attach the voltmeter
  2. Check the 'values' checkbox
  3. Flip the battery--the value moves over the battery instead of above it
  4. Tap the screen so the battery is no longer selected.
  5. Try to select the battery

Visuals

https://user-images.githubusercontent.com/87318828/152841428-aadf70c9-04a8-4128-b935-e801be84bb71.mov

Troubleshooting information: !!!!! DO NOT EDIT !!!!! Name: ‪Circuit Construction Kit: DC‬ URL: https://phet-dev.colorado.edu/html/circuit-construction-kit-dc/1.3.0-dev.14/phet/circuit-construction-kit-dc_all_phet.html Version: 1.3.0-dev.14 2022-02-01 01:48:26 UTC Features missing: touch Flags: pixelRatioScaling User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/15.1 Safari/605.1.15 Language: en-US Window: 1409x687 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: {}
arouinfar commented 2 years ago

Thanks @Nancy-Salpepi, this looks like a bug to me. The location of the value display should not depend on the battery's orientation. It's also a bit odd that the value blocks the pickability of the battery itself, but I don't know if that is to be expected or not.

Nancy-Salpepi commented 2 years ago

Thanks @arouinfar! I just went back and looked at the published versions of CCK-AC and DC and the value location changes from top to bottom when the battery is flipped. voltagevalue

arouinfar commented 2 years ago

Oh, good to know @Nancy-Salpepi!

@samreid since this affects sim usability, I'm going to tag it for the preview milestone. It should either be patched or documented as a known bug.

arouinfar commented 2 years ago

@KatieWoe also reported this issue in #847

Test device Dell Operating System Win 11 Browser Chrome Problem description For https://github.com/phetsims/qa/issues/772. This does not seem to be as much of an issue in published, as upside down elements are not as fully covered by their values label. In the dev version, if certain elements are upside down (such as by flipping the battery) the values label can cover the entire element and clicking on it does not focus on it so you can't change voltage etc.

Visuals In published:

pubupsidedown

In dev: values_in_way

Noting that this can also make dragging objects more difficult. It is particularly bad with fuses.

Troubleshooting information:

!!!!! DO NOT EDIT !!!!! Name: ‪Circuit Construction Kit: DC‬ URL: https://phet-dev.colorado.edu/html/circuit-construction-kit-dc/1.3.0-dev.14/phet/circuit-construction-kit-dc_all_phet.html Version: 1.3.0-dev.14 2022-02-01 01:48:26 UTC Features missing: applicationcache, applicationcache, touch Flags: pixelRatioScaling User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/97.0.4692.99 Safari/537.36 Language: en-US Window: 1280x649 Pixel Ratio: 1.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: 32767x32767 OES_texture_float: true Dependencies JSON: {}
samreid commented 2 years ago

Feb 10 design check-in: For the client preview, making the label non-pickable is sufficient. For the full version, the labels should be in the correct locations.

samreid commented 2 years ago

I made the labels non-pickable and confirmed that you can click through them to drag circuit elements now. Also a reminder that unpickable things cannot be studio autoselected. In this case perhaps it is OK since the text is customized through the model element's battery_0.labelTextProperty. @arouinfar is this OK for the client preview?

arouinfar commented 2 years ago

Thanks @samreid. The behavior in master is reasonable for the client preview and is definitely an improvement.

We can defer addressing the label location until the next milestone.

samreid commented 2 years ago

@arouinfar what is the desired location for the label? The current version shows this behavior:

Kapture 2022-02-18 at 17 56 44

I wonder if it ended up that way in part because it prevents some overlap when creating loops?

@arouinfar please advise.

arouinfar commented 2 years ago

@samreid the location of the label generally looks good as the component is rotated, but perhaps the label could pop back up to the top when it reaches 180°.

samreid commented 2 years ago

This commit prefers to put the label above the circuit element, the rest of the logic is the same:

Kapture 2022-08-27 at 17 23 19

@arouinfar can you please review?

arouinfar commented 1 year ago

Looks great, thanks @samreid!