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

Alt input for HeaterCoolerNode #853

Closed
pixelzoom opened this issue Apr 22, 2024 · 13 comments
Closed

Alt input for HeaterCoolerNode #853

pixelzoom opened this issue Apr 22, 2024 · 13 comments

Comments

@pixelzoom
Copy link
Contributor

Needed for phetsims/gas-properties#213 ...

@arouinfar and I reviewed the current behavior of HeaterCoolerNode. The current behavior with keyboard input needs work, it's not really usable.

With the mouse, you can drag the slider and hold it in one place to heat/cool. When the mouse is released, the slider snaps back to zero.

With the keyboard, the slider snaps back to zero as soon as you release the up/down arrow key. So it's impossible to set the slider to a specific value and "hold" it there. What should probably happen is that the arrow keys are used to set the slider to a specific value, and "0" (and Home? and blur?) makes the slider snap back to zero.

I don't have any info on the history behind the current behavior, whether it was intentionally designed, etc. I suspec that there has been no design, and we're just seeing the default Slider behavior. @jessegreenberg do you have any recollection? How would we specify different behavior for the slider's DragListener vs KeyboardDragListener?

Here's the relevant code, in HeaterCoolerFront.ts. I wonder if it's as simple as only calling setSliderToZero for DragListener, and whether endDrag can tell whether it was called by DragListener or KeyboardDragListener.

    this.slider = new VSlider( heatCoolAmountProperty, sliderRange, combineOptions<SliderOptions>( {
      ...
      endDrag: () => {
        if ( this.snapToZeroProperty.value || sliderIsCloseToZero() ) {
          setSliderToZero();
        }
      },
      ...
    }, options.sliderOptions ) );
@jessegreenberg
Copy link
Contributor

jessegreenberg commented Apr 25, 2024

We can use the event from endDrag to determine where it came from. I just pushed a proposal for a new behavior. For alt input, it will only set back to zero with the blur event.

hat should probably happen is that the arrow keys are used to set the slider to a specific value, and "0" (and Home? and blur?) makes the slider snap back to zero.

We could add another key that sets it back to zero, but I don't think that is necessary.

@pixelzoom what do you think of this change?

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Apr 29, 2024

Thanks @jessegreenberg. This seems to be working nicely. With keyboard input, the slider says where you put it, and snaps back to zero when focus is lost (blur).

But I do think that having a 0 (and Home?) shortcut would make HeaterCoolerNode more usable. A typical interaction using the mouse: I apply heat, overshoot the temperature, release the slider to quick turn of heat, then apply cool to get closer to the desired themperature, then release the slider to quickly turn off cooling. Having to loose keyboard focus to shut off heat/cool makes this type of interaction inconvenient/confusing with the keyboard.

@arouinfar what do you think?

@arouinfar
Copy link

But I do think that having a 0 (and Home?) shortcut would make HeaterCoolerNode more usable.

I agree @pixelzoom. I think it would be very helpful to have a shortcut key to instantly turn off the HeaterCoolerNode. Users may want to heat/cool to a specific temperature without overshooting. Similarly, users may want to cut the heat before the lid explodes, which can happen very quickly. Being able to use a single key press to turn off the HeaterCoolerNode would improve the user experience.

As for the key itself, I think 0 makes a lot of sense and is consistent with FaucetNode. Home would presumably take the slider to its minimum value, which corresponds to maximum cooling.

@arouinfar arouinfar removed their assignment May 1, 2024
@jessegreenberg
Copy link
Contributor

OK, I added a listener to the '0' key for this. We probably need custom keyboard help content for HeaterCoolerNode then? @arouinfar what should that look like?

@jessegreenberg
Copy link
Contributor

jessegreenberg commented May 7, 2024

Here is a patch with something to get started, but the language should probably be improved.

Subject: [PATCH] Update usages of KeyboardListener and KeyboardDragListener after changes from https://github.com/phetsims/scenery/issues/1570
---
Index: js/keyboard/help/SliderControlsKeyboardHelpSection.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/keyboard/help/SliderControlsKeyboardHelpSection.ts b/js/keyboard/help/SliderControlsKeyboardHelpSection.ts
--- a/js/keyboard/help/SliderControlsKeyboardHelpSection.ts	(revision 61197c7b28debdf6c92bc1fb55e358e40a037204)
+++ b/js/keyboard/help/SliderControlsKeyboardHelpSection.ts	(date 1715121293164)
@@ -11,7 +11,7 @@
 import Enumeration from '../../../../phet-core/js/Enumeration.js';
 import EnumerationValue from '../../../../phet-core/js/EnumerationValue.js';
 import optionize from '../../../../phet-core/js/optionize.js';
-import { HBox } from '../../../../scenery/js/imports.js';
+import { HBox, Node } from '../../../../scenery/js/imports.js';
 import sceneryPhet from '../../sceneryPhet.js';
 import SceneryPhetStrings from '../../SceneryPhetStrings.js';
 import TextKeyNode from '../TextKeyNode.js';
@@ -22,7 +22,7 @@
 import assertMutuallyExclusiveOptions from '../../../../phet-core/js/assertMutuallyExclusiveOptions.js';
 
 // Configurations of arrow keys that can be displayed for 'Move between items in a group'
-class ArrowKeyIconDisplay extends EnumerationValue {
+export class ArrowKeyIconDisplay extends EnumerationValue {
   public static readonly UP_DOWN = new ArrowKeyIconDisplay();
   public static readonly LEFT_RIGHT = new ArrowKeyIconDisplay();
   public static readonly BOTH = new ArrowKeyIconDisplay();
@@ -64,6 +64,8 @@
 
   // Determines whether this keyboard help section will have the "Adjust in Larger Steps" row.
   includeLargerStepsRow?: boolean;
+
+  additionalRows?: KeyboardHelpSectionRow[];
 };
 
 export type SliderControlsKeyboardHelpSectionOptions = SelfOptions & KeyboardHelpSectionOptions;
@@ -94,7 +96,8 @@
       adjustInLargerStepsStringProperty: SceneryPhetStrings.keyboardHelpDialog.adjustInLargerStepsStringProperty,
 
       includeSmallerStepsRow: true,
-      includeLargerStepsRow: true
+      includeLargerStepsRow: true,
+      additionalRows: []
     }, providedOptions );
 
     let adjustSliderStringProperty = options.adjustSliderStringProperty;
@@ -224,7 +227,8 @@
       ...( options.includeSmallerStepsRow ? [ adjustSliderInSmallerStepsRow ] : [] ),
       ...( options.includeLargerStepsRow ? [ adjustInLargerStepsRow ] : [] ),
       jumpToMinimumRow,
-      jumpToMaximumRow
+      jumpToMaximumRow,
+      ...options.additionalRows
     ];
 
     super( options.headingStringProperty, content, options );
Index: js/demo/keyboard/KeyboardScreenView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/demo/keyboard/KeyboardScreenView.ts b/js/demo/keyboard/KeyboardScreenView.ts
--- a/js/demo/keyboard/KeyboardScreenView.ts	(revision 61197c7b28debdf6c92bc1fb55e358e40a037204)
+++ b/js/demo/keyboard/KeyboardScreenView.ts	(date 1715121106324)
@@ -18,6 +18,7 @@
 import demoKeyboardHelpIconFactory from './demoKeyboardHelpIconFactory.js';
 import demoFaucetControlsKeyboardHelpSection from './demoFaucetControlsKeyboardHelpSection.js';
 import demoSpinnerControlsKeyboardHelpSection from './demoSpinnerControlsKeyboardHelpSection.js';
+import demoHeaterCoolerControlsKeyboardHelpSection from './demoHeaterCoolerControlsKeyboardHelpSection.js';
 
 type SelfOptions = EmptySelfOptions;
 type KeyboardScreenViewOptions = SelfOptions & PickRequired<DemosScreenViewOptions, 'tandem'>;
@@ -35,6 +36,7 @@
       { label: 'BasicActionsKeyboardHelpSection', createNode: demoBasicActionsKeyboardHelpSection },
       { label: 'ComboBoxKeyboardHelpSection', createNode: demoComboBoxKeyboardHelpSection },
       { label: 'FaucetControlsKeyboardHelpSection', createNode: demoFaucetControlsKeyboardHelpSection },
+      { label: 'HeaterCoolerControlsKeyboardHelpSection', createNode: demoHeaterCoolerControlsKeyboardHelpSection },
       { label: 'KeyboardHelpIconFactory', createNode: demoKeyboardHelpIconFactory },
       { label: 'KeyboardHelpSection', createNode: demoKeyboardHelpSection },
       { label: 'KeyNode', createNode: demoKeyNode },

image

@pixelzoom
Copy link
Contributor Author

pixelzoom commented May 7, 2024

Suggested changes to #853 (comment):

Jump to minimum => Maximum cool [Home]
Jump to maximum => Maximum heat [End]
Reset temperature => Off [0]

@arouinfar
Copy link

arouinfar commented Jun 10, 2024

@jessegreenberg I reviewed the alt input behavior of HeaterCoolerNode in Gas Properties and Energy Forms and Changes. The zero key to turn off the HeaterCoolerNode is a useful shortcut.

EFAC supports the stickyBurners query parameter which turns off the snap-to-zero behavior for HeaterCoolerNode (SoM also supports this). The current alt input implementation works well when the slider snaps to zero on release (like Gas Properties) but does not work when this behavior is turned off, creating an inconsistency between mouse/touch and alt input.

To reproduce in EFAC on main:

  1. Run with supportsInteractiveDescription=true&stickyBurners
  2. Tab to a HeaterCoolerNode and set the slider to any non-zero value.
  3. Tab away from the HeaterCoolerNode and the value snaps back to zero.
  4. With the mouse, set HeaterCoolerNode to any non-zero value. On mouse release, notice the value does NOT snap back to zero.

As for the keyboard help content, I like @pixelzoom's suggestions in #853 (comment). I don't have any other change requests.

@jessegreenberg
Copy link
Contributor

Thanks @arouinfar, in 43a25d2 the snap to zero on blur only happens if snapToZeroProperty is true.

In c4f3651 I added the new keyboard help content. @arouinfar confirmed it looks OK over slack.

image

@pixelzoom can you please review this? In particular the new string keys and changes to SliderControlsKeyboardHelpSection?

@jessegreenberg jessegreenberg removed their assignment Jun 18, 2024
jessegreenberg added a commit to phetsims/my-solar-system that referenced this issue Jun 18, 2024
jessegreenberg added a commit to phetsims/greenhouse-effect that referenced this issue Jun 18, 2024
jessegreenberg added a commit to phetsims/gas-properties that referenced this issue Jun 18, 2024
@jessegreenberg
Copy link
Contributor

jessegreenberg commented Jun 18, 2024

Over slack @pixelzoom recommended renaming these files to be consistent with #714

HeaterCoolerControlsKeyboardHelpSection -> HeatCoolControlsKeyboardHelpSection
TimeControlKeyboardHelpSection -> TimingControlsKeyboardHelpSection

So those renames were done in the above commits.

@pixelzoom
Copy link
Contributor Author

All looks good. See screenshot below for HeatCoolControlsKeyboardHelpSection used in Gas Properties.

Back to @jessegreenberg. Feel free to close if there's all work is done.

screenshot_3379

@jessegreenberg
Copy link
Contributor

OK, thank you for reviewing. Closing.

@terracoda
Copy link

@arouinfar, I have questions on this closed issue.

  • Is the single-character-key "0" repeat functionality for "Home"?
  • Does the "0" shortcut only work if the temperature control has focus?

The reason I ask is that I have recently been reading up on WCAG Success Criteria for keyboard accessibility, specifically 2.1.4 Character Key Shortcuts. Since we do not yet have a way to re-map or turn off shortcuts, single character keyboard shortcuts must require focus.

I think this is how we typically design single-character-key shortcuts, though I may not be aware of all shortcuts.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Jun 20, 2024

@terracoda Answers to your questions below. You can test drive by running Gas Properties, or by running the demo that @jessegreenberg provided.

  • Is the single-character-key "0" repeat functionality for "Home"?

No. As stated in the keyboard help, Home is "Maximum cool" (the minimum slider value, the same as other sliders). Zero is "Off", which for a heater/coooler is 0 value, at the center of the slider range. End is "Maximum heat" (the maximum slider value, the same as other sliders)

Does the "0" shortcut only work if the temperature control has focus?

Only when the control has focus.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants