use ColorProfile in joist

[pixelzoom]

As I noted in https://github.com/phetsims/joist/issues/191#issuecomment-70032427 ...

looking through the joist code, the way that this 'inversion' is handled, and the API for it, is a bit of a mess. There's a lot of coupling that shouldn't be there, there are undocumented assumptions, and the API allows clients to do things that they shouldn't be able to do.

@samreid also noted in https://github.com/phetsims/joist/issues/191#issuecomment-70382536:

I also think it will be important to rename useInvertedColorsProperty to something more straightforward like navbarFillProperty. I'm sort of holding off on issues like this until we switch to ohtwo.

[jonathanolson]

Renaming and cleaning up sounds good. It needs to handle the case when a screen changes its background color (and its colors should be standard) and controls a number of colors, so I'm not sure I like a 'navbarFillProperty'.

[samreid]

I wasn't sure why @jonathanolson concluded "I'm not sure I like a navbarFillProperty", can you elaborate? What if it was just an enum pattern with possible string values white or black?

[jonathanolson]

If it has the same functionality, because it's not just the navbar fill that changes? (e.g. in Molecule Shapes, it's called "Projector Mode" not "White Navbar Mode").

[samreid]

What about calling this the "theme"? The sim theme could be black, white or something else?

[samreid]

I'm investigating factoring out a LookAndFeel instance, which will be a member of Sim, starting with the preliminary step of modeling the fill of the navigation bar. Once things are working and refactored, we can decide on names, etc.

[samreid]

In the above commits, I created and factored out LookAndFeel. I tested with a light sim and a dark sim, and did not see any issues. But @jonathanolson @jbphet @pixelzoom @aaronsamuel137 and @bereaphysics should be on the lookout for issues that may have been introduced here.

[samreid]

I'd also like to move backgroundColor to the LookAndFeel, instead of having it be ethereal in Sim.js.

[samreid]

I moved backgroundColor to the LookAndFeel, and made everything else derived property. Seems much clearer now. I still have a bit more cleanup to do for the icons + highlighting.

[samreid]

I have cleaned up many of the issues pertaining to the color by factoring out a LookAndFeel and using that as the basis for deriving visible properties. However, this comment is somewhat vague:

looking through the joist code, the way that this 'inversion' is handled, and the API for it, is a bit of a mess. There's a lot of coupling that shouldn't be there, there are undocumented assumptions, and the API allows clients to do things that they shouldn't be able to do.

and I'm not sure what else I should address. Can @pixelzoom take a look?

[pixelzoom]

This looks much better.

I would rename LookAndFeel to something like "theme" or "color scheme", because "look and feel" is too broad. From Wikipedia:

In software design, look and feel is a term used in respect of a graphical user interface and comprises aspects of its design, including elements such as colors, shapes, layout, and typefaces (the "look"), as well as the behavior of dynamic elements such as buttons, boxes, and menus (the "feel").

Unnecessary coupling is demonstrated in HomeButton, NavigationScreenButton, PhetButton, JoistButton. They require only navigationBarFillProperty, but they are passed the entire LookAndFeel.

Undocumented assumptions and design flaws:

(1) The navbar is either 'black' or 'white', and it must be one of those 2 strings (not Color.BLACK or Color.WHITE). This is not documented in LookAndFeel, where 'black' and 'white' appear repeatedly. And it's not documented in observers, where this type of test is repeated:

var useDarkenHighlight = navigationBarFill !== 'black';

(2) If indeed 'black' and 'white' are the only 2 colors we'll need for the navbar, then it may be more appropriate to have '{Property.} navigationBarInverted' rather than a color. This might be clearer than the 'black' and 'white' value testing that's currently being done in observers.

(3) The navbar will be 'white' only if the screen is 'black', and it's not clear that this design will make it easy to change that. Might we want a 'white' navbar for other dark background colors?

[samreid]

I would rename LookAndFeel to something like "theme" or "color scheme", because "look and feel" is too broad.

Do you think this object (whatever its name) will eventually accumulate features that are looky-feely? Even if right now, it is only about colors, perhaps in the future we will want to change more features as described in the "Look and Feel" definition?

[samreid]

Unnecessary coupling is demonstrated in HomeButton, NavigationScreenButton, PhetButton, JoistButton. They require only navigationBarFillProperty, but they are passed the entire LookAndFeel.

If in the future, the entire LookAndFeel is providing more information than just colors (say it is fonts + styling, etc) then they may need the entire LookAndFeel, or at least more components from it. Still, for now, I'll pass in only what is necessary.

[samreid]

