Slight visual shift of some UI components

[stemilymill]

Test device
Dell laptop, Chromebook

Operating System
Win10, Chrome OS

Browser
Chrome 91.0.4472.124

Problem description
For phetsims/qa#669.
In the Micro screen, changing the volume of liquid in the beaker causes some of the numbers in the graph to move slightly up or down, and in the My Solution screen, when the graph is displaying Quantity, setting pH to minimum or maximum value causes the graph, beaker, and beaker control panel to move up or down.

Steps to reproduce
Micro screen:
Alternate adding and removing water from the beaker.

My Solution screen:
Toggle graph to show Quantity.
Drag the blue OH- or red H3O+ indicator to the top or bottom of the graph.

Visuals

Micro screen

https://drive.google.com/file/d/10dzmjeuit3-fbPRRqMOQ85Sr18qy_sJ4/view?usp=sharing

My Solution screen

Troubleshooting information:
!!!!! DO NOT EDIT !!!!!
Name: ‪pH Scale‬
URL: https://phet-dev.colorado.edu/html/ph-scale/1.5.0-rc.1/phet/ph-scale_all_phet.html
Version: 1.5.0-rc.1 2021-07-13 18:33:46 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/91.0.4472.124 Safari/537.36
Language: en-US
Window: 1418x656
Pixel Ratio: 1.3541666269302368/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: {}

[pixelzoom]

To clarify about the visuals above...

• On the Micro screen, interacting with the faucets causes some of the tick labels on the graph to shift up/down.
• On the My Solution screen, dragging with the graph indicators causes the beaker, panel below the beaker, and the entire graph to shift up/down.

I do not see this behavior on macOS 11.3.1 with Chrome 91.0.4472.164.

All of these elements have static positions. So this is probably a browser problem that I have no control over. Assigning to @jonathanolson to comment with high priority.

[pixelzoom]

@stemilymill Your test device is behind a few versions. The latest version of Chrome is 91.0.4472.164. Can you please update and re-test, to verify that this is not a bug that has been fixed in the browser?

[stemilymill]

@pixelzoom My device is now up-to-date and this all behaves exactly the same.

[pixelzoom]

@stemilymill Got it, thanks for checking.

[jonathanolson]

So far I'm unable to reproduce (Win10, Chrome 91.0.4472.164). It looks potentially size-specific, so I matched the browser size with the reported resolution (1418x656) and still didn't see it.

I did see an interesting pixel ratio in the debug info (Pixel Ratio: 1.3541666269302368/1), there definitely could be something going on specific to that setup on the browser-side.

@pixelzoom could we try a test version where we have a layerSplit: true separating the graph background (the shifting text instances, labels, etc.), so that the foreground controls/readouts are in a separate layer? @stemilymill do you know if this is broken on master (e.g. on phettest)?

[pixelzoom]

@pixelzoom could we try a test version where we have a layerSplit: true separating the graph background (the shifting text instances, labels, etc.), so that the foreground controls/readouts are in a separate layer?

Please clarify. Set layerSplit: true for the graph background, or for the elements that are shifting around?

Since the entire beaker and panel below the beaker are also shifting around, would it also be sufficient to set layerSplit: true for those entire elements? That might be easier than trying to separate out parts of the graph.

[jonathanolson]

I'd like the layerSplit to be put on something that:

1. Contains all of the text that is shifting, but should not be shifting. Can contain other things
2. Does NOT include things that are actually moving, like the indicators or things in the play area.

I'm curious if that will prevent the bug from happening. Ideally do this with one layerSplit... one per tick would probably be bad for performance.

[pixelzoom]

Discussed with @jonathanolson on Zoom. The plan is:

• Start by trying to fix the problem on the My Solution screen. If we fix that, we can likely address the Micro screen similarly.
• Publish a dev version from master, see if @stemilymill can reproduce it there. If this is reproducible in master, that makes our lives quite a bit easier.
• If reproducible in master, add layerSplit: true to the 2 draggable indicators, publish another dev version, and ask @stemilymill to compare behavior.
• Based on the visuals, elements appear to shift when the indicator is dragged to it max value. Verify whether that's actually the case, or whether there is shifting as the indicator is dragged within a smaller range. If it's specific to max, the fact that the displayed value changes from scientific notation to non-scientific notation may involve a resize that is significant.

