use AccordionBox - Githubissues

jessegreenberg commented 7 years ago

For #124, could the collapsible boxes in this sim be changed to use sun/AccordionBox? If so, we would leverage whatever accessibility features are implemented in sun/AccordionBox. @pixelzoom @ariel-phet @emily-phet do you know if there is a reason a custom approach was used in this sim? Not high priority, we likely wont be working on instrumenting this for a little while.

pixelzoom commented 7 years ago

One would think that substituting AccordionBox would be easy - but unfortunately it's not.

The terms in the equations (spinners), the stacks of molecules in the boxes, and the parts of the various "Tools" (selectable from the combo box) all must be horizontally aligned (currently handled by HorizontalAligner). With the current implementation, we have complete control over the width of the boxes. With AccordionBox, we give up control of the box size, because AccordionBox handles placement and margins for its expand/collapse box -- it's currently impossible to make AccordionBox's contentNode fill the box.

After much fiddling (1.5 hours), the screenshot below is as close as I could come using AccordionBox. Compare before and after screenshots. Note that there are various non-trivial horizontal alignment problems here -- arrow, spinners, graph tool,... So I bailed and didn't commit anything.

If we're going to do this, it's going to be a rewrite of how the view layout is handled. Probably in the neighborhood of 10-20 hours. If changes are required to AccordionBox, add another 15 hours or more.

Before (1.1.15): screenshot_900

After: screenshot_386

pixelzoom commented 7 years ago

Here's the patch that resulted in the above screenshot. Save it to a .doc file, then apply in IDEA or WebStorm. When this work was done, balancing-chemical-equations sha was aacc17474f86431d000e8cb969164ffbaa3205da.

Index: js/common/view/BoxesNode.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/common/view/BoxesNode.js (revision aacc17474f86431d000e8cb969164ffbaa3205da)
+++ js/common/view/BoxesNode.js (revision )
@@ -15,13 +15,18 @@
   var BoxNode = require( 'BALANCING_CHEMICAL_EQUATIONS/common/view/BoxNode' );
   var inherit = require( 'PHET_CORE/inherit' );
   var Node = require( 'SCENERY/nodes/Node' );
+  var PhetFont = require( 'SCENERY_PHET/PhetFont' );
   var RightArrowNode = require( 'BALANCING_CHEMICAL_EQUATIONS/common/view/RightArrowNode' );
+  var Text = require( 'SCENERY/nodes/Text' );
   var Vector2 = require( 'DOT/Vector2' );

   // strings
   var productsString = require( 'string!BALANCING_CHEMICAL_EQUATIONS/products' );
   var reactantsString = require( 'string!BALANCING_CHEMICAL_EQUATIONS/reactants' );

+  // constants
+  var TITLE_FONT = new PhetFont( { size: 18, weight: 'bold' } );
+
   /**
    * @param {Property.<Equation>} equationProperty the equation displayed in the boxes
    * @param {Range} coefficientsRange
@@ -40,10 +45,13 @@
       function( equation ) { return equation.reactants; },
       function( equation ) { return aligner.getReactantXOffsets( equation ); },
       coefficientsRange,
-      reactantsBoxExpandedProperty,
       {
+        expandedProperty: reactantsBoxExpandedProperty,
+        titleNode: new Text( reactantsString, {
+          font: TITLE_FONT,
+          maxWidth: 0.75 * boxSize.width
+        } ),
         fill: boxColor,
-        title: reactantsString,
         boxWidth: boxSize.width,
         boxHeight: boxSize.height,
         x: aligner.getReactantsBoxLeft(),
@@ -55,16 +63,22 @@
       function( equation ) { return equation.products; },
       function( equation ) { return aligner.getProductXOffsets( equation ); },
       coefficientsRange,
-      productsBoxExpandedProperty,
       {
+        expandedProperty: productsBoxExpandedProperty,
+        titleNode: new Text( productsString, {
+          font: TITLE_FONT,
+          maxWidth: 0.75 * boxSize.width
+        } ),
         fill: boxColor,
-        title: productsString,
         boxWidth: boxSize.width,
         boxHeight: boxSize.height,
         x: aligner.getProductsBoxLeft(),
         y: 0
       } );

+    console.log( 'reactantsBoxNode.width=' + reactantsBoxNode.width );//XXX
+    console.log( 'productsBoxNode.width=' + productsBoxNode.width );//XXX
+
     // @private right-pointing arrow, in the middle
     this.arrowNode = new RightArrowNode( equationProperty,
       { center: new Vector2( aligner.getScreenCenterX(), boxSize.height / 2 ) } );
Index: js/introduction/view/IntroductionScreenView.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/introduction/view/IntroductionScreenView.js  (revision aacc17474f86431d000e8cb969164ffbaa3205da)
+++ js/introduction/view/IntroductionScreenView.js  (revision )
@@ -31,8 +31,8 @@
   var ToolsComboBox = require( 'BALANCING_CHEMICAL_EQUATIONS/introduction/view/ToolsComboBox' );

   // constants
-  var BOX_SIZE = new Dimension2( 285, 145 );
-  var BOX_X_SPACING = 110; // horizontal spacing between boxes
+  var BOX_SIZE = new Dimension2( 275, 145 );
+  var BOX_X_SPACING = 120; // horizontal spacing between boxes

   /**
    * @param {IntroductionModel} model
Index: js/common/view/BoxNode.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/common/view/BoxNode.js   (revision aacc17474f86431d000e8cb969164ffbaa3205da)
+++ js/common/view/BoxNode.js   (revision )
@@ -10,15 +10,12 @@
   'use strict';

   // modules
+  var AccordionBox = require( 'SUN/AccordionBox' );
   var balancingChemicalEquations = require( 'BALANCING_CHEMICAL_EQUATIONS/balancingChemicalEquations' );
   var BCEConstants = require( 'BALANCING_CHEMICAL_EQUATIONS/common/BCEConstants' );
-  var ExpandCollapseButton = require( 'SUN/ExpandCollapseButton' );
   var inherit = require( 'PHET_CORE/inherit' );
   var Node = require( 'SCENERY/nodes/Node' );
-  var PhetFont = require( 'SCENERY_PHET/PhetFont' );
   var Rectangle = require( 'SCENERY/nodes/Rectangle' );
-  var Shape = require( 'KITE/Shape' );
-  var Text = require( 'SCENERY/nodes/Text' );
   var Vector2 = require( 'DOT/Vector2' );

   /**
@@ -26,65 +23,52 @@
    * @param {function} getTerms 1 parameter {Equation}, gets the terms that this box will display
    * @param {function} getXOffsets 1 parameter {Equation}, gets the x-offsets for each term
    * @param {DOT.Range} coefficientRange range of the coefficients
-   * @param {Property} expandedProperty controls whether the box is expanded or collapsed
    * @param {Object} [options]
    * @constructor
    */
