Color Properties with buttons

[jonathanolson]

I'm having to use MutableOptionsNode in multiple places in Area Model, and this was brought up during the code review. Basically, Scenery accepts fills/strokes (known as 'paints' collectively) to be of multiple types, including Property.<Color|string>. Sun buttons (and associated types like RadioButtonGroup) for the most part do NOT accept arbitrary paint values, most notably color Properties. It's basically a sub-issue of https://github.com/phetsims/sun/issues/290.

The correct way of handling this long-term might be to have a helper function that can convert any paint (or a Property.<paint>) into a Property.<Color> that you could control/dispose, so that any button/view type could:

this.baseColorProperty = Paint.toColorProperty( options.baseColor );

// usage
var someRectangle = new Rectangle( {
  // manipulate the color (the most common reason why color properties aren't supported)
  fill: this.baseColorProperty.link( function( baseColor ) {
    return baseColor.colorUtilsDarker( 0.4 );
  } ),

 // or use it directly
  stroke: this.baseColorProperty
} );

// in disposal
this.baseColorProperty.dispose();

Note that this would NOT be needed if you are just passing paints to fill/stroke, but would only be needed if you need to get color data or variants (like getting the highlight/shadow for a button). (And no, the link in the example above would not leak memory)

The creation of the helper utilities like that shouldn't be too hard, but:

• Integrating this into our buttons and other components will take time. Some cases will require a good amount of refactoring. We'll need to handle backwards compatibility/etc.
• We'll want to make sure the up-front design of how things should work is good in dev meetings
• It's not particularly ideal for performance, but probably won't have too large of an effect. (Cases where the Properties do change will gain in performance, since right now we have to fully recreate a button just to change its color).

So @ariel-phet and @samreid:

