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

RectangularButtonView doesn't verify that option values will work with content #448

Closed
pixelzoom opened this issue Jan 16, 2019 · 6 comments
Assignees

Comments

@pixelzoom
Copy link
Contributor

I ran into this over in phetsims/projectile-motion#171, where it manifested in ComboBoxButton, a subclass of RectangularPushButton.

If a RectangularPushButton has atypically small content, it's possible that the default values for cornerRadius, xMargin, and yMargin may result in overlap of the rounded corners. This will be detected and fail way down in Shape.roundedRectangleWithRadii. It would be nice if it were checked in createButtonShape, a private function in RectangularButtonView.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Jan 17, 2019

CT is failing when button content gets too small with "xss-fuzz" test. This causes a "corner overlap" assertion failure in Shape.roundedRectangleWithRadii. Over in phetsims/projectile-motion#171, we decided:

... detect this problem in RectangularPushButton and set cornerRadius:0. Log a warning to console.

@pixelzoom
Copy link
Contributor Author

This just got very complicated. See createButtonShape in RectangularButtonView:

  // convenience function for creating the shape of the button, done to avoid code duplication
  function createButtonShape( width, height, options ) {
    return Shape.roundedRectangleWithRadii( 0, 0, width, height, {
      topLeft: typeof ( options.leftTopCornerRadius ) === 'number' ? options.leftTopCornerRadius : options.cornerRadius,
      topRight: typeof ( options.rightTopCornerRadius ) === 'number' ? options.rightTopCornerRadius : options.cornerRadius,
      bottomLeft: typeof ( options.leftBottomCornerRadius ) === 'number' ? options.leftBottomCornerRadius : options.cornerRadius,
      bottomRight: typeof ( options.rightBottomCornerRadius ) === 'number' ? options.rightBottomCornerRadius : options.cornerRadius
    } );
  }

Here I discovered that RectangularButtonView supports several options related to corner radii. In addition to cornerRadius, there are 4 corner-specific options: leftTopCornerRadius, rightTopCornerRadius, leftBottomCornerRadius, and rightBottomCornerRadius.

Besides the general problems with this reported in #454 ...
This makes checking and adjusting radii much too complicated to bother with, so I'm punting.

@pixelzoom
Copy link
Contributor Author

@jbphet as primary author of RectangularButtonView, do you have any suggestions?

@pixelzoom
Copy link
Contributor Author

Putting this back on the dev meeting list, since we'll need to come up with a different solution.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Jan 31, 2019

I suppose we could use try catch:

function createButtonShape( width, height, options ) {
  let shape = null;
  try {
    shape = Shape.roundedRectangleWithRadii( 0, 0, width, height, {
       // the specified radius options
    } );
  }
  catch {
    console.log( 'WARNING: blah blah blah' );
    shape = Shape.roundedRectangleWithRadii( 0, 0, width, height, {
      cornerRadius: 0
    } );
  }
  return shape;
}

@pixelzoom
Copy link
Contributor Author

2/14/19 dev meeting consensus:

We decided not to do the try catch approach, since we prefer "loud" failures, and a warning might be missed. All of the CT failures were fixed in phetsims/projectile-motion#171 by adjusting cornerRadius without compromising the look of the sims. So we decided to use that approach going forward, and make no changes to RectangularButtonView. If someone runs into this problem again and adjusting cornerRadius can't address it, then we'll reopen this issue (or create a new issue).

Closing.

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

No branches or pull requests

3 participants