[pixelzoom]

@stemilymill requests:

(1) Please test 1.6.0-dev.2 to see if the problem occurs in that version. (This is a dev version built from master, with no changes/fixes. If it's reproducible there, then we can troubleshoot in master, rather than in the 1.5 branch.)

(2) Based on the visuals that you provided, UI elements appear to shift when the indicator is dragged to its max value (e.g. OH- = 5.0). Is that a requirement to make this happen, or does the shifting occur as you drag the indicator through a smaller range?

Thanks!

[pixelzoom]

• Publish a dev version from master, see if @stemilymill can reproduce it there. If this is reproducible in master, that makes our lives quite a bit easier.

Good news! I can reproduce this in master on macOS 11.3.1 + Chrome 91.0.4472.164. The key is to drag the indicator to its maximum value while the graph is set to "Quantity".

Steps to reproduce for the My Solution screen, 100% consistent:

1. Run the sim from master
2. Go to the My Solution screen
3. Set the AB switch at the top of the graph to "Quantity". (Does NOT happen with switch set to "Concentration"!)
4. Drag the OH- indicator to its max value (5.00). Then alternate between 5.00 and just below 5.00. You'll see several UI elements shift up and down: the beaker, the panel below the beaker, and the entire body of the graph. They shift up when OH- is 5.00, down (to their normal position) when OH- is < 5.00.

I can also reproduce for the Micro screen, but precise steps are elusive. It seems like a tick label on the graph (especially 10^-14) will shift when the level of the solution in the beaker is around the same y position as the tick label. Add a solute (like Battery Acid), then play around with the 2 faucets, adding water and draining solution.

[pixelzoom]

@stemilymill you can ignore the requests in #226 (comment), since I've reproduced this on macOS in master.

[pixelzoom]

• If reproducible in master, add layerSplit: true to the 2 draggable indicators, ...

I tried adding layerSplit: true to the default options for GraphIndicatorNode (the indicators for H2O, H3O+, and OH-). Running unbuilt from master, this resulted in no change in behavior. Static UI elements continue to shift up and down when following the steps in #226 (comment).

@jonathanolson thoughts on how to proceed? And can you also reproduce this now, following the steps in #226 (comment)?

[pixelzoom]

If it's specific to max, the fact that the displayed value changes from scientific notation to non-scientific notation may involve a resize that is significant.

This doesn't appear to be related to resizing the value display. I added this to GraphIndicatorNode, to constrain the size:

    valueNode.maxWidth = 100;
    valueNode.maxHeight = valueNode.height;

... and there was no change in behavior.

[pixelzoom]

I'm not sure if this should be considered blocking for pH Scale 1.5. It occurs with lastest Chrome on all platforms. It doesn't make the sim unusable, but it's mildly distracting. @arouinfar your call.

[arouinfar]

This reminds me of phetsims/gravity-force-lab#101, which is currently in the published version of Gravity Force Lab. The underlying cause may be different, but there's certainly precedent for publishing a sim with a visual wiggle.

This issue occurs only in a very specific situation and doesn't affect usability. Given that there isn't an obvious solution, I'm fine with deferring or closing as wont-fix. Your call @pixelzoom.

[jonathanolson]

No shift noticed, screenshots in both conditions. macOS 11.4, Chrome 91.0.4472.114, running from master, steps taken from #226 (comment).

[jonathanolson]

Updated to Chrome 92.0.4515.107 (macOS), still no reproduction of the bug, either in the full-screen or with devtools open.

[pixelzoom]

Removing high-priority label, since @arouinfar says in #226 (comment) that this is non-blocking, and could even be closed as "won't fix".

[pixelzoom]

I upgraded Chrome to 92.0.4515.107 (lastest). With macOS 11.3.1, there was no change.

Then I upgraded macOS to 11.5 (latest). With Chrome to 92.0.4515.107, there was again no change.

So I'll move on to trying preventFit: true.

[pixelzoom]

In the My Solution screen, there's a lot more shifting around than I originally noticed. Here's the (complete?) list of things that shift up/down when the steps in #226 (comment) are followed:

These parts of ph-scale/MySolutionScreenView:

• The entire control panel below the beaker, beakerControlPanel
• The entire beaker, beakerNode
• The solution in the beaker, solutionNode
• The molecule counts, moleculeCountsNode
• The Reset All button, resetAllButton

These parts of ph-scale/GraphNode:

• The complete graph body, incuding tick marks and labels
• The value in the H2O indicator on the graph, but not the rest of the H2O indicator

There parts of ph-scale/PHMeterNode:

• The "pH" label in the pH meter, which is the AccordionBox titleNode option
• The pH probe, probeNode.

If I set ANY NODE ON THIS SCREEN to preventFit: true, then this problem goes away. It doesn't have to be one of the Nodes listed above that is shifting around. It can be ANY Node. For example, I can add preventFit: true to resetAllButton in MySolutionScreenView.js, one of the Nodes that is shifting around. Or I can add preventFit: true to something like PHSpinnerNode down in PHMeterNode.js, one of the Nodes that is NOT shifting around. All it takes is for one Node on the screen to have preventFit: true, and the problem disappears.

I verified that adding preventFit: true to resetAllButton for MicroScreenView.js and MySolutionScreenView.js fixes the problem for both screen.

@jonathanolson does that tell you anything about the nature of the problem? And do you think it would be OK to add preventFit: true to resetAllButton on both screens as a workaround?

[pixelzoom]

I'm not familiar with preventFit, so I looked up the doc in Node.js:

// {boolean} - If true, Scenery will not fit any blocks that contain drawables attached to Nodes underneath this
//             Node's subtree. This will typically prevent Scenery from triggering bounds computation for this
//             sub-tree, and movement of this Node or its descendants will never trigger the refitting of a block.
preventFit: DEFAULT_OPTIONS.preventFit

So this sounds like a safe thing to set on a screen's ResetAllButton, since it never moves. Is that correct?

[jonathanolson]

So this sounds like a safe thing to set on a screen's ResetAllButton, since it never moves. Is that correct?

It's a safe thing in general, mainly could only reduce/increase performance.

@jonathanolson does that tell you anything about the nature of the problem? And do you think it would be OK to add preventFit: true to resetAllButton on both screens as a workaround?

On the ResetAllButton sounds good, but placing preventFit on the content that moves might be more appropriate, in case layer splits are added in the future. Currently, it seems everything is going into one layer, so the preventFit flag on any of those nodes will cause the improved behavior.

Presumably the shifting of the SVG element (being fitted) was causing a number of problems, so I'm curious to see if other issues are noticed in other sims as well. preventFit mainly exists to reduce memory usage and improve performance, so if it starts to wreck sims then we could presumably remove it.

[pixelzoom]

Thanks @jonathanolson.

I'm going to go with the least-cost solution here, and patch it into the 1.5 branch. I'll put preventFit: true on the ResetAllButton instances, along with a comment referencing this issue, and noting that this should be revisited if layer splits are added in the future.

[pixelzoom]

Patched in master and 1.5 branches. While this problem doesn't appear in ph-scale-basics, I did patch it in the 1.5 branch to prevent possible merge conflicts with future changes.

Ready for verification in the next RC.

Steps to verify for the My Solution screen:

1. Go to the My Solution screen
2. Set the AB switch at the top of the graph to "Quantity". (Does NOT happen with switch set to "Concentration"!)
3. Drag the OH- indicator to its max value (5.00). Then alternate between 5.00 and just below 5.00. You should not see any UI elements shifting up and down.

Steps to verify for the Micro screen:

1. Go to the Micro screen
2. Set the solute combo box to "Battery Acid"
3. Change the volume of solution in the beaker by alternative between filling the beaker using the water faucet, and draining solution using the drain faucet. You should not see any UI elements shifting up and down.

[Nancy-Salpepi]

Could not reproduce issue using the steps provided (MacBook 11.4 M1 chip + chrome).

[pixelzoom]

After a discussion with @jonathanolson and learning more in phetsims/scenery#1289, I moved the preventFit: true workaround to the ScreenView level. It's now applied to all ScreenViews in this sim via this constant in PHScaleConstants:

  SCREEN_VIEW_OPTIONS: {
    layoutBounds: new Bounds2( 0, 0, 1100, 700 ),

    // Workaround for things shifting around while dragging
    // See https://github.com/phetsims/scenery/issues/1289 and https://github.com/phetsims/ph-scale/issues/226
    preventFit: true
  },