phetsims / projectile-data-lab

"Projectile Data Lab" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
0 stars 0 forks source link

review of pointer areas #187

Closed samreid closed 6 months ago

matthew-blackman commented 6 months ago

@samreid and I reviewed the pointer areas and found the following remaining work to be done:

We recognize that the interval tool has 2 overlapping pointer areas, but noted that both of these components have the same behavior. Clicking either the interval number or data % number both drag the tool.

samreid commented 6 months ago

Expand the clickable area of the stopwatch launch button, so that it is up to the edges of the bottom and sides of the tool.

I disagree with this, because a user may expect to drag the stopwatch node via that area. Instead, I'll dilate it a bit vertically and horizontally.

samreid commented 6 months ago

I addressed 2 parts. The final part is the sample size radio buttons.

Unfortunately, this patch seems to have no effect:

```diff Subject: [PATCH] Dilate the stopwatch launch button, see https://github.com/phetsims/projectile-data-lab/issues/187 --- Index: js/sampling/view/SectionSampleSize.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/sampling/view/SectionSampleSize.ts b/js/sampling/view/SectionSampleSize.ts --- a/js/sampling/view/SectionSampleSize.ts (revision 5696c6cbf3f33fcdc28c25c5a88bc2442eba7b9a) +++ b/js/sampling/view/SectionSampleSize.ts (date 1709138053022) @@ -44,7 +44,9 @@ }, layoutOptions: { align: 'center' - } + }, + touchAreaXDilation: 30, + touchAreaYDilation: 30 } ); super( ProjectileDataLabStrings.sampleSizeNStringProperty, sampleSizeRadioButtonGroup, providedOptions ); } ```

@pixelzoom any ideas how to accomplish this?

pixelzoom commented 6 months ago

I've confirmed that RectangularRadioButtonGroup's pointer-area options are broken (touchAreaXDilation, touchAreaYDilation, mouseAreaXDilation and mouseAreaYDilation). I tried to fix them, but was unsuccessful. There is so much code in RectangularRadioButtonGroup that is no longer needed (e.g adding invisible rectangles) and should be replaced with scenery layout managers. Recommended to put our efforts there, rather than trying to work around this.

In the meantime... The spacing between the sample size buttons is 6, so you can only dilateX by 3. Why not just make the buttons 3 pixels larger? You have the space to do so. That would side-step this problem.

pixelzoom commented 6 months ago

Here's how far I got with fixing pointer areas in RectangularRadioButtonGroup. The options are at least applied now. But there are still subtle problems that are visible in other sims.

