restructure home screen buttons

[pixelzoom]

This came up in a Zoom discussion with @arouinfar, @kathy-phet, @zepumph, and @chrisklus.

ph-scale has 3 screen: Macro, Micro, and My Solution. Here's the Studio tree structure for the home screen:

Exposing the large and small buttons separately doesn't seem like the right approach. There should be 1 button for each screen, with a Property that indicates whether it's selected. Or better yet, these should act like (and be structured like) a radio button group.

Maybe something like this:

+ homeScreen
  + activeProperty
  + selectedScreenProperty - {Property.<Screen>}
  + radioButtonGroup
     + macroScreenButton
     + microScreenButton
     + mySolutionScreenButton
  + title

[samreid]

I'm hesitant to use the exact term "radio button group" because clicking on a selected element a 2nd time has a different effect than a traditional radio button group. Maybe just call it screenButtonGroup instead?

I agree with the main idea of this issue. Last time we worked on this, we didn't have time to revise joist to have a single type for each of the buttons (where each type has 2 sizes), but perhaps now we can make time for it.

There is also a lot of a11y code in HomeScreenView.js, we may need help being able to test that we don't disrupt it.

Also, it would be nice to bring HomeScreenView to ES6.

[pixelzoom]

Wow.... Small and large buttons each have their own Text node. So for example I can change a large button's text to "Foo" like this:

And then the small button's text is unchanged:

So if I want to change the name of a screen on the home screen, I have to change it in 2 places.

[pixelzoom]

I took a quick look at the implementation... Thoughts:

• ScreenButton is only used by HomeScreenView. Rename it to HomeScreenButton, for symmetry with NavigationBarScreenButton.

• Do away with the concept of "large" and "small" buttons, replace with a single HomeScreenButton .

• Change the implementation of HomeScreenButton so that it modifies it's appearance based on whether its associated screen (constructor arg) matches HomeScreenModel.selectedScreenProperty.

[chrisklus]

From #597 (comment):

• The name of a screen needs to be changed in three places: the large/small home screen buttons & the navbar. This is problematic (see restructure home screen buttons #601), as the client would need to change a screen name in 3 places. For example:
  homeScreen.view.introScreenSmallButton.text.textProperty
homeScreen.view.introScreenLargeButton.text.textProperty
general.navigationBar.introScreenButton.text.textProperty

CK: This item was improved by this issue but not solved. I'm moving it back to #597 for discussion.

[chrisklus]

progress patch:

Index: js/HomeScreenView.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/HomeScreenView.js	(revision 9586a3026456abab9f0daefe533a882144b74b17)
+++ js/HomeScreenView.js	(date 1582776491000)
@@ -17,7 +17,6 @@
   const merge = require( 'PHET_CORE/merge' );
   const Node = require( 'SCENERY/nodes/Node' );
   const PhetFont = require( 'SCENERY_PHET/PhetFont' );
-  const Property = require( 'AXON/Property' );
   const ScreenButton = require( 'JOIST/ScreenButton' );
   const ScreenView = require( 'JOIST/ScreenView' );
   const StringUtils = require( 'PHETCOMMON/util/StringUtils' );
@@ -80,77 +79,43 @@
     this.addChild( title );
     title.scale( Math.min( 1, 0.9 * this.layoutBounds.width / title.width ) );
 
-    // Keep track of which screen is highlighted so the same screen can remain highlighted even if nodes are replaced
-    // (say when one grows larger or smaller).
-    // TODO: This will be eliminated when we combine the buttons, see https://github.com/phetsims/joist/issues/601
-    const highlightedScreenIndexProperty = new Property( -1 );
+    const screenButtonGroupTandem = tandem.createTandem( 'screenButtonGroup' );
 
     const screenElements = _.map( model.simScreens, function( screen, index ) {
 
       assert && assert( screen.name, 'name is required for screen ' + model.simScreens.indexOf( screen ) );
       assert && assert( screen.homeScreenIcon, 'homeScreenIcon is required for screen ' + screen.name );
 
-      // Even though in the user interface the small and large buttons seem like a single UI component that has grown
-      // larger, it would be quite a headache to create a composite button for the purposes of tandem, so instead the
-      // large and small buttons are registered as separate instances.  See https://github.com/phetsims/phet-io/issues/99
-      const largeTandem = tandem.createTandem( screen.tandem.name + 'LargeButton' );
+      const screenButton = new ScreenButton(
+        screen,
+        model, {
+
+          // Don't resize the VBox or it will shift down when the border becomes thicker
+          resize: false,
+          cursor: 'pointer',
 
-      // a11y
-      const a11yScreenButtonOptions = {
-        tagName: 'button',
-        innerContent: screen.name,
-        descriptionContent: screen.descriptionContent,
-        appendDescription: true,
-        containerTagName: 'li'
-      };
-
-      const largeScreenButton = new ScreenButton(
-        true,
-        screen,
-        model,
-        highlightedScreenIndexProperty,
-        merge( a11yScreenButtonOptions, {
-
-          // Don't resize the VBox or it will shift down when the border becomes thicker
-          resize: false,
-          cursor: 'pointer',
-          tandem: largeTandem
-        } ) );
-
-      // Even though in the user interface the small and large buttons seem like a single UI component that has grown
-      // larger, it would be quite a headache to create a composite button for the purposes of tandem, so instead the
-      // large and small buttons are registered as separate instances.  See https://github.com/phetsims/phet-io/issues/99
-      const smallTandem = tandem.createTandem( screen.tandem.name + 'SmallButton' );
+          // a11y
+          tagName: 'button',
+          innerContent: screen.name,
+          descriptionContent: screen.descriptionContent,
+          appendDescription: true,
+          containerTagName: 'li',
 
-      const smallScreenButton = new ScreenButton(
-        false,
-        screen,
-        model,
-        highlightedScreenIndexProperty,
-        merge( a11yScreenButtonOptions, {
-          spacing: 3,
-          cursor: 'pointer',
-          showUnselectedHomeScreenIconFrame: screen.showUnselectedHomeScreenIconFrame,
-          tandem: smallTandem
-        } ) );
-
-      smallScreenButton.addInputListener( smallScreenButton.highlightListener );
-      largeScreenButton.addInputListener( smallScreenButton.highlightListener );
+          // phet-io
+          tandem: screenButtonGroupTandem.createTandem( screen.tandem.name + 'Button' )
+        } );
 
       // a11y support for click listeners on the screen buttons
       const toggleListener = function() {
-        smallScreenButton.visible && smallScreenButton.focus();
-        largeScreenButton.visible && largeScreenButton.focus();
+        screenButton.visible && screenButton.focus();
       };
-      smallScreenButton.addInputListener( { focus: toggleListener } );
-      largeScreenButton.addInputListener( { click: toggleListener } );
-      // largeScreenButton.mouseArea = largeScreenButton.touchArea = Shape.bounds( largeScreenButton.bounds ); // cover the gap in the vbox
+      screenButton.addInputListener( { focus: toggleListener } );
+      screenButton.addInputListener( { click: toggleListener } );
 
       // a11y - add the right aria attributes to the buttons
-      smallScreenButton.setAccessibleAttribute( 'aria-roledescription', simScreenString );
-      largeScreenButton.setAccessibleAttribute( 'aria-roledescription', simScreenString );
+      screenButton.setAccessibleAttribute( 'aria-roledescription', simScreenString );
 
-      return { screen: screen, small: smallScreenButton, large: largeScreenButton, index: index };
+      return { screen: screen, button: screenButton };
     } );
 
     // a11y this is needed to create the right PDOM structure, the phet menu shouldn't be a child of this 'nav', so
@@ -197,9 +162,9 @@
 
         // check for the current screen
         if ( screenElement.screen === selectedScreen ) {
-          self.highlightedScreenButton = screenElement.large;
+          self.highlightedScreenButton = screenElement.button;
         }
-        return screenElement.screen === selectedScreen ? screenElement.large : screenElement.small;
+        return screenElement.button;
       } );
       hBox = new HBox( {
         spacing: spacing,
Index: js/ScreenButton.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/ScreenButton.js	(revision 9586a3026456abab9f0daefe533a882144b74b17)
+++ js/ScreenButton.js	(date 1582776058000)
@@ -10,6 +10,7 @@
   'use strict';
 
   // modules
+  const DerivedProperty = require( 'AXON/DerivedProperty' );
   const EventType = require( 'TANDEM/EventType' );
   const FireListener = require( 'SCENERY/listeners/FireListener' );
   const Frame = require( 'JOIST/Frame' );
@@ -31,14 +32,12 @@
   const LARGE_ICON_HEIGHT = 140;
 
   /**
-   * @param {boolean} large - whether or not this is a large or small screenButton
    * @param {Screen} screen
    * @param {HomeScreenModel} model REVIEW: `model` threw me, since it's a model specifically for the homeScreen. https://github.com/phetsims/joist/issues/602
-   * @param {Property} highlightedScreenIndexProperty REVIEW: Why is this still based on screen index? https://github.com/phetsims/joist/issues/602
    * @param {Object} [options]
    * @constructor
    */
-  function ScreenButton( large, screen, model, highlightedScreenIndexProperty, options ) {
+  function ScreenButton( screen, model, options ) {
 
     options = merge( {
       showUnselectedHomeScreenIconFrame: false, // put a frame around unselected home screen icons
@@ -48,72 +47,73 @@
       phetioDocumentation: 'A pressable button in the simulation, in the home screen'
     }, options );
 
-    const index = model.simScreens.indexOf( screen );
+    const isSelectedScreenIconProperty = new DerivedProperty( [ model.selectedScreenProperty ], selectedScreen => {
+      return selectedScreen === screen;
+    } );
+
+    // Text for the screen button
+    const text = new Text( screen.name, {
+      font: new PhetFont( isSelectedScreenIconProperty.value ? 42 : 18 ),
+      tandem: options.tandem.createTandem( 'text' )
+    } );
+
+    // The children are needed in the VBox constructor, but the rest of the options should be mutated later.
+    VBox.call( this, {
+      children: [ text ]
+    } );
+
+    isSelectedScreenIconProperty.link( isSelectedScreenIcon => {
 
-    // Maps the number of screens to a scale for the small icons. The scale is percentage of LARGE_ICON_HEIGHT.
-    let smallIconScale = Utils.linear( 2, 4, 0.875, 0.50, model.simScreens.length );
-    if ( model.simScreens.length >= 5 ) {
-      smallIconScale = 0.4;
-    }
+      // Maps the number of screens to a scale for the small icons. The scale is percentage of LARGE_ICON_HEIGHT.
+      let smallIconScale = Utils.linear( 2, 4, 0.875, 0.50, model.simScreens.length );
+      if ( model.simScreens.length >= 5 ) {
+        smallIconScale = 0.4;
+      }
 
-    // Use the small icon scale if this is a small screen button
-    const height = large ? LARGE_ICON_HEIGHT : smallIconScale * LARGE_ICON_HEIGHT;
+      // Use the small icon scale if this is a small screen button
+      const height = isSelectedScreenIcon ? LARGE_ICON_HEIGHT : smallIconScale * LARGE_ICON_HEIGHT;
 
-    // Wrap in a Node because we're scaling, and the same icon will be used for small and large icon, and may be used by
-    // the navigation bar.
-    const icon = new Node( {
-      opacity: options.opacity,
-      children: [ screen.homeScreenIcon ],
-      scale: height / screen.homeScreenIcon.height
-    } );
+      // Wrap in a Node because we're scaling, and the same icon will be used for small and large icon, and may be used by
+      // the navigation bar.
+      const icon = new Node( {
+        opacity: options.opacity,
+        children: [ screen.homeScreenIcon ],
+        scale: height / screen.homeScreenIcon.height
+      } );
 
-    // Frame for small (unselected) home screen icons
-    const frame = large ? new Frame( icon ) : new Rectangle( 0, 0, icon.width, icon.height, {
-      stroke: options.showUnselectedHomeScreenIconFrame ? PhetColorScheme.SCREEN_ICON_FRAME : null,
-      lineWidth: 0.7
-    } );
+      // Frame for small (unselected) home screen icons
+      const frame = isSelectedScreenIcon ? new Frame( icon ) : new Rectangle( 0, 0, icon.width, icon.height, {
+        stroke: options.showUnselectedHomeScreenIconFrame ? PhetColorScheme.SCREEN_ICON_FRAME : null,
+        lineWidth: 0.7
+      } );
 
-    // Create the icon with the frame inside
-    const iconWithFrame = new Node( {
-      opacity: options.opacity,
-      children: [ frame, icon ]
-    } );
+      frame.setHighlighted && frame.setHighlighted( isSelectedScreenIcon );
+      icon.opacity = isSelectedScreenIcon ? 1 : 0.5;
+      text.fill = isSelectedScreenIcon ? 'white' : 'gray';
+
+      // Create the icon with the frame inside
+      const iconWithFrame = new Node( {
+        opacity: options.opacity,
+        children: [ frame, icon ]
+      } );
 
-    // Text for the screen button
-    const text = new Text( screen.name, {
-      font: new PhetFont( large ? 42 : 18 ),
-      fill: large ? PhetColorScheme.BUTTON_YELLOW : 'gray',
-      tandem: options.tandem.createTandem( 'text' )
-    } );
+      text.font = new PhetFont( isSelectedScreenIcon ? 42 : 18 );
 
-    // Shrink the text if it goes beyond the edge of the image
-    text.maxWidth = iconWithFrame.width;
+      // Shrink the text if it goes beyond the edge of the image
+      text.maxWidth = iconWithFrame.width;
 
-    // Only link if a large button
-    const highlightListener = function( highlightedIndex ) {
-      const highlighted = highlightedIndex === index;
-      frame.setHighlighted && frame.setHighlighted( highlighted );
-      icon.opacity = ( large || highlighted ) ? 1 : 0.5;
-      text.fill = ( large || highlighted ) ? 'white' : 'gray';
-    };
-    highlightedScreenIndexProperty.link( highlightListener );
-
-    // The children are needed in the VBox constructor, but the rest of the options should be mutated later.
-    VBox.call( this, {
-      children: [
-        iconWithFrame,
-        text
-      ]
+      this.children = [ iconWithFrame, text ];
     } );
 
     // Input listeners after the parent call depending on if the ScreenButton is large or small
-    const buttonDown = large ? () => {
-                               model.screenProperty.value = screen;
-                               highlightedScreenIndexProperty.value = -1;
-                             } :
-                       () => {
-                         model.selectedScreenProperty.value = screen;
-                       };
+    const buttonDown = () => {
+      if ( isSelectedScreenIconProperty.value ) {
+        model.screenProperty.value = screen;
+      }
+      else {
+        model.selectedScreenProperty.value = screen;
+      }
+    };
 
     const fireListener = new FireListener( {
       fireOnDown: true, // to match prior behavior, but I'm not sure why we have this exceptional behavior
@@ -121,45 +121,24 @@
       tandem: options.tandem.createTandem( 'inputListener' )
     } );
     this.addInputListener( fireListener );
-    this.addInputListener( { click: function() { large && fireListener.fire( null ); } } );
-    this.addInputListener( { focus: function() { !large && fireListener.fire( null ); } } );
-    this.addInputListener( {
-      focus: function() {
-        highlightedScreenIndexProperty.value = index;
-      },
-      blur: function() {
-        highlightedScreenIndexProperty.value = -1;
-      }
-    } );
+    this.addInputListener( { click: function() { isSelectedScreenIconProperty.value && fireListener.fire( null ); } } );
+    this.addInputListener( { focus: function() { !isSelectedScreenIconProperty.value && fireListener.fire( null ); } } );
 
-    // Set highlight listeners to the small screen button
-    if ( !large ) {
-
-      // @public (joist-internal)
-      this.highlightListener = {
-        over: function( event ) {
-          highlightedScreenIndexProperty.value = index;
-        },
-        out: function( event ) {
-          highlightedScreenIndexProperty.value = -1;
-        }
-      };
 
-      // On the home screen if you touch an inactive screen thumbnail, it grows.  If then without lifting your finger
-      // you swipe over to the next thumbnail, that one would grow.
-      this.addInputListener( {
-        over: function( event ) {
-          if ( event.pointer instanceof Touch ) {
-            model.selectedScreenProperty.value = screen;
-          }
-        }
-      } );
-    }
+    // On the home screen if you touch an inactive screen thumbnail, it grows.  If then without lifting your finger
+    // you swipe over to the next thumbnail, that one would grow.
+    this.addInputListener( {
+      over: function( event ) {
+        if ( event.pointer instanceof Touch ) {
+          model.selectedScreenProperty.value = screen;
+        }
+      }
+    } );
 
     this.mouseArea = this.touchArea = Shape.bounds( this.bounds ); // cover the gap in the vbox
 
     this.disposeScreenButton = function() {
-      highlightedScreenIndexProperty.unlink( highlightListener );
+      isSelectedScreenIconProperty.dispose();
     };
 
     this.mutate( options );

I need to fix the text layout, do more cleanup, and recover some lost logic relating to hover behavior - it looks like I misunderstood the purpose of highlightedScreenIndexProperty.

Other than those things, the behavior seems normal and the Studio looks nice. Once they are complete, I'll need assistance to check on a11y behavior from the changes.

[chrisklus]

Implemented above. Tested with screens 1,2,3,4,5,6, screens 1,2,3,4,5, screens 1,2,3,4 etc. of scenery-phet-demo, in comparison to https://phet-dev.colorado.edu/html/scenery-phet/1.0.0-dev.18/phet/scenery-phet_en_phet.html. Also tested with all screens of wave-interference and states-of-matter since they use the option showUnselectedHomeScreenIconFrame for stroking the unselected screen buttons. A11y keyboard navigation also appears to be normal. I haven't tested interactive description.

Test links:

https://phet-dev.colorado.edu/html/scenery-phet/1.0.0-dev.18/phet/scenery-phet_en_phet.html?a11y
http://localhost/scenery-phet/scenery-phet_en.html?brand=phet&ea&a11y

https://phet-dev.colorado.edu/html/wave-interference/2.0.2/phet/wave-interference_en_phet.html
http://localhost/wave-interference/wave-interference_en.html?brand=phet&ea

I ended up just going with buttonGroup for the tandem name for now. screenButtonGroup felt inaccurate since they're now HomeScreenButtons, and homeScreenButtonGroup felt redundant since they're the only buttons on the home screen and homeScreen is in the full phetioID, e.g. gasProperties.homeScreen.view.buttonGroup:

@zepumph please review. I apologize - I forgot that renaming the file in the same commit omits a helpful commit diff. I pasted the before file below if you'd like to compare the new version with clipboard.

// Copyright 2017-2020, University of Colorado Boulder

/**
 * A ScreenButton is displayed on the HomeScreen. There are small and large ScreenButtons, that can be toggled through
 * to select the desired sim screen to go to. See HomeScreenView.js for more information.
 *
 * @author Michael Kauzmann (PhET Interactive Simulations)
 */

import Utils from '../../dot/js/Utils.js';
import Shape from '../../kite/js/Shape.js';
import inherit from '../../phet-core/js/inherit.js';
import merge from '../../phet-core/js/merge.js';
import PhetColorScheme from '../../scenery-phet/js/PhetColorScheme.js';
import PhetFont from '../../scenery-phet/js/PhetFont.js';
import Touch from '../../scenery/js/input/Touch.js';
import FireListener from '../../scenery/js/listeners/FireListener.js';
import Node from '../../scenery/js/nodes/Node.js';
import Rectangle from '../../scenery/js/nodes/Rectangle.js';
import Text from '../../scenery/js/nodes/Text.js';
import VBox from '../../scenery/js/nodes/VBox.js';
import EventType from '../../tandem/js/EventType.js';
import Tandem from '../../tandem/js/Tandem.js';
import Frame from './Frame.js';
import joist from './joist.js';

// constants
const LARGE_ICON_HEIGHT = 140;

/**
 * @param {boolean} large - whether or not this is a large or small screenButton
 * @param {Screen} screen
 * @param {HomeScreenModel} model REVIEW: `model` threw me, since it's a model specifically for the homeScreen. https://github.com/phetsims/joist/issues/602
 * @param {Property} highlightedScreenIndexProperty REVIEW: Why is this still based on screen index? https://github.com/phetsims/joist/issues/602
 * @param {Object} [options]
 * @constructor
 */
function ScreenButton( large, screen, model, highlightedScreenIndexProperty, options ) {

  options = merge( {
    showUnselectedHomeScreenIconFrame: false, // put a frame around unselected home screen icons
    opacity: 1,  // The small screen's nodes have an opacity of .5
    tandem: Tandem.REQUIRED, // To be passed into mutate, but tandem should be a required param in joist
    phetioEventType: EventType.USER,
    phetioDocumentation: 'A pressable button in the simulation, in the home screen'
  }, options );

  const index = model.simScreens.indexOf( screen );

  // Maps the number of screens to a scale for the small icons. The scale is percentage of LARGE_ICON_HEIGHT.
  let smallIconScale = Utils.linear( 2, 4, 0.875, 0.50, model.simScreens.length );
  if ( model.simScreens.length >= 5 ) {
    smallIconScale = 0.4;
  }

  // Use the small icon scale if this is a small screen button
  const height = large ? LARGE_ICON_HEIGHT : smallIconScale * LARGE_ICON_HEIGHT;

  // Wrap in a Node because we're scaling, and the same icon will be used for small and large icon, and may be used by
  // the navigation bar.
  const icon = new Node( {
    opacity: options.opacity,
    children: [ screen.homeScreenIcon ],
    scale: height / screen.homeScreenIcon.height
  } );

  // Frame for small (unselected) home screen icons
  const frame = large ? new Frame( icon ) : new Rectangle( 0, 0, icon.width, icon.height, {
    stroke: options.showUnselectedHomeScreenIconFrame ? PhetColorScheme.SCREEN_ICON_FRAME : null,
    lineWidth: 0.7
  } );

  // Create the icon with the frame inside
  const iconWithFrame = new Node( {
    opacity: options.opacity,
    children: [ frame, icon ]
  } );

  // Text for the screen button
  const text = new Text( screen.name, {
    font: new PhetFont( large ? 42 : 18 ),
    fill: large ? PhetColorScheme.BUTTON_YELLOW : 'gray',
    tandem: options.tandem.createTandem( 'text' )
  } );

  // Shrink the text if it goes beyond the edge of the image
  text.maxWidth = iconWithFrame.width;

  // Only link if a large button
  const highlightListener = function( highlightedIndex ) {
    const highlighted = highlightedIndex === index;
    frame.setHighlighted && frame.setHighlighted( highlighted );
    icon.opacity = ( large || highlighted ) ? 1 : 0.5;
    text.fill = ( large || highlighted ) ? 'white' : 'gray';
  };
  highlightedScreenIndexProperty.link( highlightListener );

  // The children are needed in the VBox constructor, but the rest of the options should be mutated later.
  VBox.call( this, {
    children: [
      iconWithFrame,
      text
    ]
  } );

  // Input listeners after the parent call depending on if the ScreenButton is large or small
  const buttonDown = large ? () => {
                             model.screenProperty.value = screen;
                             highlightedScreenIndexProperty.value = -1;
                           } :
                     () => {
                       model.selectedScreenProperty.value = screen;
                     };

  const fireListener = new FireListener( {
    fireOnDown: true, // to match prior behavior, but I'm not sure why we have this exceptional behavior
    fire: buttonDown,
    tandem: options.tandem.createTandem( 'inputListener' )
  } );
  this.addInputListener( fireListener );
  this.addInputListener( { click: function() { large && fireListener.fire( null ); } } );
  this.addInputListener( { focus: function() { !large && fireListener.fire( null ); } } );
  this.addInputListener( {
    focus: function() {
      highlightedScreenIndexProperty.value = index;
    },
    blur: function() {
      highlightedScreenIndexProperty.value = -1;
    }
  } );

  // Set highlight listeners to the small screen button
  if ( !large ) {

    // @public (joist-internal)
    this.highlightListener = {
      over: function( event ) {
        highlightedScreenIndexProperty.value = index;
      },
      out: function( event ) {
        highlightedScreenIndexProperty.value = -1;
      }
    };

    // On the home screen if you touch an inactive screen thumbnail, it grows.  If then without lifting your finger
    // you swipe over to the next thumbnail, that one would grow.
    this.addInputListener( {
      over: function( event ) {
        if ( event.pointer instanceof Touch ) {
          model.selectedScreenProperty.value = screen;
        }
      }
    } );
  }

  this.mouseArea = this.touchArea = Shape.bounds( this.bounds ); // cover the gap in the vbox

  this.disposeScreenButton = function() {
    highlightedScreenIndexProperty.unlink( highlightListener );
  };

  this.mutate( options );
}

joist.register( 'ScreenButton', ScreenButton );

export default inherit( VBox, ScreenButton, {

  // @public
  dispose: function() {
    this.disposeScreenButton();
    VBox.prototype.dispose.call( this );
  }
} );

[pixelzoom]

This looks awesome in Studio, so much nicer. Thanks for tackling this @chrisklus.

[pixelzoom]

... and buttonGroup seems very appropriate to me.

[zepumph]

I compared

https://phet.colorado.edu/sims/html/number-line-integers/1.0.0/number-line-integers_en.html
http://localhost/number-line-integers/number-line-integers_en.html?brand=phet&ea

and found that, as I moved between the two tabs in different home screen states, they look exactly identical (from what my eyes can see). Good work!

• A local isSelectedProperty seems really nice, and makes it so that the selection when you move back to the home screen from a sim screen after changing from a nav bar button (i.e. homeScreen -> screen2 -> screen1 -> homeScreen) looks correct and up to date.

• It is a bit strange to have the button's model so intertwined with the view of it. Especially since this is a pretty intricate model. Part of me is wondering if we could use ButtonModel.js as a base type for something like DoubleSelectButtonModel, but I'm not sure that would feel any cleaner.

  I looked into this a bit and think that it's simpler to understand and implement in the way it is (over using ButtonModel.js) - let me know if you disagree. If we leave it implemented as is, do you think it'd be worth splitting the model pieces into their own file? I'll copy this down to a new comment.

// The children are needed in the VBox constructor, but the rest of the options should be mutated later.

• I do not think that this is correct. I would recommend passing options in the initial super call.

cursor: 'pointer',
resize: false, // don't resize the VBox or it will shift down when the border becomes thicker

• Where did these options come from? Did you add them or were they from somewhere else that I just missed?

  They were passed in where the ScreenButton's were created, see d46c338#diff-4432907931c61d9acf236f659f648ab8L112 and d46c338#diff-4432907931c61d9acf236f659f648ab8L129.

• I think that renaming iconWithFrame to something with "container" in it sounds clearer to me (personal preference).

  Agreed, thanks.

• The guts of isSelectedProperty.link feel a bit sloppy to me. I don't like seeing 3 ternaries one after another. A few thoughts about it:

  • What if HomeScreenView overwrites updateLayout so that all the function had to do was call update layout and it would handle the layout of everything based on the current value of the isSelectedProperty.

  • What if there was some sort of object you created that housed data for small/large settings. Like:

            { small: { spacing: 0, font: new PhetFont( 18 ) }, large: . . . }
    

    Then you could have one ternary like:

          const data = isSelected? settings.large? setting.small
    

  • wildcard?

  Nice, thanks! I went with the settings object approach.

• It would be nice if you just had one Node to toggle back and forth, like smallNode and largeNode instead of needing to toggle the frame and icon separately. UPDATE: wait a second, doesn't the largeFrame include the largeIcon? Do both need to be added as a child to the container?

  No, largeFrame just uses the icon for layout when creating itself. I added them as children to smallNode and largeNode so just those needed toggling though, thanks.

• I can see on master how with multitouch I can highlight both buttons. This feels totally fine to me. I would make sure to get a designer to sign off on our behavior change for isHighlightedProperty still though to be sure.

  I checked this one off and am copying it to a new comment for a designer to review.

• Please add documentation about why we need to update the mouseArea on bounds change

• Why are you using localBounds instead of the Shape.bounds call before for setting the mouseArea?

  I removed it during testing and it worked because (apparently) mouseArea can be a Shape or Bounds2. I reverted it to use Shape.bounds.

• This block should be moved to inside the screen button:

       // a11y support for click listeners on the screen buttons
    const toggleListener = function() {
      homeScreenButton.visible && homeScreenButton.focus();
    };
    homeScreenButton.addInputListener( { focus: toggleListener } );
    homeScreenButton.addInputListener( { click: toggleListener } );
  
    // a11y - add the right aria attributes to the buttons
    homeScreenButton.setAccessibleAttribute( 'aria-roledescription', simScreenString );
  

• The phetioType for the button is a NodeIO. That is fine with me, but wanted to make sure that felt right to you.

  It sounds good to me.

• I have been a bit out of the loop as it pertains to the PhET-iO design of this component. I can see that the current implementation is a great improvement, but I wonder if it is perfect. Is there any need to instrument the isHighlightedProperty or the isSelectedProperty? I don't think so, but perhaps a designer is needed.

  I will also check this off here and move it to a new comment for designer review.

Good work! Things are looking so much better than before. I appreciate you taking the lead on this one.

[zepumph]

• @chrisklus today during work with interactive descriptions, I noticed that you cannot activate home screen buttons with the keyboard anymore. To reproduce, go to energy-forms-and-changes, add ?supportsDescriptions and then tab to a button. Notice that enter/spacebar don't activate them. That said the logic to "highlight" on tab still works. Let me know if you want help tracking down the bug. This should stay marked as high priority until this is sorted out.

[chrisklus]

I reverted a line that shouldn't have been removed during my refactor and the problem described in #601 (comment) appears to be fixed. @zepumph then gave this a look and we also added events to the keyboard input listeners.

• @zepumph also suggested that I do a line by line comparison from the previous file to make sure any other essential code was not accidentally removed, so I'll do this when addressing the remaining review comments.

[chrisklus]

Thanks for great feedback @zepumph. Please review my responses in #601 (comment) and the commits above. I copied one item down here that I would like more input on:

• It is a bit strange to have the button's model so intertwined with the view of it. Especially since this is a pretty intricate model. Part of me is wondering if we could use ButtonModel.js as a base type for something like DoubleSelectButtonModel, but I'm not sure that would feel any cleaner.

I looked into this a bit and think that it's simpler to understand and implement in the way it is (over using ButtonModel.js) - let me know if you disagree. If we leave it implemented as is, do you think it'd be worth splitting the model pieces into their own file like HomeScreenButtonModel.js?

@arouinfar could you review these two comments from @zepumph?

• I can see on master how with multitouch I can highlight both buttons. This feels totally fine to me. I would make sure to get a designer to sign off on our behavior change for isHighlightedProperty still though to be sure.

• I have been a bit out of the loop as it pertains to the PhET-iO design of this component. I can see that the current implementation is a great improvement, but I wonder if it is perfect. Is there any need to instrument the isHighlightedProperty or the isSelectedProperty? I don't think so, but perhaps a designer is needed.

  CK addition: isHighlightedProperty is true when the button has focus or mouseover, isSelectedProperty is true when the button is "big" on the HomeScreen.

[zepumph]

I looked into this a bit and think that it's simpler to understand and implement in the way it is (over using ButtonModel.js) - let me know if you disagree. If we leave it implemented as is, do you think it'd be worth splitting the model pieces into their own file like HomeScreenButtonModel.js?

I think that it is organized pretty well as is, and don't think more work is needed. The loose structure is something like:

// options

// define Properties

// define Nodes

// Property and input listeners

That feels normal and nice to me. Good work!

[arouinfar]

I can see on master how with multitouch I can highlight both buttons. This feels totally fine to me. I would make sure to get a designer to sign off on our behavior change for isHighlightedProperty still though to be sure.

That also seems fine to me, but not sure if this is moot given the conclusion to the question below...

I have been a bit out of the loop as it pertains to the PhET-iO design of this component. I can see that the current implementation is a great improvement, but I wonder if it is perfect. Is there any need to instrument the isHighlightedProperty or the isSelectedProperty? I don't think so, but perhaps a designer is needed.

CK addition: isHighlightedProperty is true when the button has focus or mouseover, isSelectedProperty is true when the button is "big" on the HomeScreen.

Thanks for the clarification @chrisklus. I don't think either of these properties need to be instrumented. If clients want to know which button is "big" on the HomeScreen, they can see this in homeScreen.model.selectedScreenProperty.

[zepumph]

@chrisklus let me know if I can be of more help here.

[chrisklus]

Thanks @zepumph and @arouinfar, sounds good.

There was one remaining item from that was improved, but not fixed, by the work in this issue. Since the remainder of that problem doesn't relate to the home screen buttons, I'm going to close this issue and move that back to #597 for further discussion.