phetsims / scenery-phet

Reusable components based on Scenery that are specific to PhET simulations.
MIT License
8 stars 6 forks source link

Keyboard help content for FaucetNode #839

Closed jessegreenberg closed 8 months ago

jessegreenberg commented 8 months ago

In https://github.com/phetsims/scenery-phet/issues/773 we added alt input to the FaucetNode. Now we should add keyboard help content for it.

It behaves a lot like a slider but with some extras. Design is listed here: https://github.com/phetsims/scenery-phet/issues/773#issuecomment-1896504404

I would like to use SliderControlsKeyboardHelpSection as much as we can. Here is what that would look like to get us started. SliderControlsKeyboardHelpSection has other options to control the language. @terracoda @arouinfar what do you think of this and what should change from here?

image

Here is the patch but we would create a subclass just for this.

```patch Subject: [PATCH] Indicate that items have been sorted, see https://github.com/phetsims/scenery-phet/issues/815 --- Index: js/demo/SceneryPhetKeyboardHelpContent.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/demo/SceneryPhetKeyboardHelpContent.ts b/js/demo/SceneryPhetKeyboardHelpContent.ts --- a/js/demo/SceneryPhetKeyboardHelpContent.ts (revision 985eb3f059cd1b65b3486afd18568bba5c1aba21) +++ b/js/demo/SceneryPhetKeyboardHelpContent.ts (date 1706656269452) @@ -14,6 +14,9 @@ import TwoColumnKeyboardHelpContent from '../keyboard/help/TwoColumnKeyboardHelpContent.js'; import sceneryPhet from '../sceneryPhet.js'; import { combineOptions } from '../../../phet-core/js/optionize.js'; +import KeyboardHelpSectionRow from '../keyboard/help/KeyboardHelpSectionRow.js'; +import TextKeyNode from '../keyboard/TextKeyNode.js'; +import KeyboardHelpIconFactory from '../keyboard/help/KeyboardHelpIconFactory.js'; export default class SceneryPhetKeyboardHelpContent extends TwoColumnKeyboardHelpContent { @@ -22,7 +25,7 @@ const helpContentOptions = { // i18n, restricts both labelText and maxWidth, see KeyboardHelpSection - textMaxWidth: 130 + textMaxWidth: 200 }; const basicActionsHelpContent = new BasicActionsKeyboardHelpSection( @@ -33,7 +36,17 @@ const grabDragHelpContent = new GrabReleaseKeyboardHelpSection( new StringProperty( 'Grabbable' ), new StringProperty( 'grabbable' ), helpContentOptions ); - const leftHelpContent = [ basicActionsHelpContent ]; + + const stopFlowRow = KeyboardHelpSectionRow.labelWithIcon( 'Stop faucet flow', new TextKeyNode( '0' ) ); + const tapToDispenseRow = KeyboardHelpSectionRow.labelWithIcon( 'Tap to dispense', KeyboardHelpIconFactory.spaceOrEnter() ) + const faucetKeyboardHelpSection = new SliderControlsKeyboardHelpSection( { + arrowKeyIconDisplay: SliderControlsKeyboardHelpSection.ArrowKeyIconDisplay.LEFT_RIGHT, + headingStringProperty: 'Faucet Controls', + sliderStringProperty: 'Faucet', + additionalContents: [ stopFlowRow, tapToDispenseRow ], + } ); + + const leftHelpContent = [ basicActionsHelpContent, faucetKeyboardHelpSection ]; KeyboardHelpSection.alignHelpSectionIcons( [ grabDragHelpContent, sliderControlsKeyboardHelpSection ] ); const rightHelpContent = [ grabDragHelpContent, sliderControlsKeyboardHelpSection ]; 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 985eb3f059cd1b65b3486afd18568bba5c1aba21) +++ b/js/keyboard/help/SliderControlsKeyboardHelpSection.ts (date 1706656333103) @@ -62,6 +62,8 @@ // This option determines whether this keyboard help section will have the // "Adjust in Smaller Steps" row. The row includes the string and key icons. includeSmallerStepsRow?: boolean; + + additionalContents?: KeyboardHelpSectionRow[]; }; export type SliderControlsKeyboardHelpSectionOptions = SelfOptions & KeyboardHelpSectionOptions; @@ -91,7 +93,9 @@ adjustInSmallerStepsStringProperty: SceneryPhetStrings.keyboardHelpDialog.adjustInSmallerStepsStringProperty, adjustInLargerStepsStringProperty: SceneryPhetStrings.keyboardHelpDialog.adjustInLargerStepsStringProperty, - includeSmallerStepsRow: true + includeSmallerStepsRow: true, + + additionalContents: [] }, providedOptions ); let adjustSliderStringProperty = options.adjustSliderStringProperty; @@ -216,7 +220,7 @@ } ); // assemble final content for KeyboardHelpSection - const content = [ adjustSliderRow, ...( options.includeSmallerStepsRow ? [ adjustSliderInSmallerStepsRow ] : [] ), adjustInLargerStepsRow, jumpToMinimumRow, jumpToMaximumRow ]; + const content = [ adjustSliderRow, ...( options.includeSmallerStepsRow ? [ adjustSliderInSmallerStepsRow ] : [] ), adjustInLargerStepsRow, jumpToMinimumRow, jumpToMaximumRow, ...options.additionalContents ]; super( options.headingStringProperty, content, options ); } ```
jessegreenberg commented 8 months ago

Oh, I noticed @pixelzoom also started a class called FaucetControlsKeyboardHelpContent in ph-scale (from before we added other key commands) that looks like this:

image

pixelzoom commented 8 months ago

Feel free to use FaucetControlsKeyboardHelpContent as a starting point, if it's helpful. We'll need an option to add a description of "Space", for FaucetNode instances that are created with tapToDispense=true.

arouinfar commented 8 months ago

Thanks, @jessegreenberg @pixelzoom for starting this content. There are aspects I like about both, and would like to combine them somewhat. I don't know how much flexibility we have to change the core slider content, but having separate entries for Home and 0 seems odd.

Here's my proposal:

Faucet Controls

Adjust faucet flow [Left] [Right]
Adjust in smaller steps [Shift] + [Left] [Right] Adjust in larger steps [Pg Up] [Pg Down] Close faucet [Home] or [0] Open faucet fully [End] Dispense a little fluid [Space] or [Enter]

Since the faucet has a left/right orientation, I only included the left/right arrows. I think it looks a bit cleaner with just left/right, even though up/down also works. If we need all four arrows, that's also fine with me.

I took some liberties with how we describe the home/end behavior, and I don't know if that's acceptable. If the content needs to look more slider-like, we could use the typical home/end content and add a separate entry for the [0] key, like @jessegreenberg's screenshot.

If it would be more helpful to iterate on this content synchronously, let me know, and I'd be happy to schedule a meeting.

pixelzoom commented 8 months ago

@arouinfar Your proposal sounds great to me.

@jessegreenberg @terracoda Do you have any additional input?

@jessegreenberg Would you like me to modify and generalize FaucetControlsKeyboardHelpContent?

terracoda commented 8 months ago

Yes, I might. Will review shortly.

terracoda commented 8 months ago

@arouinfar & @pixelzoom I was also thinking "flow" needed to be part of the description. Adding "Flow" to the main heading makes the control more about flow and less about the faucet.

"Faucet Flow" or "Fluid Flow" or "Faucet Flow Control" or "Fluid Flow Control" or even "Faucet Fluid Flow"

terracoda commented 8 months ago

@arouinfar, I think adjusting the Home/End Shortcuts makes total sense.

Since Faucet Node can also empty a container, do we need to account for that case in the general descriptions, or have 2 versions? Screenshot 2024-02-05 at 17 25 51

"Faucet Fluid Flow"

terracoda commented 8 months ago

@arouinfar, or others, what combinations of words feels best or perhaps most general?

terracoda commented 8 months ago

A very rough visual rough-visual-keyboard-shortcuts-faucet-node

pixelzoom commented 8 months ago

@jessegreenberg pointed out that there's a rough version of this in pH Scale, see https://github.com/phetsims/scenery-phet/issues/839#issuecomment-1919288920.

In our design discussions for pH Scale, we intentionally avoided using the word "Fluid", because:

So since FaucetNode looks like a faucet regardless of what it is dispensing, I think it would be more general to title this "Faucet Controls", as @arouinfar suggested.

terracoda commented 8 months ago

Thanks @pixelzoom for the context on "fluid". I did suggest "Faucet Flow" and "Faucet Flow Control".

terracoda commented 8 months ago

If you have flow in the main heading, it might work to use it less in the instructions. For example:

Faucet Flow Control

terracoda commented 8 months ago

Totally open to a different order as well.

arouinfar commented 8 months ago

I was also thinking "flow" needed to be part of the description. Adding "Flow" to the main heading makes the control more about flow and less about the faucet.

I think it's important to call it a "Faucet" because that is the name of the component, and I would likely use "Faucet" in the accessible name of the control, e.g. "Solute Faucet". I think "flow" is best suited to the description of the controls, not the section title. If it needs to be in the section title, then 'Faucet Flow Controls" is fine.

I think adjusting the Home/End Shortcuts makes total sense.

Great! Glad to know we have that flexibility. It would be nice if the language for home/end was parallel, like using close/open. But if we use "Jump to max flow" then "Stop flow" works better.

Since Faucet Node can also empty a container, do we need to account for that case in the general descriptions, or have 2 versions?

Oh, that's an excellent point @terracoda. In principle, a sim could have only draining faucets, only dispensing faucets, or both. It might be simpler to avoid conditional dispense/drain language altogether and use something more general. I think "Open briefly" could work.

Depending on the direction of the faucet the Right or Left arrow can start the flow or open the drain, so wondering if "start" is useful.

I think "Adjust flow" is a flexible enough description to include starting the flow, but if we want to be more explicit "Start or adjust flow" sounds good to me.

Here's another iteration, combining some of the ideas thus far:

Faucet Controls or Faucet Flow Controls

terracoda commented 8 months ago

So glad we agree on some ideas :-)

Re "Controls", I see it as a single control object, a "Faucet Control" or a "Faucet Flow Control", either work.

The control object has many ways to control the flow based on input method. I would not make it plural unless there was more than one faucet in the sim.

I do like the idea of language matching start/stop flow or open/close faucet, but was having trouble finding a parallel with the addition of "flow". Iteration brings new ideas:

Faucet Control

I think open/close works well with the faucet metaphor, but the last one is hardest as it is kind of a manual self control that can dispense a little or a lot.

Faucet Control

The last item needs to optional.

kathy-phet commented 8 months ago

I've spent time reviewing this issue and playing with the demo, and my recommendation (and reasoning is here).

Faucet Controls -> (to be consistent with the other headers ... e.g. Slider Controls ... and there are many controls below) Adjust faucet flow [left] [right] -> (to make it shorter, it will be obvious that it starts a flow) Adjust in smaller steps [Shift] + [Left] [Right] Adjust in larger steps [Pg Up][Pg Down] Close faucet [Home] or [0] Open faucet fully [End] -> (“fully” word matches "open" context) Open faucet briefly [Space] or [Enter] -> (makes sense, other options do not make sense based on the way the control works)

terracoda commented 8 months ago

Thanks for bringing all the different pieces together, @kathy-phet .

kathy-phet commented 8 months ago

Thanks for the review, @terracoda.

Let's go with this version then. FYI - @jessegreenberg @arouinfar @pixelzoom

Faucet Controls Adjust faucet flow [left] [right] Adjust in smaller steps [Shift] + [Left] [Right] Adjust in larger steps [Pg Up][Pg Down] Close faucet [Home] or [0] Open faucet fully [End] Open faucet briefly [Space] or [Enter]

pixelzoom commented 8 months ago

In the above commits, I implemented what is described in https://github.com/phetsims/scenery-phet/issues/839#issuecomment-1933006978, using what I had done in ph-scale as a starting point. Following the naming convention used elsewhere, the class is FaucetControlsKeyboardHelpSection.

You can test-drive in scenery-phet demo, "Keyboard" screen, see screenshot below. Note that the "Open faucet briefly" row is optional, to be included if your FaucetNode is created with tapToDispenseEnabled: true.

This is fully translatable, but does not include description. Looking at other keyboard help sections, I don't see a clear "exemplar" for how to set up keyboard help for description. In SliderControlsKeyboardHelpSection, it looks like more than half of the implementation is for description, so considerably more work. We don't have a description design for this. And description is not included in the FEL 1.0 release.

Back to @jessegreenberg to review the implementation, and decided whether description needs to be addressed now. Feel free to close this issue or leave it open, as you see fit.

I'll proceed with integrating this help into FEL in https://github.com/phetsims/faradays-electromagnetic-lab/issues/74.

screenshot_3020
jessegreenberg commented 8 months ago

FaucetControlsKeyboardHelpSection looks really great! Not extending SliderControlsKeyboardHelpSection (adding options to it for this) makes sense at this point. As part of review I spot checked the keys in scenery-phet-strings_en.json and tested FEL with ?stringTest=dynamic and it worked well.

Thanks for adding this!

Lets not include description as part of this issue. We can open a new issue for that when it it is time.

Thanks everyone! Closing.