Highlight lineWidth should not scale with the Node #1469
Assignees
Labels
Comments
|
OK I think the lineWidth code is generally trying to make the highlight lineWidth the same regardless of the transform. But it isn't working correctly because it is using the wrong Node in HighlightOverlay to calculate the lineWidth. This patch fixes the issue except for when zooming in with pan/zoom. Index: js/overlays/HighlightOverlay.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/overlays/HighlightOverlay.ts b/js/overlays/HighlightOverlay.ts
--- a/js/overlays/HighlightOverlay.ts (revision add2a95a38e87e30c409052a9f9b6f207f7aaee9)
+++ b/js/overlays/HighlightOverlay.ts (date 1665790122045)
@@ -638,10 +638,10 @@
*/
private afterTransform(): void {
if ( this.mode === 'shape' ) {
- this.shapeFocusHighlightPath.updateLineWidth();
+ this.shapeFocusHighlightPath.updateLineWidth( this.highlightNode );
}
else if ( this.mode === 'bounds' ) {
- this.boundsFocusHighlightPath.updateLineWidth();
+ this.boundsFocusHighlightPath.updateLineWidth( this.highlightNode );
}
else if ( this.mode === 'node' && this.activeHighlight instanceof FocusHighlightPath && this.activeHighlight.updateLineWidth ) {
|
|
I made a little progress on this but the changes are too disruptive for me to commit to tonight. Basically, I found that the highlightNode should almost always be used to determine the lineWidth of the highlight because it is transformed by the TransformTracker. Then the final width can be corrected by the scale of the The most disruptive part of this is that the correction makes all highlights the same but collectively thinner. So to have the same look I had to increase the base linewidth of FocusHighlightPath. But there are places throughout the codebase that are using Here is a patch: Index: joist/js/NavigationBarScreenButton.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/joist/js/NavigationBarScreenButton.ts b/joist/js/NavigationBarScreenButton.ts
--- a/joist/js/NavigationBarScreenButton.ts (revision 4d15087209c5fadd822ca0e3bf54ecf658836d3f)
+++ b/joist/js/NavigationBarScreenButton.ts (date 1666219757357)
@@ -241,8 +241,8 @@
`this.width ${this.width} !== options.maxButtonWidth ${options.maxButtonWidth}` );
// pdom - Pass a shape to the focusHighlight to prevent dilation, then tweak the top up just a hair.
- const highlightLineWidth = FocusHighlightPath.getOuterLineWidthFromNode( this );
- this.focusHighlight = Shape.bounds( this.bounds.withMinY( this.bounds.minY - highlightLineWidth / 2 ) );
+ // const highlightLineWidth = FocusHighlightPath.getOuterLineWidthFromNode( this );
+ // this.focusHighlight = Shape.bounds( this.bounds.withMinY( this.bounds.minY - highlightLineWidth / 2 ) );
this.mutate( options );
}
Index: scenery/js/accessibility/FocusHighlightPath.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery/js/accessibility/FocusHighlightPath.ts b/scenery/js/accessibility/FocusHighlightPath.ts
--- a/scenery/js/accessibility/FocusHighlightPath.ts (revision ef06d9923c358574a40c1c6b657f3a1d9bbf29a5)
+++ b/scenery/js/accessibility/FocusHighlightPath.ts (date 1666218198674)
@@ -15,7 +15,7 @@
import Vector2 from '../../../dot/js/Vector2.js';
import { Shape } from '../../../kite/js/imports.js';
import optionize, { combineOptions } from '../../../phet-core/js/optionize.js';
-import { Color, TPaint, Node, Path, PathOptions, scenery, Trail } from '../imports.js';
+import { animatedPanZoomSingleton, Color, Node, Path, PathOptions, scenery, TPaint, Trail } from '../imports.js';
// constants
// default inner and outer strokes for the focus highlight
@@ -31,12 +31,12 @@
const OUTER_DARK_GROUP_FOCUS_COLOR = new Color( 'rgba(159,15,80,1.0)' );
// Determined by inspection, base widths of focus highlight, transform of shape/bounds will change highlight line width
-const INNER_LINE_WIDTH_BASE = 2.5;
-const OUTER_LINE_WIDTH_BASE = 4;
+const INNER_LINE_WIDTH_BASE = 4;
+const OUTER_LINE_WIDTH_BASE = 6;
// determined by inspection, group focus highlights are thinner than default focus highlights
-const GROUP_OUTER_LINE_WIDTH = 2;
-const GROUP_INNER_LINE_WIDTH = 2;
+const GROUP_OUTER_LINE_WIDTH = 2.5;
+const GROUP_INNER_LINE_WIDTH = 2.5;
type SelfOptions = {
@@ -171,25 +171,37 @@
this.innerHighlightPath.lineWidth = this.getInnerLineWidth( node );
this.highlightChangedEmitter.emit();
}
+
+ /**
+ * Update the line width of this focus highlight, corrected by scaling of the root node of the display
+ * which has likely been modified by an animatedPanZoomListener. See getWidthMagnitudeFromTransform for more info.
+ * @param node
+ */
+ public updateLineWidthWithRootScaleCorrection( node?: Node ): void {
+ node = node || this; // update based on node passed in or on self.
+ this.lineWidth = this.getOuterLineWidth( node, true );
+ this.innerHighlightPath.lineWidth = this.getInnerLineWidth( node, true );
+ this.highlightChangedEmitter.emit();
+ }
/**
* Given a node, return the lineWidth of this focus highlight.
*/
- public getOuterLineWidth( node: Node ): number {
+ public getOuterLineWidth( node: Node, correctRootTransform?: boolean ): number {
if ( this.outerLineWidth ) {
return this.outerLineWidth;
}
- return FocusHighlightPath.getOuterLineWidthFromNode( node );
+ return FocusHighlightPath.getOuterLineWidthFromNode( node, correctRootTransform );
}
/**
* Given a node, return the lineWidth of this focus highlight.
*/
- public getInnerLineWidth( node: Node ): number {
+ public getInnerLineWidth( node: Node, correctRootTransform?: boolean ): number {
if ( this.innerLineWidth ) {
return this.innerLineWidth;
}
- return FocusHighlightPath.getInnerLineWidthFromNode( node );
+ return FocusHighlightPath.getInnerLineWidthFromNode( node, correctRootTransform );
}
/**
@@ -272,24 +284,30 @@
/**
- * Get the outer line width of a focus highlight based on the node's scale and rotation transform information.
+ * Get the outer line width of a focus highlight based on the node's local scale and rotation transform information.
*/
- public static getInnerLineWidthFromNode( node: Node ): number {
- return INNER_LINE_WIDTH_BASE / FocusHighlightPath.getWidthMagnitudeFromTransform( node );
+ public static getInnerLineWidthFromNode( node: Node, correctRootTransform?: boolean ): number {
+ return INNER_LINE_WIDTH_BASE / FocusHighlightPath.getWidthMagnitudeFromTransform( node, correctRootTransform );
}
/**
- * Get the outer line width of a node, based on its scale and rotation transformation.
+ * Get the outer line width of a node, based on its local scale and rotation transformation.
*/
- public static getOuterLineWidthFromNode( node: Node ): number {
- return OUTER_LINE_WIDTH_BASE / FocusHighlightPath.getWidthMagnitudeFromTransform( node );
+ public static getOuterLineWidthFromNode( node: Node, correctRootTransform?: boolean ): number {
+ return OUTER_LINE_WIDTH_BASE / FocusHighlightPath.getWidthMagnitudeFromTransform( node, correctRootTransform );
}
/**
- * Get a scalar width based on the node's transform excluding position.
+ * Get a scalar width based on the node's local transform excluding position. We modify the line width
+ * by the Node's scale so that focus highlights look the same in the HighlightOverlay, regardless of
+ * how the focused component is scaled in the Display.
*/
- private static getWidthMagnitudeFromTransform( node: Node ): number {
- return node.transform.transformDelta2( Vector2.X_UNIT ).magnitude;
+ private static getWidthMagnitudeFromTransform( node: Node, correctRootTransform?: boolean ): number {
+
+ // The listener of the animatedPanZoomSingleton is transforming a root level Node of the display. The
+ // provided Node (from highlightOverlay)
+ const panZoomScale = ( animatedPanZoomSingleton.initialized && correctRootTransform ) ? animatedPanZoomSingleton.listener!.getTargetScale() : 1;
+ return node.transform.transformDelta2( Vector2.X_UNIT ).magnitude / panZoomScale;
}
/**
@@ -325,7 +343,7 @@
// Dilate the group focus highlight slightly more to give whitespace in between the node being highlighted's
// bounds and the inner edge of the highlight.
- const whiteSpaceScalar = 1.4;
+ const whiteSpaceScalar = 1;
return widthOfFocusHighlight * ( scalarToEdgeOfBounds + whiteSpaceScalar );
}
Index: joist/js/PhetButton.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/joist/js/PhetButton.ts b/joist/js/PhetButton.ts
--- a/joist/js/PhetButton.ts (revision 4d15087209c5fadd822ca0e3bf54ecf658836d3f)
+++ b/joist/js/PhetButton.ts (date 1666219172772)
@@ -23,6 +23,7 @@
import Sim from './Sim.js';
import updateCheck from './updateCheck.js';
import UpdateState from './UpdateState.js';
+import Shape from '../../kite/js/Shape.js';
// Accommodate logos of any height by scaling them down proportionately.
// The primary logo is 108px high and we have been scaling it at 0.28 to make it look good even on higher resolution
Index: joist/js/HomeButton.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/joist/js/HomeButton.ts b/joist/js/HomeButton.ts
--- a/joist/js/HomeButton.ts (revision 4d15087209c5fadd822ca0e3bf54ecf658836d3f)
+++ b/joist/js/HomeButton.ts (date 1666219704831)
@@ -82,8 +82,8 @@
// pdom - Pass a shape to the focusHighlight to prevent dilation, then tweak the bottom up just a hair so it
// isn't off the screen.
- const highlightLineWidth = FocusHighlightPath.getOuterLineWidthFromNode( this );
- this.focusHighlight = Shape.bounds( this.bounds.setMaxY( this.bounds.maxY - highlightLineWidth / 2 ) );
+ // const highlightLineWidth = FocusHighlightPath.getOuterLineWidthFromNode( this );
+ // this.focusHighlight = Shape.bounds( this.bounds.setMaxY( this.bounds.maxY - highlightLineWidth / 2 ) );
Multilink.multilink( [ this.interactionStateProperty, navigationBarFillProperty ],
( interactionState, navigationBarFill ) => {
Index: scenery/js/overlays/HighlightOverlay.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery/js/overlays/HighlightOverlay.ts b/scenery/js/overlays/HighlightOverlay.ts
--- a/scenery/js/overlays/HighlightOverlay.ts (revision ef06d9923c358574a40c1c6b657f3a1d9bbf29a5)
+++ b/scenery/js/overlays/HighlightOverlay.ts (date 1666218704864)
@@ -638,16 +638,19 @@
*/
private afterTransform(): void {
if ( this.mode === 'shape' ) {
- this.shapeFocusHighlightPath.updateLineWidth();
+ this.shapeFocusHighlightPath.updateLineWidthWithRootScaleCorrection( this.highlightNode );
}
else if ( this.mode === 'bounds' ) {
- this.boundsFocusHighlightPath.updateLineWidth();
+ this.boundsFocusHighlightPath.updateLineWidthWithRootScaleCorrection( this.highlightNode );
}
- else if ( this.mode === 'node' && this.activeHighlight instanceof FocusHighlightPath && this.activeHighlight.updateLineWidth ) {
+ else if ( this.mode === 'node' && this.activeHighlight instanceof FocusHighlightPath && this.activeHighlight.updateLineWidthWithRootScaleCorrection ) {
// Update the transform based on the transform of the node that the focusHighlight is highlighting.
assert && assert( this.node, 'Need an active Node to update line width' );
- this.activeHighlight.updateLineWidth( this.node! );
+
+ // The highlight Node always has the transform of the TransformTracker, even in cases of layerable, so
+ // it can be used to get the correct line widths
+ this.activeHighlight.updateLineWidthWithRootScaleCorrection( this.highlightNode );
}
}
Index: joist/js/JoistButton.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/joist/js/JoistButton.ts b/joist/js/JoistButton.ts
--- a/joist/js/JoistButton.ts (revision 4d15087209c5fadd822ca0e3bf54ecf658836d3f)
+++ b/joist/js/JoistButton.ts (date 1666219671740)
@@ -18,6 +18,7 @@
import PushButtonModel from '../../sun/js/buttons/PushButtonModel.js';
import HighlightNode from './HighlightNode.js';
import joist from './joist.js';
+import Bounds2 from '../../dot/js/Bounds2.js';
type SelfOptions = {
highlightExtensionWidth?: number;
@@ -122,9 +123,8 @@
// eliminate interactivity gap between label and button
this.mouseArea = this.touchArea = Shape.bounds( this.bounds.dilatedXY( options.pointerAreaDilationX, options.pointerAreaDilationY ) );
- // shift the focus highlight for the joist button so that the bottom is always on screen
const highlightLineWidth = FocusHighlightPath.getOuterLineWidthFromNode( this );
- this.focusHighlight = Shape.bounds( this.bounds.shiftedY( -highlightLineWidth ) );
+ this.focusHighlight = Shape.bounds( content.bounds.dilated( highlightLineWidth / 2 ) );
}
/**
Index: scenery/js/listeners/AnimatedPanZoomListener.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery/js/listeners/AnimatedPanZoomListener.js b/scenery/js/listeners/AnimatedPanZoomListener.js
--- a/scenery/js/listeners/AnimatedPanZoomListener.js (revision ef06d9923c358574a40c1c6b657f3a1d9bbf29a5)
+++ b/scenery/js/listeners/AnimatedPanZoomListener.js (date 1666212638622)
@@ -234,6 +234,18 @@
this.animateToTargets( dt );
}
+ /**
+ * Returns the scale of the target of this MultiListener. Same result as super getCurrentScale, but without
+ * the Vector2 allocation. We know the sourceScale is up to date since it is set to the scale of the current
+ * target as this listener animates.
+ *
+ * @public
+ * @returns {number}
+ */
+ getTargetScale() {
+ return this.sourceScale;
+ }
+
/**
* Attach a MiddlePress for drag panning, if detected.
* @public
|
|
I just noticed this one again while working on alt input for Buoyancy that this is quite awkward. Radio button highlights look very thin compared the the combo box right above it. Perhaps we could loop this issue into Buoyancy work. |
|
This came up again while working on phetsims/scenery-phet#848, it does not look good and is bad for accessibility. Ill try to work on it as part of that work. |
|
I consulted with @arouinfar and @terracoda over slack and we want it to behave like this:
The highlight is transformed by the Node's local-to-global matrix, that is how it scales and transforms correctly. So to correct this issue, we need to keep that matrix, but then correct the line widths afterward by the inverse of it + the pan/zoom matrix and layout scale. |
jessegreenberg
added a commit
that referenced
this issue
Apr 23, 2024
jessegreenberg
added a commit
to phetsims/joist
that referenced
this issue
Apr 23, 2024
jessegreenberg
added a commit
to phetsims/sun
that referenced
this issue
Apr 23, 2024
jessegreenberg
added a commit
to phetsims/friction
that referenced
this issue
Apr 23, 2024
jessegreenberg
added a commit
to phetsims/greenhouse-effect
that referenced
this issue
Apr 23, 2024
|
This was done in the above commits. I reviewed the behavior with @arouinfar and @terracoda, we all did some spot checking to look for regressions. Everything looks OK and CT is not showing problems from this. Closing. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
While working on phetsims/joist#858. From slack:
The text was updated successfully, but these errors were encountered: