Del key command only works if element was last thing clicked on

[KatieWoe]

Test device Samsung Operating System Win 11 Browser Firefox Problem description For https://github.com/phetsims/qa/issues/908. The Del keyboard command only works if you last clicked on the element being focused on. If an element is being focused on, but you've since interacted with something else, such as changing the voltage of the battery, then the Del key won't work, even though the element is still focused. This also occurs in published. Steps to reproduce

1. Drag a battery into the play area.
2. Click the battery so it is focused
3. Drag the battery's voltage slider around
4. Press the Del keyboard key

Troubleshooting information:

!!!!! DO NOT EDIT !!!!! Name: ‪Circuit Construction Kit: DC‬ URL: https://phet-dev.colorado.edu/html/circuit-construction-kit-dc/1.3.0-rc.1/phet/circuit-construction-kit-dc_all_phet.html Version: 1.3.0-rc.1 2023-02-23 22:57:25 UTC Features missing: touch Flags: pixelRatioScaling User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:109.0) Gecko/20100101 Firefox/110.0 Language: en-US Window: 1536x731 Pixel Ratio: 1.25/1 WebGL: WebGL 1.0 GLSL: WebGL GLSL ES 1.0 Vendor: Mozilla (ANGLE (Intel, Intel(R) HD Graphics Direct3D11 vs_5_0 ps_5_0)) Vertex: attribs: 16 varying: 30 uniform: 4096 Texture: size: 16384 imageUnits: 16 (vertex: 16, combined: 32) Max viewport: 32767x32767 OES_texture_float: true Dependencies JSON: {}

[samreid]

@jessegreenberg can you please advise how to address this?

[jessegreenberg]

When selected, the element receives focus. If you click somewhere else in the sim, focus is lost and so the keydown listeners like these will not be received.

One way to fix is to use a global KeyboardListener to implement this behavior regardless of where focus is in the document. Also, this means you don't need a listener per element if you have a reference to the selected component in the ScreenView.

Something like this, in a root level Node like CircuitNode, that can cut and delete everything:

    this.addInputListener( new KeyboardListener( {
      keys: [ 'delete', 'backspace' ],
      global: true,
      callback: ( event, listener ) => {

        // prevent default so 'backspace' and 'delete' don't navigate back a page in Firefox, see
        // https://github.com/phetsims/circuit-construction-kit-common/issues/307
        event?.domEvent?.preventDefault();

        // Double guard to work around errors in fuzzing
        if ( this.vertexCutButton.inputEnabled && this.circuit.getSelectedVertex() ) {
          console.log( 'hello?' );
          this.circuit.cutVertex( this.circuit.getSelectedVertex()! );
        }
      }
    } ) );

And delete all the other keydown listeners.

[samreid]

I added the listener in CCKCScreenView, and it seems to be working OK. @jessegreenberg and @matthew-blackman can you please review?

[matthew-blackman]

This looks good to me and appears to fix the issue on Firefox.

[jessegreenberg]

I did not test, but that listener looks great! One thought - I have caused problems before by adding listeners directly to the ScreenView. It makes every single Node under the ScreenView act like pickable: true. See https://github.com/phetsims/sun/issues/676#issuecomment-765803724 and https://github.com/phetsims/sun/issues/676#issuecomment-767296291.

It may be fine for this sim but wanted to warn. Perhaps CircuitNode or something like that would work just in case to not interfere with the other UI.

[samreid]

@jessegreenberg and @matthew-blackman and I thought it would be problematic to listen to the ScreenView, like it was for https://github.com/phetsims/sun/issues/676#issuecomment-765803724, but we weren't able to trigger any of that incorrect behavior. @jessegreenberg hypothesizes that something improved in scenery recently that may have corrected this.

We would like to move forward with this, and request that QA double check through the pickability of components to see if anything was disrupted.

[jessegreenberg]

@jonathanolson joined us for discussion. We believe that a change to AccordionBox may explain why we are not seeing the same problem as https://github.com/phetsims/collision-lab/issues/195 anymore. But @jonathanolson recommended that we not add listeners to the ScreenView because it can interfere with pickability. One way is to add the listener to a "dummy" Node that is a child of the ScreenView. That way there is no impact on pickability of other ScreenView children. The other option is to add the listener to the CircuitNode or CircuitNode sub-components. But that requires multiple listeners. Another option is to add the listener to the Display. But then the listener would need code to only act on the active ScreenView and circuit elements.

I believe @samreid and @matthew-blackman preferred adding the listener to a dummy Node and will give that a try.

[samreid]

Implemented and it seems to be working well.

[samreid]

For QA, please test in RC.3 according to the instructions in the top comment.

[Nancy-Salpepi]

This looks good for batteries, bulbs, resistors, and fuses in rc.3. Closing.

[Nancy-Salpepi]

I don't think this matters but I noticed that on my Dell, there is a Delete key and a Backspace key. Both successfully delete the circuit element.