patch ```diff Subject: [PATCH] changed a few color names with MB, https://github.com/phetsims/projectile-data-lab/issues/196 --- Index: projectile-data-lab/js/sampling/view/SectionSampleSize.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/projectile-data-lab/js/sampling/view/SectionSampleSize.ts b/projectile-data-lab/js/sampling/view/SectionSampleSize.ts --- a/projectile-data-lab/js/sampling/view/SectionSampleSize.ts (revision ffc16e88955d57d809691603606394bb2f81379d) +++ b/projectile-data-lab/js/sampling/view/SectionSampleSize.ts (date 1709230691030) @@ -44,7 +44,9 @@ }, layoutOptions: { align: 'center' - } + }, + mouseAreaXDilation: 3, + touchAreaXDilation: 3 } ); super( ProjectileDataLabStrings.sampleSizeNStringProperty, sampleSizeRadioButtonGroup, providedOptions ); } Index: sun/js/buttons/RectangularRadioButtonGroup.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/sun/js/buttons/RectangularRadioButtonGroup.ts b/sun/js/buttons/RectangularRadioButtonGroup.ts --- a/sun/js/buttons/RectangularRadioButtonGroup.ts (revision 25d1dfebf488939f6797390fa5a43f13fa94a1ae) +++ b/sun/js/buttons/RectangularRadioButtonGroup.ts (date 1709231007379) @@ -160,6 +160,8 @@ groupFocusHighlight: true }, providedOptions ); + console.log( `touchAreaXDilation=${options.touchAreaXDilation}` );//TODO + assert && assert( options.soundPlayers === null || options.soundPlayers.length === items.length, 'If soundPlayers is provided, there must be one per radio button.' ); @@ -167,12 +169,6 @@ const radioButtonMap = new Map>(); - // Maximum width of the line that strokes the button. - const maxLineWidth = Math.max( - options.radioButtonOptions.buttonAppearanceStrategyOptions!.selectedLineWidth!, - options.radioButtonOptions.buttonAppearanceStrategyOptions!.deselectedLineWidth! - ); - const nodes = getGroupItemNodes( items, options.tandem ); assert && assert( _.every( nodes, node => !node.hasPDOMContent ), 'Accessibility is provided by RectangularRadioButton and RectangularRadioButtonGroupItem.labelContent. ' + @@ -190,6 +186,12 @@ const xMargin: number = options.radioButtonOptions.xMargin!; const yMargin: number = options.radioButtonOptions.yMargin!; + // Maximum width of the line that strokes the button. + const maxLineWidth = Math.max( + options.radioButtonOptions.buttonAppearanceStrategyOptions!.selectedLineWidth!, + options.radioButtonOptions.buttonAppearanceStrategyOptions!.deselectedLineWidth! + ); + for ( let i = 0; i < items.length; i++ ) { const item = items[ i ]; const node = nodes[ i ]; @@ -203,7 +205,14 @@ tandem: item.tandemName ? options.tandem.createTandem( item.tandemName ) : options.tandem === Tandem.OPT_OUT ? Tandem.OPT_OUT : Tandem.REQUIRED, - phetioDocumentation: item.phetioDocumentation || '' + phetioDocumentation: item.phetioDocumentation || '', + + // Pointer areas must account for the possibility that selectedLineWidth and deselectedLineWidth are different. + // See https://github.com/phetsims/projectile-data-lab/issues/187. + mouseAreaXDilation: options.mouseAreaXDilation + ( maxLineWidth / 2 ), + mouseAreaYDilation: options.mouseAreaYDilation + ( maxLineWidth / 2 ), + touchAreaXDilation: options.touchAreaXDilation + ( maxLineWidth / 2 ), + touchAreaYDilation: options.touchAreaYDilation + ( maxLineWidth / 2 ) }, options.radioButtonOptions, item.options ); // Create the label and voicing response for the radio button. @@ -275,25 +284,10 @@ options.children = buttons; - // Pointer areas and focus highlight, sized to fit the largest button. See https://github.com/phetsims/sun/issues/708. + // Focus highlight is sized to fit the largest button. See https://github.com/phetsims/sun/issues/708. const maxButtonWidth = _.maxBy( buttonsWithLayoutNodes, ( buttonWithLayoutParent: ButtonWithLayoutNode ) => buttonWithLayoutParent.layoutNode.width )!.layoutNode.width; const maxButtonHeight = _.maxBy( buttonsWithLayoutNodes, ( buttonWithLayoutParent: ButtonWithLayoutNode ) => buttonWithLayoutParent.layoutNode.height )!.layoutNode.height; buttonsWithLayoutNodes.forEach( ( buttonWithLayoutParent: ButtonWithLayoutNode ) => { - - buttonWithLayoutParent.radioButton.touchArea = Shape.rectangle( - -options.touchAreaXDilation - maxLineWidth / 2, - -options.touchAreaYDilation - maxLineWidth / 2, - maxButtonWidth + 2 * options.touchAreaXDilation, - maxButtonHeight + 2 * options.touchAreaYDilation - ); - - buttonWithLayoutParent.radioButton.mouseArea = Shape.rectangle( - -options.mouseAreaXDilation - maxLineWidth / 2, - -options.mouseAreaYDilation - maxLineWidth / 2, - maxButtonWidth + 2 * options.mouseAreaXDilation, - maxButtonHeight + 2 * options.mouseAreaYDilation - ); - const defaultDilationCoefficient = HighlightPath.getDilationCoefficient( buttonWithLayoutParent.layoutNode ); buttonWithLayoutParent.radioButton.focusHighlight = Shape.rectangle( -options.a11yHighlightXDilation - maxLineWidth / 2 - defaultDilationCoefficient, ```
samreid commented 6 months ago

This looks related to https://github.com/phetsims/sun/issues/856

samreid commented 6 months ago

In the meantime... The spacing between the sample size buttons is 6, so you can only dilateX by 3. Why not just make the buttons 3 pixels larger? You have the space to do so. That would side-step this problem.

Good idea! I adjusted that in the commit and it looks very nice. I added https://github.com/phetsims/sun/issues/856 to the next developer meeting agenda, to see if we want to schedule a subteam to work on it. Closing.