(1) The navbar is either 'black' or 'white', and it must be one of those 2 strings (not Color.BLACK or Color.WHITE). This is not documented in LookAndFeel, where 'black' and 'white' appear repeatedly. And it's not documented in observers, where this type of test is repeated:

Would @pixelzoom prefer to (a) document existing enum-style pattern, (b) additionally factor out string constants 'white' and 'black' or (c) switch to using Color instances?

(2) If indeed 'black' and 'white' are the only 2 colors we'll need for the navbar, then it may be more appropriate to have '{Property.} navigationBarInverted' rather than a color. This might be clearer than the 'black' and 'white' value testing that's currently being done in observers.

I cannot say for certain whether we will eventually need different navbar colors--but in any case, I think inverted as a terminology was a mistake, because it doesn't indicate what it was inverted from and assumes but does not make explicit a default color. Better would be to use a field like navbarIsBlack. But I think leaving it open-ended as an arbitrary color is very readable and maintainable.

(3) The navbar will be 'white' only if the screen is 'black', and it's not clear that this design will make it easy to change that. Might we want a 'white' navbar for other dark background colors?

Perhaps when this comes up, a good way to solve it would be to make the color derivation function in LookAndFeel an option. Should we eagerly add this feature now or wait until it comes up in context? I somewhat prefer the latter and think it would be straightforward when it comes up.

[pixelzoom]

I realize that it's been awhile, and I haven't gotten to this. If I don't get to it before @samreid's leave, I'll handle it.

[pixelzoom]

No plans to work on this in the near future, so unassigning for now.

[pixelzoom]

Status of this issue is that it's still very relevant. We standardized scenery-phet.ColorProfile. Now we need to use it in joist. And figure out how it relates to LookAndFeel. When we do this is up to project management priorities.

[pixelzoom]

This should also be addressed, from #255:

HomeScreen currently seems to assume that the background will always be 'black'. Perhaps that's an OK assumption. But if that ever changes, we'll need entries for the home screen in LookAndField.

[pixelzoom]

This issue came up again while investigation how to address https://github.com/phetsims/joist/issues/538. If joist used ColorProfiles, that issue would have been trivial to address. Instead, I'm having to wade into this odd LookAndFeel thing.

LookAndFeel is something totally different than ColorProfiles, and it's a bit of a hack, based on whether the Screen's backgroundColorProperty is 'black'. And having joist and sims using different ways of handling color makes little sense to me. So a proposal:

Joist should ditch LookAndFeel , and use color profiles, just like sims. It should have something like JoistColorProfile that describes (and controls) all colors in joist, and would currently include ‘default’ and ‘projector’ profiles.

Things we'd need to discuss:

• how would sims customize joist's color profiles?
• should sims extend joist's color profiles, or have separate sim-specific color profiles?
• should the sim be responsible for switching color profile?
• should other common-code repos have default and projector ColorProfiles?

Labeling for discussion at developer meeting. Assigning to @ariel-phet to prioritize and assign. And I have to be honest; I'm not hugely excited to work on this, mainly because this aspect of joist is such a mess.

[jonathanolson]

I guess I don't understand how the 'default' and 'projector' would work. Some sims have a naturally white background (with a black navigation bar as default), and others have a black background (with a light navigation bar as default).

[pixelzoom]

There should be a default that works in most situations -- i.e., projector mode typically involves lightening up the screen background, and using a dark navigation bar. And the client should be able to override the default part of defining the 'default' and 'projector' profiles.

[jonathanolson]

I guess I'm just concerned that for sims with certain background colors, the sim's "normal" color profile would be paired with the "projector" profile for joist, and vice versa (being somewhat confusing).

[pixelzoom]

It can't be any more confusing (or brittle) than the current "invert" behavior in joist. E.g. https://github.com/phetsims/joist/issues/538 should be simple; instead it's a major project.

[pixelzoom]

11/19/18 dev meeting notes:

• No objections to investigating use of ColorProfile in joist.
• @jbphet Sounds like the right way to go.
• @jonathanolson We should keep the joist profile independent.
• @jonathanolson Having one active profile might not be the correct abstraction.

Self assigning to discuss with @ariel-phet.

[pixelzoom]

Unassigning. After looking around at the state of things, I'm not interested in tackling this.

[ariel-phet]

Looks like this is unlikely to get worked on by anyone anytime soon. There is good discussion in this issue, so going to mark deferred and remove assignees. If Joist gets some love sometime in the future, might be good to tackle then (likely for some a11y work).

[zepumph]

From https://github.com/phetsims/scenery-phet/issues/515, we are getting a very new strategy about our color profiles. Note that this will likely apply to this issue.