"Saturated" in other locales extends beyond highlighted area

[Nancy-Salpepi]

Test device
MacBook Air (m1 chip)

Operating System
13.1

Browser
Safari 16.2

Problem description
For phetsims/qa#889, the word "saturated" in some locales (ex. fr, et, ja) extends past the highlighted area.

Steps to reproduce
Here is an example:

1. On the Concentration Screen, add solute until "saturated" appears in the solution
2. Change locale to fr

Visuals
With locale =fr

With locale=ja

Troubleshooting information: !!!!! DO NOT EDIT !!!!! Name: ‪Laboratoire Loi de Beer-Lambert‬ URL: https://phet-dev.colorado.edu/html/beers-law-lab/1.7.0-dev.8/phet/beers-law-lab_all_phet.html Version: 1.7.0-dev.8 2023-01-27 19:51:37 UTC Features missing: applicationcache, applicationcache, touch Flags: pixelRatioScaling User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/109.0.0.0 Safari/537.36 Language: en-US Window: 1536x781 Pixel Ratio: 1.7999999523162842/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: 31 uniform: 1024 Texture: size: 16384 imageUnits: 16 (vertex: 16, combined: 32) Max viewport: 16384x16384 OES_texture_float: true Dependencies JSON: {}

[zepumph]

Taking a look now.

[pixelzoom]

In SaturatedIndicator.ts, add labelText.boundsProperty listener to dynamically size backgroundNode and adjust backgroundNode.center. Or better would be to use scenery-phet BackgroundNode here (which was created long after this sim was written).

[samreid]

How does BackgroundNode differ from Panel? When would I use one vs the other?

[pixelzoom]

They are similar. BackgroundNode is already set up with useful default opacity, and it's more lightweight. BackgroundNode could probably be reimplemented by extending Panel, and perhaps narrowing its interface. If I just want to put a transparent Background on something, I typically use BackgroundNode.

[zepumph]

Ahh right! BackgroundNode. That was helpful to remember, thanks. I converted SaturatedIndicator to use BackgroundNode.

Before that though, I started with manual boundsProperty linking, like so:

Subject: [PATCH] a patch
---
Index: js/concentration/view/SaturatedIndicator.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/concentration/view/SaturatedIndicator.ts b/js/concentration/view/SaturatedIndicator.ts
--- a/js/concentration/view/SaturatedIndicator.ts	(revision ec916f06841abe870e8f97185e407e16e2d48375)
+++ b/js/concentration/view/SaturatedIndicator.ts	(date 1675116987448)
@@ -22,15 +22,20 @@
     const labelText = new Text( BeersLawLabStrings.saturatedStringProperty, { font: new PhetFont( 20 ), maxWidth: 400 } );
 
     // translucent light-gray background, so this shows up on all solution colors
-    const backgroundNode = new Rectangle( 0, 0, 1.2 * labelText.width, 1.2 * labelText.height, 8, 8,
-      { fill: 'rgba( 240, 240, 240, 0.6 )' } );
+    const backgroundNode = new Rectangle( 0, 0, 1, 1, 8, 8, {
+      fill: 'rgba( 240, 240, 240, 0.6 )'
+    } );
 
     // rendering order
     this.children = [ backgroundNode, labelText ];
 
