Active line label overlaps with y=x, y=-x lines in long and rtl string tests

[rea-laura]

Test Device

Leibniz

Operating System

iOS 12.1.3

Browser

Safari 12.1.3

Problem Description

For phetsims/qa#277:
When y=x or y=-x lines are activated and the interactive line is overlaid over these green lines, the labels overlap, but only on some string tests.

Steps to Reproduce

1. go to sim with either long or rtl string test query parameter
2. go to slope-intercept or point-slope screen (2 and 3rd panels on startup)
3. check the y=x or y=-x box
4. position the movable line on top of a green line
5. note that the line label overlaps with the green line label. Only happens when string test is long or rtl. Otherwise the green label is replaced with the black label and no overlap occurs.

Visuals

Overlap (rtl and long):

No overlap (no string test, double, and X):

[pixelzoom]

Thanks @rea-laura, I'll investigate.

I was hoping that ?stringTest=double would make it a little easier to see what's going on. But that looks fine:

[pixelzoom]

This problem is present on the Slope-Intercept and Point-Slope screens.

I experimented with ?stringTest=XXXXXX. The problem doesn't show up at all until you hit 6 Xs, see screenshots below. My WAG is that the equations have different maxWidth values, with the black equation (for the interactive line) having the smaller maxWidth value. Same behavior for y=x and y=-x.

?stringTest=XXXXX (5X):

?stringTest=XXXXXX (6X):

[pixelzoom]

I said:

My WAG is that the equations have different maxWidth values ...

To test this...

In LineFormsGraphNode, I added this to the constructor:

console.log( 'interactiveLineNode.equationNode.maxWidth=' + this.interactiveLineNode.equationNode.maxWidth );

And I modified standardLineAdded like so:

    standardLineAdded: function( line ) {
      var lineNode = new LineNode( new Property( line ), this.model.graph, this.model.modelViewTransform,
        { equationType: this.equationType } );
      console.log( 'standardLineAdded: line=' + line.toString() + ', lineNode.equationNode.maxWidth=' + lineNode.equationNode.maxWidth );
      this.standardLinesParentNode.addChild( lineNode );
    },

Start the sim, go to Slope-Intercept screen, 'y = x' checkbox checked. Console output is:

interactiveLineNode.equationNode.maxWidth=200
interactiveLineNode.equationNode.maxWidth=200
interactiveLineNode.equationNode.maxWidth=200
standardLineAdded: line=Line[x1=0 y1=0 x2=1 y2=1 rise=1 run=1 color=rgb( 16, 178, 15 )], lineNode.equationNode.maxWidth=200

So maxWidth is the same (200) for the equations on the interactive line and y=x.

[pixelzoom]

Odd... If I move the interactive line from y=x, to y=0, then back to y=x, the problem (permanently!) goes away. The interactive equation then perfectly overlaps y=x (and y=-x).

[pixelzoom]

My next WAG is that SlopeInterceptEquationNode and PointSlopeEquationNode both have some subcomponent Node that is invisible, but whose default position is making the Node unnecessarily wide. And that setting the line to y=0 triggers a layout change that permanently resolves the issue.

Equations on the graph are "dynamic", so start with createDynamicLabel in SlopeInterceptEquationNode and PointSlopeEquationNode.

[pixelzoom]

My WAG in the preceding comment was correct. So this was easily fixed by fully resetting the location of all equation subcomponents (Nodes) whenever the equation updates, see commit above.

A potential risk of this fix is that some equation is relying on the location of a previous update, and some part of the equation would be in the wrong location. I looked carefully at equations in Slope-Intercept, Point-Slope and Line Game screens (because this same code is used in Line Game), and I didn't see any issues.

@rea-laura back to you to verify in master. If all looks OK, please leave this issue open for regression testing in 1.3.0-rc.2.

[KatieWoe]

To me this looks like the same issue as #114, as that only occurred with longer strings.
Edit: never mind, it does seem a bit different from what I can tell. Good catch.

[pixelzoom]

@KatieWoe it's actually a subtly different issue.

[pixelzoom]

To verify this issue:

1. Start the sim with normal strings (no stringTest query parameter)
2. Go to Slope-Intercept screen
3. Select check boxes for y=x and y=-x.
4. Move the interactive line to y=x and verify that the equation on the interactive (black) line exactly covers the equations on the y=x (green) line.
5. Repeat step 4 for y=-x.
6. Go to Point-Slope screen.
7. Repeat steps 3-5
8. Re-starting the sim with ?stringTest=long and repeat steps 1-7.

You could also do the above verification steps 1-7 for a RTL language, but stringTest=long is essentially testing the same code path.

[KatieWoe]

Looks good on master.

[pixelzoom]

Pending regression testing in 1.3.0-rc.2.

[KatieWoe]

This seems to be occurring again.

[pixelzoom]

My apologies, the fix in 5478172 did not get cherry-picked into the 1.3 branch, so it's not in 1.3.0-rc.2. Please skip this we'll come back to it in a quick 1.3.0-rc.3.

[pixelzoom]

To verify this issue, follow the steps in #120 (comment).

[KatieWoe]

Fixed in both rcs