Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AccordionBox: remove or instrument interactivity of titlebar #477

Closed
pixelzoom opened this issue Feb 28, 2019 · 13 comments
Closed

AccordionBox: remove or instrument interactivity of titlebar #477

pixelzoom opened this issue Feb 28, 2019 · 13 comments

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Feb 28, 2019

In phetsims/graphing-quadratics#112, the design team identified that interactivity of the AccordionBox's titlebar is problematic, and the consensus is that we'd like to remove that feature. The way to expand/collapse the AccordionBox would be via its ExpandCollapseButton. This would address other issues that titlebar interactively has caused, and simplify AccordionBox's implementation a bit.

I was assigned to create this issue, evaluate the impact on AccordionBox implementation, and
discuss a11y impact with @jessegreenberg.

This issue also affects how we proceed with #480.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Mar 1, 2019

Removing titlebar interactivity from AccordionBox would be very easy, is very isolated, and is very low risk. Remove option titleBarExpandCollapse: true. Then remove this code, starting at line 228:

    if ( options.titleBarExpandCollapse ) {
      this.collapsedTitleBar.addInputListener( {
        down: function() {
          self.phetioStartEvent( 'expanded' );
          self.expandedProperty.value = true;
          self.phetioEndEvent();
        }
      } );
    }

    // Set the input listeners for the expandedTitleBar
    if ( options.showTitleWhenExpanded ) {
      if ( options.titleBarExpandCollapse ) {
        this.expandedTitleBar.addInputListener( {
          down: function() {
            self.phetioStartEvent( 'collapsed' );
            self.expandedProperty.value = false;
            self.phetioEndEvent();
          }
        } );
      }
    }

And there are 2 sims that set option titleBarExpandCollapse:

  • balancing-chemical-equations
  • unit-rates

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Mar 1, 2019

Removing this feature would resolve #423.

There are 2 open issues for AccordionBox a11y design, #359 and #444. General design is described in AccordionBox.md.

It looks like the main change for a11y is the focus highlight. Instead of surrounding the title, the focus highlight would surround the ExpandCollapseButton. Relevant bits from AccordionBox.md:

  • Focus highlight would go around title in both the expanded and collapsed states, if title remains visible.
  • If title visually disappears when expanded, the focus highlight would be limited to the open/close icon and the designer would need to consider extra padding to ensure a reasonable clickable area.
  • It is possible that the focus highlight can change size when toggling between expanded/collapsed states in the scenario when the expanded box does not have a title.

@pixelzoom
Copy link
Contributor Author

@jessegreenberg Let's discuss whenever you're available.

@pixelzoom pixelzoom changed the title AccordionBox: remove interactivity of titlebar AccordionBox: remove or instrument interactivity of titlebar Mar 4, 2019
@pixelzoom
Copy link
Contributor Author

@jessegreenberg and I discussed on Slack. He doesn't see any problem with what's proposed. And he pointed out that removing interactivity will not affect the focus highlight specification in AccordionBox.md, since the titlebar will still be present. So I will proceed.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Mar 4, 2019

@arouinfar was not in the meeting for phetsims/graphing-quadratics#112, so I checked in with her on Slack:

Chris Malley [12:40 PM]
I believe that you’re the only designer who was not consulted this proposed change to AcccordionBox, see #477. The proposal is to remove interactivity of the titlebar, so that you can only expand/collapse using the button. Do you foresee any problems with this?

Amy Rouinfar [12:41 PM]
So long as the touch area on the open/close button is reasonable, that sounds fine with me.
thanks for checking

Chris Malley [12:41 PM]
There will still be full control of pointer areas for the expand/collapse button.

pixelzoom added a commit to phetsims/balancing-chemical-equations that referenced this issue Mar 4, 2019
pixelzoom added a commit to phetsims/unit-rates that referenced this issue Mar 4, 2019
@pixelzoom
Copy link
Contributor Author

titleBarExpandCollapse and its usages have been removed. I tested in a large cross-section of sims that use AccordionBox.

@ariel-phet please assign someone to review. This should be a quick review, the changes were minimal.

@chrisklus
Copy link
Contributor

Code changes look good and I tested a number of AccordionBox usages as well.

@chrisklus chrisklus assigned pixelzoom and unassigned chrisklus Mar 5, 2019
@pixelzoom
Copy link
Contributor Author

By popular demand, we're restoring this feature.

From #502 (comment)...

Consensus is that I will put things back they way it was before #477, and will solve the PhET-iO problem (disabling title interactivity when button is invisible) a different way. I'm going to do the work in #477, which I will reopen.

@pixelzoom
Copy link
Contributor Author

Feature is restored, and interactivity of the title bar is now disabled when the button is hidden.

@jbphet please review, since you have a sim to patch this in.

@pixelzoom
Copy link
Contributor Author

@jbphet to verify that the PhET-iO issue was addressed:

  1. start phetmarks
  2. run graphing-quadratics with Mode=studio
  3. in Studio, navigate to graphingQuadratics.standardFormScreen.view.equationAccordionBox.expandCollapseButton.visibleProperty
  4. Toggle visibleProperty, and verify that the title is only interactive when expandCollapseButton is visible.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented May 2, 2019

Since option titleBarExpandCollapse was restored, I also restored the usage in sims that were overriding the default. There were 2 such sims: BCE and unit-rates. They were restored in the above commits.

@jbphet
Copy link
Contributor

jbphet commented May 2, 2019

The commits look reasonable, and I verified the functionality in Isotopes and Atomic Mass, since that's the sim I want to patch. I then ran test on Graphing Quadratics described a couple of comments above this (I had to run grunt generate-phet-io-elements-files to get the sim to run in studio), and it worked as expected.

I'm going to assign this to @zepumph for one (hopefully) small followup item: The code that was restored uses the following pattern:

          self.phetioStartEvent( 'expanded' );
          self.expandedProperty.value = true;
          self.phetioEndEvent();

I haven't seen the explicit start and stop invocations for a while, and I know that the API has changed a fair amount, so it would be good for a PhET-iO expert to have a look and make sure that this is still the way things are done. If so, please close.

@jbphet jbphet assigned zepumph and unassigned jbphet May 2, 2019
jbphet pushed a commit that referenced this issue May 2, 2019
jbphet added a commit to phetsims/isotopes-and-atomic-mass that referenced this issue May 2, 2019
@zepumph
Copy link
Member

zepumph commented May 7, 2019

This all looks good from a phet-io perspective

I haven't seen the explicit start and stop invocations for a while

Most of the time we are using Property and Action for our event stream. Those types use the "phetioStart/EndEvent" call. In this case we have a custom event so we implement them here, and AccordionBoxIO supports the events in particular (notice AccordionBoxIO.events). Thanks for checking in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants