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 is broken for momentary buttons. #796

Open
pixelzoom opened this issue Oct 3, 2022 · 27 comments
Open

Alt input is broken for momentary buttons. #796

pixelzoom opened this issue Oct 3, 2022 · 27 comments

Comments

@pixelzoom
Copy link
Contributor

I have a RoundMomentaryButton in ph-scale. It's the button on scenery-phet DropperNode, and looks like this:

screenshot_1901

RoundMomentaryButton and RectangularMomentaryButton) do not currently work correctly with alt input. A momentary button is a toggle button that is “on” on pointer down, then “off” on pointer up. The equivalent for keyboard seems like it would be “on” when the spacebar is pressed, “off” when the spacebar is released, but I don’t know if that’s possible.

This is a prerequisite for ph-scale, which is included in the PhET-iO batch release. And I believe that ph-scale is towards the top of the deploy list.

Assigning to @jessegreenberg and @zepumph. Feel free to reassign to whoever is responsible for alt input of sun buttons, or consult with @kathy-phet.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Oct 3, 2022

Sky#a11y-dev discussion:

Michael Kauzmann
I don't believe that we have gotten to momentary buttons yet, checking

Michael Kauzmann
I see that on space, it is the keyup that triggers the button, but with enter, it is key down, so by holding enter, you get momentary behavior. Is that what you see?

Chris Malley
Yeah, I’m digging into it. Looks like there were some assumptions made about buttons that didn’t consider this type of button. Adding this to RoundMomentaryButton’s default options:

      listenerOptions: {
        press: () => console.log( 'press' ),
        release: () => console.log( 'release' )
      },

Both callbacks fire when I release the spacebar. (edited)

Michael Kauzmann
But it does work with enter.

Chris Malley
Checking… And is that how you want alt input to work for momentary buttons, or just a happy coincidence? Doesn’t seem very intuitive.

Chris Malley
Using Enter, I see the button toggling multiple times, seeming randomly. Certainly not usable.

Chris Malley
To clarify… I’m pressing and holding just the Enter key, then releasing it when I want the button to toggle off.

Chris Malley
And the button does change state, but its listeners do not fire. In the case of the dropper, no solution comes out of the dropper while the button is in the “on” state.

Michael Kauzmann
I see. Ok good to know. I didn't realize that it didn't let a small amount out each time the enter button fired.

Chris Malley
Can you explain why a PressListener press callback fires when the spacebar is released? Is that expected? It seems like listenerOptions (above) is the way this should be addressed.

Michael Kauzmann
We are at the mercy of how browsers deliver their "click" event. This is given on up for space, and down for enter. If we wanted to heavily optimize, you'd THINK we could listen to keyup and keydown, but when using screen readers, we don't have that information, just the click event.

Chris Malley
I see. Thanks for clarifying.

Michael Kauzmann
So the bug for that issue is to make sure that just a bit can come out based on a single-point callback, instead of relying on the start/stop events being at least >0ms apart. (I think)

Michael Kauzmann
But I also just have a feeling that there is a reason why we haven't gotten things working just yet. My guess is that it may open a can o insert favorite gross thing.

Chris Malley
I’m not clear on what you mean by “just a bit coming out”. The user should be able to “hold the button down” (however that is done via the keyboard) for as long as they want.

Michael Kauzmann
Right, but in terms of events, for mouse it is "on" and then "off" but for keyboard it is just "click", and then if you hold it down it is "click, click, click, click, click, click, click, click, click". So we need to discretize that so that the button responds in a small way as being "on" for a bit of time each time there is a click event.

Chris Malley
It also seems like a usability problem to use spacebar for most buttons, and Enter for momentary buttons. As well as having implications for the “Basic Actions” section of keyboard help, where this use of Enter is currently not mentioned. (edited)

Chris Malley
Perhaps we should consider making momentary toggle buttons behave like standard toggle buttons, for alt input. Spacebar on, spacebar off.

Amy Rouinfar
I like the idea of having momentary buttons behave like standard toggle buttons for alt input.

Michael Kauzmann
Perhaps with an option like "timePerAltInputPress" with a good default

@terracoda
Copy link
Contributor

I am so glad we are looking at the dropper button interaction, aka "momentary button".

The PhET code base has many more button interactions than want can be represented natively in HTML, so we might need to think outside the box a little.

In a browser, both Enter and Space are keys that activate buttons. And as you noticed they do not function in exactly the same way. In contrast, only Enter can activate a link and only Space toggles a checkbox. These associations with actual HTML control elements are well-known to people who rely on the keyboard.

If the control is communicated visually as a button (and in the code as a button), both the Enter and Space keys will need to work with the button. Keyboard users will likely try either or both.

I think we need to think about what the function of the eye-dropper is meant to allow the learner to accomplish, e.g. constant stream of drops, and/or a quick single drop.

I think we can make the Enter key work differently from the Space key. I think we did that with the Step button in the Timing Control Node.

Let's ask what does the eye-dropper button need to do and then see if that function can be mapped to the native key presses that work for an HTML button interaction.

@terracoda
Copy link
Contributor

And by HTML button, I mean, any HTML code that is communicated as a button, for example (and this is rough):

<button>Eye-dropper</button>, communicated as "Eye-dropper, button"
<button aria-pressed="false">Eye-dropper</button>, communicated as "Eye-dropper, toggle button"
<button aria-pressed="true">Eye-dropper</button>, communicated as "Eye-dropper, selected toggle button"
<button aria-pressed="false" role="switch">Eye-dropper</button>, communicated as "Eye-dropper, off, switch"
<button aria-pressed="true" role="switch">Eye-dropper</button>, communicated as "Eye-dropper, on, switch"

@terracoda
Copy link
Contributor

We can make custom widgets that look like buttons. It's harder to make a custom widget sound good with a screen reader, so if we can find a way for a button to work well for the eye-dropper interaction, I think that will be a good thing.

@terracoda
Copy link
Contributor

terracoda commented Oct 4, 2022

I am wondering if Enter can turn it on/off to squirt a little and holding Space can be used to fire a continuous stream? Would that be a good thing for users and work in a browser?

@terracoda
Copy link
Contributor

Oh, that should be the other way around and more like this:

  • Enter & Space might both turn the eye-streamer button on/off to squirt a little - this would require 2 toggles - once to on, and once to off.
  • Pressing & holding the Enter key would allow for a continuous stream.
  • Pressing & holding Space key doesn't do anything as a key-up is required to fire an interaction with Space key.

The questions I have are:

  1. Would the following key presses make sense for the sim and learners:
    - the Space key behaves as per usual toggling the button on then off with and key-up event, and then
    - the Enter key behaves more like the mouse/touch interaction and starts streaming on the down event and it stops steaming on the up event.
  2. Can we use a native button element to create this type functionality that will work across browsers and screen readers?

In our current custom drag interactions, my understanding is that we have currently two options:

  • Use a Grab button to first grab (pick-up/ activate) the object and enter a dragging mode (an on mode). An intentional release is required to release the object (or turn it off). This technique is used for things like the Balloons in BASE. And has proven to work well with blind learners using screen readers. Extra supports are implemented for visual learners.
  • Skip the grab button and go directly to a dragging mode (a on mode) using the Arrow keys and WASD keys. This type of interaction is used for the Both Hands in RaP. In that sim, the interaction is scaffolded by two sliders (Left Hand and Right Hand) that precede it. Custom widgets like these are generally not communicated to users in consistent ways when using screen reader software. That said, browser support can improve over time.

@jessegreenberg
Copy link
Contributor

Can we use a native button element to create this type functionality that will work across browsers and screen readers?

I had the same question, I am worried that some AT won't give us keydown/keyup events for button elements. Profiling DOM events while using screen readers is the first step for solving this.

I remember reviewing this case with the team and we predicted it would be difficult. Press and hold is generally not done (even discouraged) for accessibility and it does not have good native support with screen readers. How would press and hold even work with iOS VoiceOver gestures?

@zepumph
Copy link
Member

zepumph commented Oct 6, 2022

Subset of phetsims/scenery#1298

@zepumph
Copy link
Member

zepumph commented Nov 25, 2022

It isn't clear to me that this is in fact high priority. @pixelzoom is this not longer pressing because ph-scale is proceeding without alt-input?

@pixelzoom
Copy link
Contributor Author

Priority is not my call. While it's no longer an issue for ph-scale (which is imo unfortunate), I don't know what sims may or may not need this to proceed. Please ask @kathy-phet.

@terracoda
Copy link
Contributor

Ok, this issue is a priority again as we design description for phSB.

@terracoda
Copy link
Contributor

@jessegreenberg said:

I remember reviewing this case with the team and we predicted it would be difficult. Press and hold is generally not done (even discouraged) for accessibility and it does not have good native support with screen readers. How would press and hold even work with iOS VoiceOver gestures?

Agreed, requiring a hold is not a robust inclusive design option.

@jessegreenberg
Copy link
Contributor

I started by changing mostly PressListener. It was my first instinct because MomentaryButtonModel is controlled by downProperty is controlled by PressListener isPressedProperty. This patch works. But it might be better to handle this right in sun, similar to how StickyToggleButtonModel is done. Ill give that a try before committing.

Subject: [PATCH] Remove opt out of Interactive Highlights, see https://github.com/phetsims/friction/issues/201
---
Index: scenery/js/listeners/PressListener.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery/js/listeners/PressListener.ts b/scenery/js/listeners/PressListener.ts
--- a/scenery/js/listeners/PressListener.ts	(revision e8345d931a4cb38e2e844105b5dd82acf390b292)
+++ b/scenery/js/listeners/PressListener.ts	(date 1727203357142)
@@ -103,6 +103,9 @@
   // (a11y) - How long something should 'look' pressed after an accessible click input event, in ms
   a11yLooksPressedInterval?: number;
 
+  clickToggle?: boolean;
+  releaseOnBlur?: boolean;
+
   // If true, multiple drag events in a row (between steps) will be collapsed into one drag event
   // (usually for performance) by just calling the callbacks for the last drag event. Other events (press/release
   // handling) will force through the last pending drag event. Calling step() every frame will then be generally
@@ -142,6 +145,9 @@
   private _attach: boolean;
   private _collapseDragEvents: boolean;
 
+  private _clickToggle: boolean;
+  private _releaseOnBlur: boolean;
+
   // Contains all pointers that are over our button. Tracked by adding with 'enter' events and removing with 'exit'
   // events.
   public readonly overPointers: ObservableArray<Pointer>;
@@ -249,6 +255,8 @@
       useInputListenerCursor: false,
       canStartPress: truePredicate,
       a11yLooksPressedInterval: 100,
+      clickToggle: false,
+      releaseOnBlur: false,
       collapseDragEvents: false,
 
       // EnabledComponent
@@ -289,6 +297,8 @@
 
     this._mouseButton = options.mouseButton;
     this._a11yLooksPressedInterval = options.a11yLooksPressedInterval;
+    this._clickToggle = options.clickToggle;
+    this._releaseOnBlur = options.releaseOnBlur;
     this._pressCursor = options.pressCursor;
 
     this._pressListener = options.press;
@@ -568,12 +578,12 @@
       if ( stepTimer.hasListener( this._pdomClickingTimeoutListener ) ) {
         // @ts-expect-error TODO: This looks buggy, will need to ignore for now https://github.com/phetsims/scenery/issues/1581
         stepTimer.clearTimeout( this._pdomClickingTimeoutListener );
+      }
 
-        // interrupt may be called after the PressListener has been disposed (for instance, internally by scenery
-        // if the Node receives a blur event after the PressListener is disposed)
-        if ( !this.pdomClickingProperty.isDisposed ) {
-          this.pdomClickingProperty.value = false;
-        }
+      // interrupt may be called after the PressListener has been disposed (for instance, internally by scenery
+      // if the Node receives a blur event after the PressListener is disposed)
+      if ( !this.pdomClickingProperty.isDisposed ) {
+        this.pdomClickingProperty.value = false;
       }
     }
     else if ( this.isPressed ) {
@@ -922,59 +932,87 @@
     sceneryLog && sceneryLog.InputListener && sceneryLog.pop();
   }
 
-  /**
-   * Click listener, called when this is treated as an accessible input listener.
-   * In general not needed to be public, but just used in edge cases to get proper click logic for pdom.
-   *
-   * Handle the click event from DOM for PDOM. Clicks by calling press and release immediately.
-   * When assistive technology is used, the browser may not receive 'keydown' or 'keyup' events on input elements, but
-   * only a single 'click' event. We need to toggle the pressed state from the single 'click' event.
-   *
-   * This will fire listeners immediately, but adds a delay for the pdomClickingProperty so that you can make a
-   * button look pressed from a single DOM click event. For example usage, see sun/ButtonModel.looksPressedProperty.
-   *
-   * @param event
-   * @param [callback] optionally called immediately after press, but only on successful click
-   * @returns success - Returns whether the press was actually started
-   */
-  public click( event: SceneryEvent<MouseEvent> | null, callback?: () => void ): boolean {
-    if ( this.canClick() ) {
-      this.interrupted = false; // clears the flag (don't set to false before here)
+  private pdomToggle( event: SceneryEvent<MouseEvent> | null, callback?: () => void ): void {
+    if ( this.isPressed ) {
+      this.pdomRelease( event, true );
+    }
+    else if ( this.canClick() ) {
+      this.pdomPress( event, callback );
+    }
+  }
+
+  private pdomPress( event: SceneryEvent<MouseEvent> | null, callback?: () => void ): void {
+    this.interrupted = false; // clears the flag (don't set to false before here)
 
-      this.pdomClickingProperty.value = true;
+    this.pdomClickingProperty.value = true;
 
-      // ensure that button is 'focused' so listener can be called while button is down
-      this.isFocusedProperty.value = true;
-      this.isPressedProperty.value = true;
+    // ensure that button is 'focused' so listener can be called while button is down
+    this.isFocusedProperty.value = true;
+    this.isPressedProperty.value = true;
 
-      // fire the optional callback
-      // @ts-expect-error
-      this._pressListener( event, this );
+    // fire the optional callback
+    // @ts-expect-error
+    this._pressListener( event, this );
 
-      callback && callback();
+    callback && callback();
+  }
+
+  private pdomRelease( event: SceneryEvent<MouseEvent> | null, setPdomClicking?: boolean ): void {
+    assert && assert( this.isPressed, 'This listener is not pressed' );
 
-      // no longer down, don't reset 'over' so button can be styled as long as it has focus
-      this.isPressedProperty.value = false;
+    // no longer down, don't reset 'over' so button can be styled as long as it has focus
+    this.isPressedProperty.value = false;
 
-      // fire the callback from options
-      this._releaseListener( event, this );
+    // fire the callback from options
+    this._releaseListener( event, this );
 
-      // if we are already clicking, remove the previous timeout - this assumes that clearTimeout is a noop if the
-      // listener is no longer attached
-      // @ts-expect-error TODO: This looks buggy, will need to ignore for now https://github.com/phetsims/scenery/issues/1581
-      stepTimer.clearTimeout( this._pdomClickingTimeoutListener );
+    if ( setPdomClicking ) {
+      this.pdomClickingProperty.value = false;
+    }
+  }
+
+  /**
+   * Click listener, called when this is treated as an accessible input listener.
+   * In general not needed to be public, but just used in edge cases to get proper click logic for pdom.
+   *
+   * Handle the click event from DOM for PDOM. Clicks by calling press and release immediately.
+   * When assistive technology is used, the browser may not receive 'keydown' or 'keyup' events on input elements, but
+   * only a single 'click' event. We need to toggle the pressed state from the single 'click' event.
+   *
+   * This will fire listeners immediately, but adds a delay for the pdomClickingProperty so that you can make a
+   * button look pressed from a single DOM click event. For example usage, see sun/ButtonModel.looksPressedProperty.
+   *
+   * @param event
+   * @param [callback] optionally called immediately after press, but only on successful click
+   * @returns success - Returns whether the press was actually started
+   */
+  public click( event: SceneryEvent<MouseEvent> | null, callback?: () => void ): boolean {
+
+    if ( this._clickToggle ) {
+      this.pdomToggle( event, callback );
+    }
+    else {
+      if ( this.canClick() ) {
+        this.pdomPress( event, callback );
+        this.pdomRelease( event, false );
+
+        // if we are already clicking, remove the previous timeout - this assumes that clearTimeout is a noop if the
+        // listener is no longer attached
+        // @ts-expect-error TODO: This looks buggy, will need to ignore for now https://github.com/phetsims/scenery/issues/1581
+        stepTimer.clearTimeout( this._pdomClickingTimeoutListener );
 
-      // Now add the timeout back to start over, saving so that it can be removed later. Even when this listener was
-      // interrupted from above logic, we still delay setting this to false to support visual "pressing" redraw.
-      // @ts-expect-error TODO: This looks buggy, will need to ignore for now https://github.com/phetsims/scenery/issues/1581
-      this._pdomClickingTimeoutListener = stepTimer.setTimeout( () => {
+        // Now add the timeout back to start over, saving so that it can be removed later. Even when this listener was
+        // interrupted from above logic, we still delay setting this to false to support visual "pressing" redraw.
+        // @ts-expect-error TODO: This looks buggy, will need to ignore for now https://github.com/phetsims/scenery/issues/1581
+        this._pdomClickingTimeoutListener = stepTimer.setTimeout( () => {
 
-        // the listener may have been disposed before the end of a11yLooksPressedInterval, like if it fires and
-        // disposes itself immediately
-        if ( !this.pdomClickingProperty.isDisposed ) {
-          this.pdomClickingProperty.value = false;
-        }
-      }, this._a11yLooksPressedInterval );
+          // the listener may have been disposed before the end of a11yLooksPressedInterval, like if it fires and
+          // disposes itself immediately
+          if ( !this.pdomClickingProperty.isDisposed ) {
+            this.pdomClickingProperty.value = false;
+          }
+        }, this._a11yLooksPressedInterval );
+      }
     }
 
     return true;
@@ -1012,6 +1050,10 @@
       this.display = null;
     }
 
+    if ( this._releaseOnBlur && this.isPressed ) {
+      this.pdomRelease( null, true );
+    }
+
     // On blur, the button should no longer look 'over'.
     this.isFocusedProperty.value = false;
   }
Index: sun/js/buttons/RoundMomentaryButton.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/sun/js/buttons/RoundMomentaryButton.ts b/sun/js/buttons/RoundMomentaryButton.ts
--- a/sun/js/buttons/RoundMomentaryButton.ts	(revision cf35ec817e8675b01eb978e9301d64a2693ceeef)
+++ b/sun/js/buttons/RoundMomentaryButton.ts	(date 1727202882310)
@@ -35,6 +35,10 @@
   public constructor( property: TProperty<T>, valueOff: T, valueOn: T, providedOptions?: RoundMomentaryButtonOptions ) {
 
     const options = optionize<RoundMomentaryButtonOptions, SelfOptions, RoundButtonOptions>()( {
+      listenerOptions: {
+        clickToggle: true,
+        releaseOnBlur: false
+      },
       tandem: Tandem.REQUIRED
     }, providedOptions );
 

@jessegreenberg
Copy link
Contributor

jessegreenberg commented Sep 24, 2024

This is what it could like like if we don't change scenery, I think this is better:

EDIT: Better patch that fixes a bug when pressing the button while it has focus and moves more work into MomentaryButtonModel

Subject: [PATCH] Remove opt out of Interactive Highlights, see https://github.com/phetsims/friction/issues/201
---
Index: js/buttons/ButtonModel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/buttons/ButtonModel.ts b/js/buttons/ButtonModel.ts
--- a/js/buttons/ButtonModel.ts	(revision cf35ec817e8675b01eb978e9301d64a2693ceeef)
+++ b/js/buttons/ButtonModel.ts	(date 1727210804100)
@@ -61,6 +61,10 @@
   // will be true if and PressListeners' looksOverProperty is true, see PressListener for that definition.
   public readonly looksOverProperty: Property<boolean>;
 
+  // True when the button is being clicked via the PDOM. You can use this Property if
+  // custom behavior is needed that is specific to alternative input.
+  public readonly pdomClickingProperty: Property<boolean>;
+
   // (read-only by users, read-write in subclasses) - emitter that is fired when sound should be produced
   public readonly produceSoundEmitter: TEmitter;
 
@@ -110,6 +114,7 @@
     // model Properties
     this.overProperty = new BooleanProperty( false );
     this.downProperty = new BooleanProperty( false, { reentrant: true } );
+    this.pdomClickingProperty = new BooleanProperty( false );
     this.focusedProperty = new BooleanProperty( false );
     this.looksPressedProperty = new BooleanProperty( false );
     this.looksOverProperty = new BooleanProperty( false );
@@ -155,6 +160,7 @@
       // This will unlink all listeners, causing potential issues if listeners try to unlink Properties afterwards
       this.overProperty.dispose();
       this.downProperty.dispose();
+      this.pdomClickingProperty.dispose();
       this.produceSoundEmitter.dispose();
 
       this.looksPressedMultilink && this.looksPressedMultilink.dispose();
@@ -192,6 +198,7 @@
     } );
     pressListener.isOverProperty.lazyLink( this.overProperty.set.bind( this.overProperty ) );
     pressListener.isFocusedProperty.lazyLink( this.focusedProperty.set.bind( this.focusedProperty ) );
+    pressListener.pdomClickingProperty.lazyLink( this.pdomClickingProperty.set.bind( this.pdomClickingProperty ) );
 
     // dispose the previous multilink in case we already created a PressListener with this model
     this.looksPressedMultilink && this.looksPressedMultilink.dispose();
Index: js/buttons/ButtonNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/buttons/ButtonNode.ts b/js/buttons/ButtonNode.ts
--- a/js/buttons/ButtonNode.ts	(revision cf35ec817e8675b01eb978e9301d64a2693ceeef)
+++ b/js/buttons/ButtonNode.ts	(date 1727210654875)
@@ -280,13 +280,6 @@
   public pdomClick(): void {
     this._pressListener.click( null );
   }
-
-  /**
-   * Is the button currently firing because of accessibility input coming from the PDOM?
-   */
-  public isPDOMClicking(): boolean {
-    return this._pressListener.pdomClickingProperty.get();
-  }
 }
 
 /**
Index: js/buttons/MomentaryButtonModel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/buttons/MomentaryButtonModel.ts b/js/buttons/MomentaryButtonModel.ts
--- a/js/buttons/MomentaryButtonModel.ts	(revision cf35ec817e8675b01eb978e9301d64a2693ceeef)
+++ b/js/buttons/MomentaryButtonModel.ts	(date 1727211487934)
@@ -12,6 +12,7 @@
 import Tandem from '../../../tandem/js/Tandem.js';
 import sun from '../sun.js';
 import ButtonModel, { ButtonModelOptions } from './ButtonModel.js';
+import BooleanProperty from '../../../axon/js/BooleanProperty.js';
 
 type SelfOptions = EmptySelfOptions;
 
@@ -40,20 +41,53 @@
 
     super( options );
 
+    // For 'toggle' like behavior for alternative input. The button should only become pressed when it is
+    // not pressed while down, and should only become unpressed when it is pressed while down. The down
+    // state is controlled by the PressListener. Note that
+    this.pressedWhileDownProperty = new BooleanProperty( false );
+
     const downListener = ( down: boolean ) => {
 
-      // turn on when pressed (if enabled)
-      if ( down ) {
-        if ( this.enabledProperty.get() ) {
-          valueProperty.set( valueOn );
-        }
-      }
-      else {
-        valueProperty.set( valueOff );
+      // For alternative input,
+      if ( this.pdomClickingProperty.value ) {
+        if ( down && valueProperty.value === valueOff ) {
+          valueProperty.set( valueOn );
+          this.pressedWhileDownProperty.set( false );
+        }
+        if ( !down && valueProperty.value === valueOn ) {
+          if ( this.pressedWhileDownProperty.value ) {
+            valueProperty.set( valueOff );
+          }
+          else {
+            this.pressedWhileDownProperty.set( true );
+          }
+        }
+      }
+      else {
+
+        // turn on when pressed (if enabled)
+        if ( down ) {
+          if ( this.enabledProperty.get() ) {
+            valueProperty.set( valueOn );
+          }
+        }
+        else {
+          valueProperty.set( valueOff );
+        }
       }
     };
     this.downProperty.lazyLink( downListener );
 
+    this.valueProperty = valueProperty;
+
+    this.focusedProperty.lazyLink( focused => {
+      if ( !focused ) {
+        valueProperty.set( valueOff );
+      }
+    } );
+
+    this.valueOn = valueOn;
+
     // if valueProperty set externally, signify to ButtonModel
     const valuePropertyListener = ( value: T ) => {
       this.downProperty.set( value === valueOn );
Index: js/buttons/RoundMomentaryButton.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/buttons/RoundMomentaryButton.ts b/js/buttons/RoundMomentaryButton.ts
--- a/js/buttons/RoundMomentaryButton.ts	(revision cf35ec817e8675b01eb978e9301d64a2693ceeef)
+++ b/js/buttons/RoundMomentaryButton.ts	(date 1727211552094)
@@ -43,8 +43,12 @@
 
     super( buttonModel, new MomentaryButtonInteractionStateProperty( buttonModel ), options );
 
+    const setAriaPressed = () => this.setPDOMAttribute( 'aria-pressed', property.value === valueOff );
+    valueProperty.link( setAriaPressed );
+
     this.disposeRoundMomentaryButton = () => {
       buttonModel.dispose();
+      valueProperty.unlink( setAriaPressed );
     };
   }
 
Index: js/buttons/MomentaryButtonInteractionStateProperty.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/buttons/MomentaryButtonInteractionStateProperty.ts b/js/buttons/MomentaryButtonInteractionStateProperty.ts
--- a/js/buttons/MomentaryButtonInteractionStateProperty.ts	(revision cf35ec817e8675b01eb978e9301d64a2693ceeef)
+++ b/js/buttons/MomentaryButtonInteractionStateProperty.ts	(date 1727205649191)
@@ -14,10 +14,13 @@
 export default class MomentaryButtonInteractionStateProperty<T> extends DerivedProperty2<ButtonInteractionState, boolean, boolean> {
   public constructor( buttonModel: MomentaryButtonModel<T> ) {
     super(
-      [ buttonModel.looksOverProperty, buttonModel.looksPressedProperty ],
-      ( looksOver, looksPressed ) => {
-        return looksOver && !looksPressed ? ButtonInteractionState.OVER :
-               looksPressed ? ButtonInteractionState.PRESSED :  // remain pressed regardless of whether 'over' is true
+      [ buttonModel.looksOverProperty, buttonModel.looksPressedProperty, buttonModel.valueProperty ],
+      ( looksOver, looksPressed, buttonValue ) => {
+        const isValueOn = ( buttonValue === buttonModel.valueOn );
+        const pressedOrLooksPressed = looksPressed || isValueOn;
+
+        return looksOver && !pressedOrLooksPressed ? ButtonInteractionState.OVER :
+               pressedOrLooksPressed ? ButtonInteractionState.PRESSED :  // remain pressed regardless of whether 'over' is true
                ButtonInteractionState.IDLE;
       },
       { valueType: ButtonInteractionState }

@jessegreenberg
Copy link
Contributor

I ended up going with the second approach and discussed it a bit with @pixelzoom. Most changes are in MomentaryButtonModel, and the button acts like a "sticky toggle" when activated with alt input. The button is off when focus moves away from it.

Then I added the aria-pressed state to momentary and sticky toggle buttons.

@pixelzoom can you please review? It should be ready for next steps in ph-scale.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Sep 30, 2024

Implementation and behavior look good to me, in ph-scale and in sun demo. I'd like to have @terracoda and @arouinfar review before I proceed with next steps in ph-scale (adding hotkeys, ... see phetsims/ph-scale#297)

I also notice that sound for momentary toggle buttons has not been addressed, so I opened #896.

I'm also removing the high-priority label, because it's been high since 10/22/22 and no longer feels appropriate.

@terracoda
Copy link
Contributor

@terracoda to test on iphone and macos.

@terracoda
Copy link
Contributor

Alt Input is working for the Dropper button on MacOS 14.5 when using Safari and Chrome.
I am not finding an easy way to the sim on my iPhone, so not testing that right now.

@terracoda
Copy link
Contributor

Ok, I managed to briefly on my iPhone using Safari. I have iOS 15.8.3

I can move focus to the dropper button. I can toggle it on, but I cannot toggle it off.

Moving focus does not turn it off, but opening the combobox does turn it off.

There may be other issues, but that seems like a enough to investigate.

As a side note, the water and drain faucet work nicely with VO enabled.

@jessegreenberg
Copy link
Contributor

jessegreenberg commented Oct 9, 2024

OK thanks @terracoda. I am able to see that as well on my iPad.

I can toggle it on, but I cannot toggle it off.

For some reason, the valueProperty of the button model indicates it is already valueOff when I try to activate the button a second time on iOS VoiceOver. I will debug this more to understand what is going on.

Moving focus does not turn it off, but opening the combobox does turn it off.

This is expected. It sounds the virtual cursor is moving instead of focus. Once you interact with something other than the button, focus moves and turns off the button.

@terracoda
Copy link
Contributor

terracoda commented Nov 12, 2024

@jessegreenberg , I do not understand your second comment above:

This is expected. It sounds the virtual cursor is moving instead of focus. Once you interact with something other than the button, focus moves and turns off the button.

Are you saying movement of the virtual cursor is not sensed until another action is taken?

@jessegreenberg
Copy link
Contributor

Are you saying movement of the virtual cursor is not sensed until another action is taken

Correct. There is no way to know where the virtual cursor is or when it moves. We can only respond to focus events.

@terracoda
Copy link
Contributor

terracoda commented Nov 13, 2024

And if focus moves, creating a new focus event, you can tell, right?

Adding, is the main issue that if focus is lost and not re-established?

@jessegreenberg
Copy link
Contributor

Yes, when focus moves, there is an event for that. The behavior you are observing is because when the user moves the virtual cursor, focus stays on the momentary button.

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

6 participants