phetsims / fourier-making-waves

"Fourier: Making Waves" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
6 stars 3 forks source link

Waveform combo box does not change to 'custom' when amplitude slider is adjusted with keyboard nav. #200

Closed Nancy-Salpepi closed 2 years ago

Nancy-Salpepi commented 2 years ago

Test device iMac

Operating System 10.15.7

Browser chrome

Problem description https://github.com/phetsims/qa/issues/711

In the Discrete Screen, when using keyboard nav, the waveform type doesn't switch to custom when any of the amplitude sliders are manipulated.

Steps to reproduce

  1. In the discrete screen, choose any waveform type (except custom) from the combo box.
  2. Tab to one of the amplitude sliders and adjust it.

Visuals waveformtype

Troubleshooting information: !!!!! DO NOT EDIT !!!!! Name: ‪Fourier: Making Waves‬ URL: https://phet-dev.colorado.edu/html/fourier-making-waves/1.0.0-rc.1/phet/fourier-making-waves_all_phet.html Version: 1.0.0-rc.1 2021-09-28 15:44:23 UTC Features missing: applicationcache, applicationcache, touch User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/94.0.4606.71 Safari/537.36 Language: en-US Window: 1288x875 Pixel Ratio: 1/1 WebGL: WebGL 1.0 (OpenGL ES 2.0 Chromium) GLSL: WebGL GLSL ES 1.0 (OpenGL ES GLSL ES 1.0 Chromium) Vendor: WebKit (WebKit WebGL) Vertex: attribs: 16 varying: 31 uniform: 1024 Texture: size: 16384 imageUnits: 16 (vertex: 16, combined: 32) Max viewport: 16384x16384 OES_texture_float: true Dependencies JSON: {}
pixelzoom commented 2 years ago

Wow, good catch @Nancy-Salpepi. I guess there's yet-another thing special/different that has to be done for keyboard navigtion + Slider.

pixelzoom commented 2 years ago

Notes to self:

In DiscreteScreenView, the Amplitudes chart is instantiated with option onEdit which switches the Waveform to 'custom' when any amplitude is changed by the user:

    const amplitudesChartNode = new DiscreteAmplitudesChartNode( model.amplitudesChart, amplitudeKeypadDialog, {

      // Changing any amplitude switches the waveform to 'custom'.
      onEdit: () => { model.waveformProperty.value = Waveform.CUSTOM; },
      tandem: amplitudesTandem.createTandem( 'amplitudesChartNode' )
    } );

In InteractiveAmplitudesChartNode (the superclass of DiscreteAmplitudesChartNode) onEdit is passed to the Slider and NumberDisplay with the press option:

    const sliders = fourierSeries.harmonics.map( harmonic =>
      new AmplitudeSlider( harmonic, emphasizedHarmonics, merge( {
        press: options.onEdit,
...
    const numberDisplays = fourierSeries.harmonics.map( harmonic =>
      new AmplitudeNumberDisplay( harmonic, emphasizedHarmonics, amplitudeKeypadDialog, merge( {
        press: options.onEdit,

In AmplitudeSlider, press is passed to a PressListener. There is something similair in AmplitudeNumberDisplay.

   this.addInputListener( new PressListener( {
      attach: false,
      press: options.press
    } ) );

So it looks like keyboard input does not result in firing of PressListeners. Is that expected behavior @jessegreenberg ? If so... What doesn't PressListener handle keyboard input? And what do I need to add to duplicate this behavior for keyboard input.

jessegreenberg commented 2 years ago

This is what I would expect, this is very similar to https://github.com/phetsims/fourier-making-waves/issues/87. PressListener does support activation with alternative input, but not arrow key presses like this.

Instead of a PressListener would it be possible to use the startDrag option of Slider.js? That will be called from all forms of supported input.

pixelzoom commented 2 years ago

PressListener does support activation with alternative input, but not arrow key presses like this.

@jessegreenberg If I want to listen for arrow key presses, what should I use? I'm surprised that there's nothing like "KeyboardListener".

pixelzoom commented 2 years ago

Fix pushed to master and 1.0 branches. I used @jessegreenberg's suggestion -- I replaced PressListener with option startDrag. That was complicated by the fact that I have a custom track and thumb. It would have been much more straightforward to add something like "KeyboardListener", but no such thing seems to exist.

@Nancy-Salpepi would you please give this a test in master? Then assign back to me.

Nancy-Salpepi commented 2 years ago

Looks good on Master using MacBook (11.6) + chrome/safari and iMac (10.15.7) + chrome.

pixelzoom commented 2 years ago

Thanks @Nancy-Salpepi.

Ready for testing in the next RC. To verify:

  1. Run the sim, go to Discrete screen
  2. Set "Waveform" combo box to any value except "custom"
  3. Navigate to one of the amplitude sliders with the keyboard.
  4. Verify that changing the amplitude slider via the keyboard causes "Waveform" to change to "custom"
Nancy-Salpepi commented 2 years ago

This works as it should following the steps in https://github.com/phetsims/fourier-making-waves/issues/200#issuecomment-933714636