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

PhET Button sometimes disappears #191

Closed
samreid opened this issue Jan 14, 2015 · 23 comments
Closed

PhET Button sometimes disappears #191

samreid opened this issue Jan 14, 2015 · 23 comments
Assignees

Comments

@samreid
Copy link
Member

samreid commented Jan 14, 2015

Chrome/OSX. Try running Gravity and Orbits, then switch to a sim screen, then back to the home screen. Voila! The PhET button is gone!

@pixelzoom
Copy link
Contributor

Version numbers? I'm running GAO master, and I don't see this on Mac OS 10.9.5 + Chrome 39.

@samreid
Copy link
Member Author

samreid commented Jan 15, 2015

Do you see the triple horizontal line get dimmer?

image

@samreid
Copy link
Member Author

samreid commented Jan 15, 2015

The disappearing logo was happening with the phetsims/brand 'adapted-from-phet' branch--I think it was just using the black logo against the black background.

@samreid
Copy link
Member Author

samreid commented Jan 15, 2015

I do not see this problem in reactants-products-and-leftovers, perhaps it is related to the white navbar in gravity-and-orbits.

@samreid
Copy link
Member Author

samreid commented Jan 15, 2015

I'm on latest master branch of everything, on OSX 10.10.1 and Chrome Version 39.0.2171.95 (64-bit)

@pixelzoom
Copy link
Contributor

@samreid asked:

Do you see the triple horizontal line get dimmer?

Yes, with GAO master, Mac OS 10.9.5, Chrome 39.

@pixelzoom
Copy link
Contributor

In GAO, the navbar background changes when switching between home and other screens. I'm guessing that the PhET menu is being modified once (when a screen is selected) to work on a white background, and then it no longer works on the home screens black background. And if navbar background is screen-specific, then the PhET menu (screen icons? home icon?) needs to be modified every time you switch screens.

@pixelzoom
Copy link
Contributor

Looking at joist code, I would expect GAO to be setting Sim.useInvertedColors = true. But I don't find useInvertedColors in a search of GAO source. How is GOA requesting the inverted navbar?

@pixelzoom
Copy link
Contributor

Ah, I see... useInvertedColors is annotated 'read-only' and is computed in Sim.js:

if ( sim.currentScreen ) {
sim.useInvertedColors = !!new Color( sim.currentScreen.backgroundColor ).equals( Color.BLACK );
}

@pixelzoom
Copy link
Contributor

Here's the bug, in PhetButton:

Property.multilink( [this.interactionStateProperty, sim.useInvertedColorsProperty], function( interactionState, useInvertedColors ) {
      optionsButton.fill = useInvertedColors ? '#222' : 'white';
      phetLabel.image = useInvertedColors ? phetLogoDarker : phetLogo;
    } );

This code isn't taking into account whether we're on the home screen or some other screen. (It's also ignoring the interactionState, which makes me wonder why it's linking to it.)

@pixelzoom
Copy link
Contributor

Proposed fix:

Property.multilink( [ sim.useInvertedColorsProperty, sim.simModel.showHomeScreenProperty ], function( useInvertedColors, showHomeScreen ) {
  var invert = ( useInvertedColors && !showHomeScreen );
  optionsButton.fill = invert ? '#222' : 'white';
  phetLabel.image = invert ? phetLogoDarker : phetLogo;
} );

@pixelzoom
Copy link
Contributor

Btw... The above proposed fix looks (to me) like it resolves this issue. But 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.

@jonathanolson
Copy link
Contributor

My recollection is that each screen has a background color which determines if the colors are inverted (not done on a whole-dim level).

@samreid
Copy link
Member Author

samreid commented Jan 17, 2015

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.

@pixelzoom
Copy link
Contributor

I also think it will be important to rename useInvertedColorsProperty

Agreed. That name is overly general for what it does.

@pixelzoom
Copy link
Contributor

I'm sort of holding off on issues like this until we switch to ohtwo.

Double agreed.

@pixelzoom
Copy link
Contributor

This is still a bug, fix I proposed (#191 (comment)) has not been implemented.

@pixelzoom
Copy link
Contributor

Color-inversion issues moved to #222.

@pixelzoom
Copy link
Contributor

I'm going to implement the fix that I proposed. gravity-and-orbits is broken (see phetsims/gravity-and-orbits#139) so I can't test there. But molecule-shapes exhibits this same problem, so I'll use that to verify.

@pixelzoom
Copy link
Contributor

Fixed as proposed. Assign to @jonathanolson to verify. You can verify using 1 sim that has an inverted navbar (e.g. molecule-shapes) and one that has a black navbar (e.g. RPAL).

@jonathanolson
Copy link
Contributor

Looks good, tested on Molecule Shapes (both color modes) and RPAL. Thanks!

@jonathanolson
Copy link
Contributor

Also a note that it's a bit weird that the currentScreen isn't some sort of home screen (as the HomeScreen is a ScreenView).

@pixelzoom
Copy link
Contributor

Bothers me too, and doesn't seem to be any good reason for the bogus naming. Noted in #224.

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