-  function BoxNode( equationProperty, getTerms, getXOffsets, coefficientRange, expandedProperty, options ) {
+  function BoxNode( equationProperty, getTerms, getXOffsets, coefficientRange, options ) {

     var self = this;

     options = _.extend( {
-      buttonLength: 15,
-      xMargin: 5,
-      yMargin: 5,
+
+      boxWidth: 100,
+      boxHeight: 100,
+
+      // AccordionBox options
       fill: 'white',
       stroke: 'black',
       lineWidth: 1,
-      title: '',
-      boxWidth: 100,
-      boxHeight: 100
+      buttonAlign: 'right',
+      buttonLength: 15,
+      buttonXMargin: 5,
+      buttonYMargin: 5,
+      showTitleWhenExpanded: false,
+      titleBarExpandCollapse: false,
+      titleXMargin: 0,
+      buttonTouchAreaXDilation: 20,
+      buttonTouchAreaYDilation: 20,
+      buttonMouseAreaXDilation: 10,
+      buttonMouseAreaYDilation: 10,
+      contentXMargin: 0,
+      contentYMargin: 0,
+      contentXSpacing: 0,
+      contentYSpacing: 0
+
     }, options );

     this.boxHeight = options.boxHeight; // @private
     this.coefficientRange = coefficientRange; // @private
     this.termNodes = {}; // @private molecule nodes for each term, key: term.molecule.symbol, value: [{Node}]

-    // expanded box shows molecules
-    var expandedBoxNode = new Rectangle( 0, 0, options.boxWidth, options.boxHeight,
-      { fill: options.fill, lineWidth: options.lineWidth, stroke: options.stroke } );
-    this.moleculesParent = new Node(); // @private parent for all molecule nodes
-    expandedBoxNode.addChild( this.moleculesParent );
+    // constant-sized rectangle
+    var contentNode = new Rectangle( 0, 0, options.boxWidth, options.boxHeight );

-    // collapsed box shows the title
-    var collapsedBoxNode = new Rectangle( 0, 0, options.boxWidth, options.buttonLength + 2 * options.yMargin, {
-      fill: options.fill,
-      lineWidth: options.lineWidth,
-      stroke: options.stroke
-    } );
-    collapsedBoxNode.addChild( new Text( options.title, {
-      font: new PhetFont( { size: 18, weight: 'bold' } ),
-      center: collapsedBoxNode.center,
-      maxWidth: 0.75 * options.boxWidth  // constrain width for i18n
-    } ) );
+    // @private parent for all molecule nodes
+    this.moleculesParent = new Node();
+    contentNode.addChild( this.moleculesParent );

-    // expand/collapse button
-    var expandCollapseButton = new ExpandCollapseButton( expandedProperty, {
-      sideLength: options.buttonLength,
-      right: expandedBoxNode.right - options.xMargin,
-      top:   expandedBoxNode.top + options.yMargin
-    } );
-    expandCollapseButton.mouseArea = Shape.bounds( expandCollapseButton.localBounds.dilatedXY( 10, 10 ) );
-    expandCollapseButton.touchArea = Shape.bounds( expandCollapseButton.localBounds.dilatedXY( 20, 20 ) );
-
-    // expand/collapse the box
-    expandedProperty.link( function( expanded ) {
-      expandedBoxNode.visible = expanded;
-      collapsedBoxNode.visible = !expanded;
-    } );
-
-    options.children = [ collapsedBoxNode, expandedBoxNode, expandCollapseButton ];
-    Node.call( this, options );
+    AccordionBox.call( this, contentNode, options );

     // update visible molecules to match the coefficients
     var coefficientsObserver = function() {
@@ -104,7 +88,7 @@

   balancingChemicalEquations.register( 'BoxNode', BoxNode );

-  return inherit( Node, BoxNode, {
+  return inherit( AccordionBox, BoxNode, {

     // No dispose needed, instances of this type persist for lifetime of the sim.

jessegreenberg commented 7 years ago

Thanks for looking into this @pixelzoom, understood that it is not a trivial change. We should wait on accessibility in this sim until we can devote time to this issue (recommended in #124) or instrument the accordion boxes as they are now in this sim.

I think the decision depends on whether this rewrite would be worthwhile for reasons other than a11y. I estimate it would take ~1 hour to make the accordion boxes accessible as they are (including keyboard nav, maybe custom focus highlights, adding content for screen readers, hiding the content when the boxes are collapsed). A re-architecture doesn't seem worthwhile for a11y, but if it would be beneficial for other reasons (maybe phet-io?) then a11y should wait.

pixelzoom commented 7 years ago

As discussed in today's status meeting... In general, converting to common UI components is preferable to instrumenting components that are specific to a sim. This will be a longterm win as additional a11y and phet-io features are added. So I recommend that we convert to AccordionBox, and not instrument the "custom accordion boxes" that are in the sim.

pixelzoom commented 6 years ago

Self assigning to revisit this. There is a desire to republish this sim with ePub files by March 2019, see #130. At 11/29/18 status meeting, @emily-phet indicated that it would also be nice to have this conversion done, so that it will be ready for further a11y work.

pixelzoom commented 6 years ago

The discrepancy in the width of the AccordionBox is causing the problem. It's wider than the AccordionBox's content, because when showTitleWhenExpanded: false, the content is put (in this case) to the left of the expand/collapse button.

pixelzoom commented 6 years ago

It took a bunch of fiddling, but I finally cracked the code. The sim is now using AccordionBox, and it's almost "pixel identical" to 1.1.15 (the published version). If you switch between 1.1.15 and 1.2.0-dev.4, you'll see that the expand/collapse button moves down 1 pixel, and the right edge of the accordion box moves to the right 1 pixel.

Below are before/after screenshots with ?showPointerAreas&playAll. Note that in the Game screen, the scoreboard spacing is a bit different due to https://github.com/phetsims/vegas/issues/66.

Before (1.1.15): screenshot_902

After (1.2.0-dev.4): screenshot_901

Before (1.1.15): screenshot_903

After (1.2.0-dev.4): screenshot_904

pixelzoom commented 6 years ago

@arouinfar please verify. The current published version is 1.1.15, the dev version with the changes is 1.2.0-dev.4. You may wish to compare with ?showPointerAreas and/or ?playAll (which plays all challenges in order).

@jessegreenberg @emily-phet FYI, this sim is now ready for a11y.

@kathy-phet FYI, including the previous work that I did on this in September (see above commits), total time for this was ~4 hours.

kathy-phet commented 6 years ago

Sounds worthwhile. I guess you were feeling inspired to do this today. ;)

Sent from my iPhone

On Nov 29, 2018, at 7:04 PM, Chris Malley notifications@github.com<mailto:notifications@github.com> wrote:

@arouinfarhttps://github.com/arouinfar please verify. The current published version is 1.1.15http://localhost/~cmalley/GitHub/balancing-chemical-equations/balancing-chemical-equations_en.html, the dev version with the changes is 1.2.0-dev.4https://phet-dev.colorado.edu/html/balancing-chemical-equations/1.2.0-dev.4/phet/balancing-chemical-equations_en_phet.html. You may wish to compare with ?showPointerAreas and/or ?playAll (which plays all challenges in order).

@jessegreenberghttps://github.com/jessegreenberg @emily-phethttps://github.com/emily-phet FYI, this sim is now ready for a11y.

@kathy-phethttps://github.com/kathy-phet FYI, including the previous work that I did on this in September (see above commits), total time for this was ~4 hours.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/phetsims/balancing-chemical-equations/issues/125#issuecomment-443064077, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AE3FZDHE3rQsvVK6KtCZCiDBSQDPOj_wks5u0JINgaJpZM4PVEMe.

pixelzoom commented 6 years ago

My intuition was telling me that the planets were in the correct alignment. So I went with it.

arouinfar commented 5 years ago

@pixelzoom everything looks good to me!

KatieWoe commented 5 years ago

Something a bit odd that I'm seeing in 1.2.0-rc.1. When you mouse over the top portion of these boxes (either closed or open) the mouse seems to indicate that they are clickable. However, they only collapse/open when you hit the +/- sign. This behavior does not bother itself, but I find the mouse changing when you can't, in fact, click very odd. For https://github.com/phetsims/QA/issues/237. Removing fixed tag for now since it seems there is still an issue. @pixelzoom if you think this behavior is ok feel free to close. Let me know if you think this is a separate issue. acordianboxodd

jessegreenberg commented 5 years ago

Sorry, I just verified that this was caused by https://github.com/phetsims/scenery/issues/888. Now that keyboard input is using the same system as pointer input, the cursor is changing because the title bar has input listeners for a11y.