1. Do you feel this is the right time to handle this refactoring (to avoid MutableOptionsNode in Area Model and other sims)? (There's a chance MutableOptionsNode will mess with phet-io in the future, so I see it as a workaround for technical debt).
2. If so, can we discuss this at dev meeting, and decide if this is the right approach?
3. If so, should I be the person refactoring common components to use this approach?

(tagging for dev meeting tentatively).

[samreid]

(There's a chance MutableOptionsNode will mess with phet-io in the future, so I see it as a workaround for technical debt).

If the color of a button changes, and that is implemented by disposing a button and creating a new one, will that have any negative ramifications for the parallel dom and accessibility? For example, the screen reader mentioning that a button was removed and another one created? Or do screen readers tend to ignore that kind of thing?

[samreid]

Next question: for Area Model, do attributes other than color change (for instance, the icon or size of the button)? Implementing mutable color may be significantly easier than implementing something that mutates the bounds/size of the button.

[jonathanolson]

If the color of a button changes, and that is implemented by disposing a button and creating a new one, will that have any negative ramifications for the parallel dom and accessibility?

I'd need to defer to @jessegreenberg or @zepumph, but so far it's appeared that you can just focus the "new" object and things generally work OK.

[jonathanolson]

Next question: for Area Model, do attributes other than color change (for instance, the icon or size of the button)?

The current usage of MutableOptionsNode is now only blocked by color changes. I've had to refactor common code for Area Model / Proportion Playground for certain cases where other things needed to change (for instance, resizable accordion boxes).

[zepumph]

I'd need to defer to @jessegreenberg or @zepumph, but so far it's appeared that you can just focus the "new" object and things generally work OK.

That too is my understanding, from one frame to another, it may be 2 different DOMElement objects in memory, but it will have the same id, accessible name, role, etc. such that there will be no issues for the AT client.

[samreid]

I ran this test:

<html>
<head></head>
<body>
<button id="myButton">it's the hello button</button>
<script>
  setTimeout( function() {
    var oldButton = document.getElementById( 'myButton' );
    var parentNode = oldButton.parentNode;
    parentNode.removeChild( oldButton );

    var newButton = document.createElement( 'button' );
    newButton.innerText = 'it\'s the hello button';
    newButton.id = 'myButton';
    parentNode.appendChild( newButton );
  }, 5000 );
</script>
</body>
</html>

I tested with VoiceOver with the button focused, and VoiceOver didn't mention any changes (even when in one test, I changed the text of the button to 'it\'s the hello button!').

[jessegreenberg]

Correct, the AT won't announce changes to the DOM unless the element is marked with aria-live. It will be up to us to restore focus if the button is swapped while it has focus.

EDIT by @samreid: he=>the

[jonathanolson]

Added PaintColorProperty to handle this type of case. Rect/round buttons now accept baseColors that are Properties. RadioButtonGroupAppearance was also reworked to support color properties.

Since this is not necessarily "complete", I'm unassigning instead of closing.

[pixelzoom]

Since this is not necessarily "complete", I'm unassigning instead of closing.

Major unreviewed changes and unassigning?....

Labeling as "block-publication". What work is remaining here? Who will review the changes?

[jonathanolson]

What work is remaining here?

I only integrated support for arbitrary paints to the fields used in area-model. I haven't checked to see how many places do not support arbitrary paints, but there are probably a decent amount. A quick check shows disabledBaseColor in Rectangular/RoundButtonView, stroke in RectangularButtonView, etc.

Who will review the changes?

I'll leave that to @ariel-phet to answer. I definitely should have assigned for review.

[jbphet]

I was asked to review the changes made so far, here are my comments:

• Test code should be added to the Sun demo that exercises these changes, please see ButtonsScreenView.js, particularly the variable changeButtonColorsButton which currently tests the ability to change button colors.
• The header is marked as TODO for PaintColorProperty.js. This should be thoroughly documented, including why it is necessary to have this type instead of just wrapping a paint in a standard Axon property.
• PaintColorProperty.js has a private variable named _.factor. This is a vague name, its purpose is not documented, and it appears from the GitHub history that it was previously named "brightnessFactor". It is indeed used to to brighten the paint, but only in the invalidatePaint method, which seems suspicious in itself. If the intent was to always change the paint, I'm not sure that this would work in the case of a max white value. And why would this factor need to be settable? Why does the header comment for invalidatePaint say "Update the value of this property"? At the least, this all needs documentation, but it should also get some scrutiny to make sure the intent is being met.
• In `Paint.isPaint()', the raw type can be string, Color, or Paint, but the wrapped type can only be string or Color. Is this correct?

Also, @jonathanolson said:

I only integrated support for arbitrary paints to the fields used in area-model.

I understand that there is an immediate need for Area Model (though a lot of it seems to be based on the incorrect assumption that button color could not be changed), but it doesn't seem like a good idea to do this partially and then leave the issue unassigned. If there is more to be done, such as for the disabled state mentioned above, I'd advise that we do it now while it is fresh in our collective minds.

[pixelzoom]

@jbphet said:

I understand that there is an immediate need for Area Model (though a lot of it seems to be based on the incorrect assumption that button color could not be changed), but it doesn't seem like a good idea to do this partially and then leave the issue unassigned. If there is more to be done, such as for the disabled state mentioned above, I'd advise that we do it now while it is fresh in our collective minds.

+1. Partially implementing something to meet a need of some sim, then leaving it (often unassigned) in a temporary/experimental state happens (in my opinion) too often. Not to pick on JO, but https://github.com/phetsims/phet-core/issues/35 (improved Pooling) is another recent example. And last time I check, there were > 100 in-progress issues with the "dev:a11y" label.

[jonathanolson]

Test code should be added to the Sun demo that exercises these changes, please see ButtonsScreenView.js, particularly the variable changeButtonColorsButton which currently tests the ability to change button colors.

Done, both mutating the Property value and individual Color channels independently.

The header is marked as TODO for PaintColorProperty.js. This should be thoroughly documented, including why it is necessary to have this type instead of just wrapping a paint in a standard Axon property.

Yikes, I'm not sure how I let that slip through. I've added documentation at the top.

PaintColorProperty.js has a private variable named _.factor. This is a vague name, its purpose is not documented, and it appears from the GitHub history that it was previously named "brightnessFactor".

I asked on Slack (dev-public):

Anyone have better name ideas for a “brightness factor” for a color, where 0 is the color itself, 1 is white, and -1 is black? and ranges in-between are a brightened or darkened version of the color

@samreid did prefer something other than "factor", but his recommendation of "brighteningAmount" also had the same problem of my example, that it sounded like it couldn't be darkened.

I'd be more than happy to have a better name. I considered things like exposure, adjustment, etc., but I'm fine with whatever as long as it doesn't sound like it can't darken colors too.

It is indeed used to to brighten the paint

And darken!

but only in the invalidatePaint method, which seems suspicious in itself. Why does the header comment for invalidatePaint say "Update the value of this property"? At the least, this all needs documentation, but it should also get some scrutiny to make sure the intent is being met.

It's an internal method that gets called whenever the value of the Property may be incorrect (usually because the color is likely to have changed), thus "invalidate". It computes the color from our paint and sets our Property value. Can you recommend an improved name or documentation?

If the intent was to always change the paint, I'm not sure that this would work in the case of a max white value.

I'm not sure I understand, can you clarify? new phet.scenery.PaintColorProperty( 'white', { factor: 1 } ).value is white, so it seems to work with the max white value.

And why would this factor need to be settable?

Mutability is desirable because it's trivial to implement and I can imagine cases where we'll want to "fade" colors in an animation.

In `Paint.isPaint()', the raw type can be string, Color, or Paint, but the wrapped type can only be string or Color. Is this correct?

Yes, paints do not allow arbitrary nesting, so you can't say fill: new Property( new Property( new Property( 'red' ) ) ) (it will error out). However there are other paints (null, gradients, etc.) not listed above, and those also cannot be wrapped with a Property.

but it doesn't seem like a good idea to do this partially and then leave the issue unassigned. If there is more to be done, such as for the disabled state mentioned above, I'd advise that we do it now while it is fresh in our collective minds. +1. Partially implementing something to meet a need of some sim, then leaving it (often unassigned) in a temporary/experimental state happens (in my opinion) too often.

Is it sufficient to ensure all sun/scenery-phet components can accept arbitrary paints, or does this also need to be done for other common repositories? Presumably I'm not going to modify sim-specific components?

I'm leaving myself assigned for patching sun/scenery-phet components

[jonathanolson]

I've updated a number of different places/components in the commits above.

I still need to handle:

• [x] ProbeNode
• [x] ShadedRectangle
• [x] SpinningIndicatorNode
• [x] VisibleColor

Once I'm done I'll reassign for review of all of the commits (since I'm having to touch a lot of core components and code).

[jonathanolson]

Refactoring also removed updateFillsAndStrokes from RoundButtonView/RectangularButtonView.

One question I had was based on the old disabledStroke for RoundButtonView (3d appearance). It looks like it was initialized to a conditional value, but was overwritten before being used by anything else. Extra review from @jbphet to make sure the current form of this is OK (since I removed the unused value).

I'll also need to run some memory leak testing to make sure I didn't introduce anything.

[jonathanolson]

Additionally, I'll want feedback on the variable names for the "derived" color properties. They could potentially be more verbose/clear.

[jonathanolson]

Refactored the remaining cases above (VisibleColor didn't look like it warranted changes).

[jonathanolson]

Memory testing looked OK. Assigning to @jbphet for review of above commits.

[samreid]

I noticed the shading of ShadedRectangle changed recently. Here is https://phet-dev.colorado.edu/html/wave-interference/1.0.0-dev.13/phet/wave-interference_en_phet.html

And here is master:

Was this change intentional?

[jonathanolson]

Not intentional, I'll look into the break.

[jonathanolson]

@samreid can you see if the above fix resolves all of the issues? I hadn't properly negated things.

[samreid]

Here's a screenshot after the preceding commit:

Looks great to me, thanks! Leaving assigned to @jbphet as described in https://github.com/phetsims/sun/issues/362#issuecomment-400017122

[jbphet]

Looking good. Here are my comments from the 2nd round of review:

• I agree that factor is too generic of a term. I looked around online, and my suggestions would be to use one of the following (see http://www.workwithcolor.com/color-properties-definitions-0101.htm):
  • luminanceFactor
  • luminanceChangeFactor
  • brightnessChangeFactor
  • tintFactor
  • tintAndShadeFactor
• Since 'PaintColorPropertyis being used in a lot of common code, it should probably be used in the demo too. In https://github.com/phetsims/sun/commit/e5cf9a5871824409ed89e0b1b1deff1ad339d711, a number of color properties were added, but at this point they should probably becomePaintColorProperty` instances (right?).
• There are a number of places in common code where PaintColorProperty instances were added for the color values, but the values aren't public and don't have setters, so they can't be changed. I don't understand the point of doing this. If the idea is that now they can be changed, the documentation should be changed to reflect this. I noticed this in ShadedRectangle.js and SpinningIndicatorNode.js, but there are likely other instances as well.

[jonathanolson]

I agree that factor is too generic of a term.

I'll make the change to luminanceFactor then if that sounds good?

Since 'PaintColorPropertyis being used in a lot of common code, it should probably be used in the demo too. In https://github.com/phetsims/sun/commit/e5cf9a5871824409ed89e0b1b1deff1ad339d711, a number of color properties were added, but at this point they should probably becomePaintColorProperty instances (right?).

In that particular case, it was mirroring how our color profile properties work (since the buttons will need to use PaintColorProperty to handle the property in all cases). I don't see how making those PaintColorProperties would help. Might be good to discuss on a call?

There are a number of places in common code where PaintColorProperty instances were added for the color values, but the values aren't public and don't have setters, so they can't be changed.

But they support passing in Property.<Color|string> for e.g. baseColor in ShadedRectangle, and that can change. It would probably be good to discuss on a call.

[jonathanolson]

Discussed with @jbphet.

I'll make the luminanceFactor name change, and the rest was handled with documentation changes.

[jonathanolson]

Implemented above, closing.