search

phetsims / geometric-optics

Geometric Optics is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
6 stars 5 forks source link

Interactive highlight doesn't expand with longer screen names in nav bar #470

Closed Nancy-Salpepi closed 1 year ago

Nancy-Salpepi commented 1 year ago

Test device MacBook Air M1 chip

Operating System 13.3.1

Browser Safari and Chrome

Problem description For https://github.com/phetsims/qa/issues/947, but also seen in published MSS:

In the nav bar, when a screen name is long in another locale, the interactive highlight doesn't expand to surround the entire name. Also, the white highlight that usually appears on either side of the screen icon when you hover is only covered by the interactive highlight on one side.

Steps to reproduce

  1. add ?stringTest=dynamic to the end of the url
  2. Go to either screen
  3. In the Visual Tab of of the Preferences Menu, turn on Interactive highlights and exit the menu
  4. Press the right arrow once
  5. Hover mouse over both screen names in the nav bar

Visuals

Screenshot 2023-05-24 at 1 38 43 PM Screenshot 2023-05-24 at 1 07 13 PM
pixelzoom commented 1 year ago

This is a common-code issue, not specific to the simulation. (I wonder - is it even specific to navigation bar buttons?) It seems like something should be resizing the highlight rectangle when a Node's bounds change. But I'm not familiar with how/where interactive highlights are implemented. Let's start by assigning to @jonathanolson and @jessegreenberg to determine how to proceed.

jessegreenberg commented 1 year ago

Thanks for reporting this @Nancy-Salpepi. Generally the highlight does resize to fit the Node's bounds. But that isn't happening here because NavigationBarScreenButton is using a custom highlight that does not update.

pixelzoom commented 1 year ago

Thanks @jessegreenberg, good to know that it's specific to NavigationBarScreenButton, but unfortunate that NavigationBarScreenButton isn't able to use the general Node support for highlight. Who do you think should address this? And would the changes need to be made in NavigationBarScreenButton.ts?

pixelzoom commented 1 year ago

Since this is a general issue, it will be tracked in https://github.com/phetsims/joist/issues/926. When that issue is addressed, we'll cherry-pick the fix to geometric-optics 1.3.

pixelzoom commented 1 year ago

This is ready to proceed with verification and cherry-pick, see https://github.com/phetsims/joist/issues/926#issuecomment-1563554462.

The sha to cherry-pick is https://github.com/phetsims/joist/commit/2e895f32f3e08127c47da57542ad9c1feaf29954.

pixelzoom commented 1 year ago

Patch is completed for geometric-optics 1.3 and geometric-optics-basics 1.3.

To verifying in the next RC, follow the steps in https://github.com/phetsims/geometric-optics/issues/470#issue-1724478680.

pixelzoom commented 1 year ago

Ready for review in 1.3.0-rc.3. Please follow the steps in https://github.com/phetsims/geometric-optics/issues/470#issue-1724478680.

KatieWoe commented 1 year ago

On Win 11 Chrome the bottom of the highlight looks cut off now: image

KatieWoe commented 1 year ago

Other than https://github.com/phetsims/geometric-optics/issues/470#issuecomment-1574012861, which may not be an issue, this looks good on Win 11 Chrome. I'll leave it to you to decide if this is worth addressing @pixelzoom. If not this can probably be closed.

pixelzoom commented 1 year ago

In https://github.com/phetsims/joist/issues/926#issuecomment-1563411208, @jessegreenberg said:

... The highlight shape was changed slightly so that it does not cover up the title text when longer strings get scaled down. The bottom of the highlight is cut off a little but @amanda-phet and I think it is acceptable and the best option with limited space in the nav bar.

But it looks like the bottom of the highlight is entirely cut off on Win11 + Chrome, as reported by @KatieWoe above. @arouinfar @jessegreenberg is that acceptable? Imo it looks lousy.

jessegreenberg commented 1 year ago

The above commit is an improvement that cuts off less. This is what it looks like for me on Win 10 Chrome. image image

@KatieWoe can you try master on Win 11?

pixelzoom commented 1 year ago

Noting that for default screen names, the highlight extends slightly above the navbar. This occurs on all supported browsers for macOS. I have not tested other platforms -- but @jessegreenberg's Win10 screenshot above leads me to believe that this occurs on all platforms. I think this is fine, and definitely better than the previous behavior.

screenshot_2559
KatieWoe commented 1 year ago

Looks better on master to me on Win 11 Chrome too. I did notice that if a letter has a bellow the line aspect it can get covered up. image

jessegreenberg commented 1 year ago

OK, great. If the slight letter cover up is not acceptable I can revert the change. If you like this improvement then https://github.com/phetsims/joist/commit/a1d9d683b6776d1c990a71bde6618ccbe11f54a5 can be cherry-picked into the joist release branch for geometric-optics. @arouinfar what do you prefer?

arouinfar commented 1 year ago

Thanks @jessegreenberg. I think the current behavior on master is an improvement over what happened in https://github.com/phetsims/geometric-optics/issues/470#issuecomment-1574012861. I recommend we keep the changes.

pixelzoom commented 1 year ago

My recommendation for next steps:

If this is the only issue found in 1.3.0-rc.3, then publish 1.3 with this as a known problem. This does not affect usability at all, and it does not seem worth the cost to do another RC.

If on the other hand there are other issues with 1.3.0-rc.3, then I'll cherry-pick this and the change to examples.md in https://github.com/phetsims/geometric-optics-basics/issues/40.

@arouinfar do you agree?

arouinfar commented 1 year ago

QA just finished the spot check, and this was the only issue identified. @pixelzoom, I don't think this merits another RC cycle. We can pick up the fix in the 1.4 release.