-    // layout
-    labelText.centerX = backgroundNode.centerX;
-    labelText.centerY = backgroundNode.centerY;
+    labelText.boundsProperty.link( bounds => {
+      backgroundNode.rectBounds = bounds.dilatedXY( bounds.width * 0.1, bounds.height * 0.1 );
+
+      // layout
+      labelText.centerX = backgroundNode.centerX;
+      labelText.centerY = backgroundNode.centerY;
+    } );
 
     // make this node visible when the solution is saturated
     isSaturatedProperty.link( saturated => {
Index: js/concentration/view/ConcentrationScreenView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/concentration/view/ConcentrationScreenView.ts b/js/concentration/view/ConcentrationScreenView.ts
--- a/js/concentration/view/ConcentrationScreenView.ts	(revision ec916f06841abe870e8f97185e407e16e2d48375)
+++ b/js/concentration/view/ConcentrationScreenView.ts	(date 1675117071201)
@@ -180,12 +180,15 @@
 
     // Layout for things that don't have a position in the model.
 
-    // centered towards bottom of beaker
-    const saturatedIndicatorVisible = saturatedIndicator.visible; // so we can layout an invisible node
-    saturatedIndicator.visible = true;
-    saturatedIndicator.centerX = beakerNode.centerX;
-    saturatedIndicator.bottom = beakerNode.bottom - 30;
-    saturatedIndicator.visible = saturatedIndicatorVisible;
+    saturatedIndicator.boundsProperty.link( () => {
+
+      // centered towards bottom of beaker
+      const saturatedIndicatorVisible = saturatedIndicator.visible; // so we can layout an invisible node
+      saturatedIndicator.visible = true;
+      saturatedIndicator.centerX = beakerNode.centerX;
+      saturatedIndicator.bottom = beakerNode.bottom - 30;
+      saturatedIndicator.visible = saturatedIndicatorVisible;
+    } );
 
     // upper right
     solutePanel.right = this.layoutBounds.right - 20;

But that felt like the "old" way of dynamic layout. @marlitas and @chrisklus helped me take ManualConstraint for my first test drive. Let me just say that I love it!

Above, I updated BackgroundNode to use it (which made the code simpler and better in my opinion), and I also needed another one in BLL too for keeping the indicator centered in the beaker. @pixelzoom will you please review?

I was testing with http://localhost:8080/beers-law-lab/beers-law-lab_en.html?brand=phet&ea&debugger&locales=*&keyboardLocaleSwitcher and then I toggling locales with the keyboard once I saturated the solution.

[pixelzoom]

Conversion to BackgroundNode in SaturationIndicator.ts looks great, thanks.

.... But that felt like the "old" way of dynamic layout. @marlitas and @chrisklus helped me take ManualConstraint for my first test drive. Let me just say that I love it!

These are not "old ways" vs "new ways". They are different approaches for different situations. And in this case, I'm not at all a fan of ManualConstraint - it's much too heavyweight and obfuscated.

Compare this, where I have to understand "proxy nodes" and have the proper order of dependencies:

    // Layout for things that don't have a position in the model. Keep centered even if "Saturated" text changes.
    ManualConstraint.create( this, [ saturatedIndicator, beakerNode ], ( saturatedIndicatorProxy, beakerNodeProxy ) => {

      // centered towards bottom of beaker
      saturatedIndicatorProxy.centerX = beakerNodeProxy.centerX;
      saturatedIndicatorProxy.bottom = beakerNodeProxy.bottom - 30;
    } );

...to this more straightforward listener, where it's obvious what I'm listening to and what I'm doing:

    // Center 'Saturated' at centerBottom of beaker.
    saturatedIndicator.boundsProperty.link( () => {
      saturatedIndicator.centerX = beakerNode.centerX;
      saturatedIndicator.bottom = beakerNode.bottom - 30;
    } );

So I've replaced the former with the latter in ConcentrationScreenView.ts.

I'm on the fence about the use of ManualConstraint in BackgroundNode. It also feels too heavy-handed. And it was working code that didn't need to be touched - again, it's not "old way" vs "new way", there was nothing wrong or out-dated about the code that was in place. So since I'm the responsible dev for scenery-phet and original author of BackgroundNode, I've reverted the commit to BackgroundNode.ts.

Closing.

[pixelzoom]

While reviewing this issue, I noticed that SaturationIndicator was not instrumented. I opened #314, and confirmed with @arouinfar that this was an oversight. As the result of instrumenting SaturationIndicator, it was largely rewritten. See 3183271.

[pixelzoom]

Reopening this in case @zepumph wants to review or chat about ManualConstraint.

[zepumph]

Thanks. Your thoughts about ManualConstraint make sense, and I'll update my toolbox algorithm accordingly. Ready to close on this side.

[pixelzoom]

Reopening, this may not have made it into 1.7 release branch.

[pixelzoom]

OK in 1.7 release branch. Closing.