KeyboardListener bug is likely when listening to 'tab'

[zepumph]

Discovered with @jessegreenberg over in https://github.com/phetsims/sun/issues/859. We found that ComboBoxListBox was using 'tab' as a key to close the list box, but the new KeyboardListener (added to combo box in https://github.com/phetsims/sun/commit/756d2ef0899b813504b81a654b63b0618795badd) caused an issue because we weren't listening to 'shift+tab' in addition.

The fix for the ComboBoxListBox case is:

Subject: [PATCH] NamedArrayItems now use component references so that renaming a component will correctly update reference in the array
---
Index: js/ComboBoxListBox.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/ComboBoxListBox.ts b/js/ComboBoxListBox.ts
--- a/js/ComboBoxListBox.ts (revision 11c1c103788fdefeed8f8e0b13218980c1ef826f)
+++ b/js/ComboBoxListBox.ts (date 1702922526123)
@@ -249,7 +248,7 @@

     // pdom - listener that navigates listbox items and closes the box from keyboard input
     const keyboardListener = new KeyboardListener( {
-      keys: [ 'escape', 'tab', 'arrowUp', 'arrowDown', 'home', 'end' ],
+      keys: [ 'escape', 'tab', 'shift+tab', 'arrowUp', 'arrowDown', 'home', 'end' ],
       callback: ( event, keysPressed ) => {
         const sceneryEvent = event!;
         assert && assert( sceneryEvent, 'event is required for this listener' );
@@ -258,7 +257,7 @@
         // order.
         const visibleItemNodes = this.getVisibleListItemNodes();

-        if ( keysPressed === 'escape' || keysPressed === 'tab' ) {
+        if ( keysPressed === 'escape' || keysPressed === 'tab' || keysPressed === 'shift+tab' ) {

           // This keyboard event is captured so that escape doesn't forward to other popupable components. If
           // ComboBox is ever implemented with generalized popupable/pane system this abort will not be necessary.

The only other usage of this that is likely a problem is in Dialog.

[zepumph]

Hydrogen MR has been applied this fix to these sims:

center-and-variability 1.1
greenhouse-effect 1.2
my-solar-system 1.3

Next step for me is testing.

@jessegreenberg is going to take another look at KeyboardListener to make sure this is feeling good. We don't really want to change the behavior of 'tab' to also cover 'shift+tab' because that is weird, but we feel it's likely this could come up again. Let's at least add some documentation.

[jessegreenberg]

Sorry about this issue! Thank you for including a fix in the recent maintenance release. I reviewed usages of KeyboardListener and do not see any other problem cases. I added pitfall documentation to KeyboardListener about this.

Leaving open as needed to track the MR but removing my assignment.

[zepumph]

For QA to test:

Tab navigate to a combo box and open it with the keyboard. While on items in the list box, shift tab should close the list box and focus the item before the combo box. It should not go to the previous item in the list.

[zepumph]

This was confirmed fixed by QA for all MR sims with a Combo Box. Closing