search

phetsims / mean-share-and-balance

"Mean: Share and Balance" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
2 stars 1 forks source link

Focus sometimes missing when moving an object with keyboard nav #329

Closed Nancy-Salpepi closed 2 months ago

Nancy-Salpepi commented 2 months ago

Test device MacBook Air M1 chip

Operating System 14.5

Browser Safari 17.5

Problem description For https://github.com/phetsims/qa/issues/1121, on the Distribute and Balance Point screens, the focus highlights for keyboard nav will fail to appear after moving a chocolate bar or a soccer ball with alt input, then moving the same/different bar/ball with the mouse and then switching back to keyboard navigation.

Steps to reproduce

  1. On the Distribute screen, add the max amount of people
  2. Tab to and grab one of the chocolate bars on the notepad and move it to another pile
  3. While that bar still has keyboard focus, use the mouse to grab the same bar or a different one and move it to another pile
  4. Press the HOME,END or arrow keys-- they will move the original bar.
  5. Follow similar steps on the Balance Point screen.

Visuals

https://github.com/user-attachments/assets/ab8beade-e8cd-4942-8b85-fdcadd727e63

Nancy-Salpepi commented 2 months ago

This behavior doesn't always happen on the Balance Point screen and I can't quite figure out what I may be doing differently.

marlitas commented 2 months ago

I'm going to take a stab at this. @jessegreenberg I'll let you know if I'll need your help.

marlitas commented 2 months ago

I think this is now being caught by an assertion that was turned on after some work I did yesterday in https://github.com/phetsims/soccer-common/issues/7. Great news, because we have a clear starting point to search from.

marlitas commented 2 months ago

@jessegreenberg and I worked on some behavior questions for groupSortInteraction today that ended up solving this issue. However I did notice some inconsistencies in where the sim was focusing after a mouse even.

JG and I committed code that would reset keyboard state and fire a blur event whenever we received a mouse down event. Our understanding is that upon the user hitting tab the next item in the pdom would be focused (in the case of group sort, the first number spinner). However, I noticed that sometimes tab would lead me to the number spinner, and sometimes it would focus on the group again.

@jessegreenberg would love your thoughts on how buggy this feels to you? It seems strange but I'm not sure how broken it feels.

https://github.com/user-attachments/assets/14a3a257-b888-4e04-8ba3-b718e61441f4

jessegreenberg commented 2 months ago

I took a brief look. To consistently reproduce:

I confirmed that in both cases, we successfully call blur from https://github.com/phetsims/scenery-phet/commit/5fa5a3130c9cd81f2323cf22ff0f8ef2b6469d97. Because of that, this should not be an issue and the behavior should be correct.

This may be happening because of a DOM state change triggered by GroupSortInteractionView, if something causes the PDOM elements to re-render (like changing the accessibleName) the browser might behave differently after the blur. Or it may be happening because of https://github.com/phetsims/joist/issues/750#issuecomment-2261048786. I didn't investigate further and recommend continuing without fixing.

Back to you @marlitas.

marlitas commented 2 months ago

Thanks @jessegreenberg. This is ready to cherry pick!

Nancy-Salpepi commented 2 months ago

The original issue is fixed so I am going to close this one. A new issue related to this can be found at https://github.com/phetsims/mean-share-and-balance/issues/343

Nancy-Salpepi commented 2 months ago

Sorry but I need to reopen. Here is one way to get it to happen reliably:

  1. Turn on interactive highlights
  2. With the pointer, grab the highlighted bar and hold it in the air
  3. While the bar is still grabbed, tab to and grab the next available bar (bar on top of left stack)
  4. With alt input move that second bar to the stack on the right but do not release
  5. Move the first bar with the pointer outside of the outer highlight
  6. place the bar on the stack above the one that was grabbed using alt input
  7. press the arrow keys

https://github.com/user-attachments/assets/c3be25f0-9588-47b6-84ac-5bfc48b53f70

marlitas commented 2 months ago

@jbphet and I were surprised to find that a keyboard event was not cancelling a pointer. We noticed that you can do also move the same element with both keyboard and mouse at the same time as shown in the published version of Faraday's Electromagnetic Lab. We are curious if this is expected behavior that individual sims need to handle.

https://github.com/user-attachments/assets/6daf4e37-7dfa-42b7-9ba9-c38338b81b1f

jessegreenberg commented 2 months ago

Discussed with @marlitas. Indeed, this is a problem for several sims but we have generally been deferring issues related to using mouse and keyboard simultaneously. @marlitas described that this can create serious problems for GrabDragInteraction and this sim, so it would be ideal if it can be fixed.

I will take a look at GroupSortInteractionView to see if there is a way we can interrupt the pointer-related drag listeners when keys are pressed. I will also run the problem by @jonathanolson to see if he has any thoughts about fixing this more deeply in scenery (I was nervous about generally interrupting listeners whenever a key or tab is pressed, that could prevent mouse+keyboard behavior we might want in the future).

@marlitas will also check with design leads about whether this needs to be fixed before publishing.

We will check in again on Monday.

jessegreenberg commented 2 months ago

OK, I have a couple of ideas. I noticed that in the unbuilt version, an assertion is thrown before the bug in https://github.com/phetsims/mean-share-and-balance/issues/329#issuecomment-2289600534 which is helpful.

@marlitas @jbphet can you please review?

1) You may be able to put model.resetInteractionState(); at the top of the focus listener in GroupSortInteractionView. That makes the assertion failure go away. This is a simple change. But I don't know enough about GroupSortInteraction to say whether it is working correctly.

2) We can give GroupSortInteractionModel emitters to reset keyboard and mouse listener when necessary. GroupSortInteractionView can handle the interruption for keyboard input. But it will be up to clients to implement interruption for mouse/touch. This is a similar idea to setMouseSortedGroupItem. Here is a patch with this. Tested lightly but will need some further review and cleanup for disposal and such.

```patch Subject: [PATCH] Interrupt mouse/keyboard when one form of input starts --- Index: mean-share-and-balance/js/distribute/view/NotepadCandyBarNode.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/mean-share-and-balance/js/distribute/view/NotepadCandyBarNode.ts b/mean-share-and-balance/js/distribute/view/NotepadCandyBarNode.ts --- a/mean-share-and-balance/js/distribute/view/NotepadCandyBarNode.ts (revision 431ad8b359b09fd8c719b6c77c6f2f8d68ea352e) +++ b/mean-share-and-balance/js/distribute/view/NotepadCandyBarNode.ts (date 1724080946791) @@ -116,6 +116,11 @@ tandem: options.tandem.createTandem( 'dragListener' ) } ); + // When the mouse input is interrupted, stop dragging the candy bar. + groupSortInteractionModel.interruptMouseInputEmitter.addListener( () => { + this.dragListener.interrupt(); + } ); + this.addInputListener( this.dragListener ); this.candyBar.positionProperty.link( position => Index: scenery-phet/js/accessibility/group-sort/model/GroupSortInteractionModel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/scenery-phet/js/accessibility/group-sort/model/GroupSortInteractionModel.ts b/scenery-phet/js/accessibility/group-sort/model/GroupSortInteractionModel.ts --- a/scenery-phet/js/accessibility/group-sort/model/GroupSortInteractionModel.ts (revision 8124a3a8728a7647543e585b2b9636f085074b62) +++ b/scenery-phet/js/accessibility/group-sort/model/GroupSortInteractionModel.ts (date 1724081963236) @@ -65,6 +65,7 @@ import IOType from '../../../../../tandem/js/types/IOType.js'; import ReferenceIO from '../../../../../tandem/js/types/ReferenceIO.js'; import NullableIO from '../../../../../tandem/js/types/NullableIO.js'; +import Emitter from '../../../../../axon/js/Emitter.js'; type SelfOptions = { @@ -124,6 +125,12 @@ // input (not from keyboard/group sort input), but is important for cue showing. public readonly hasMouseSortedGroupItemProperty = new BooleanProperty( false ); + // When a new input method starts, you may need to interrupt the other. GroupSortInteractionView handles interruption + // for keyboard input. You will need to add a listener to the interruptMouseInputEmitter to interrupt your + // implementation of mouse/touch input. + public readonly interruptKeyboardInputEmitter = new Emitter(); + public readonly interruptMouseInputEmitter = new Emitter(); + // Whether any group item has yet been sorted to a new value, even if not by the "group sort" interaction. This // Property should be used to control the mouseSortCueVisibleProperty. The mouse sort cue does not need to be shown // if a keyboard sort has occurred (because now the user knows that the group items are sortable). @@ -276,6 +283,9 @@ this.hasKeyboardSelectedGroupItemProperty.dispose(); this.mouseSortCueVisibleProperty.dispose(); this.hasGroupItemBeenSortedProperty.dispose(); + + this.interruptKeyboardInputEmitter.dispose(); + this.interruptMouseInputEmitter.dispose(); super.dispose(); } } Index: scenery-phet/js/accessibility/group-sort/view/GroupSortInteractionView.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/scenery-phet/js/accessibility/group-sort/view/GroupSortInteractionView.ts b/scenery-phet/js/accessibility/group-sort/view/GroupSortInteractionView.ts --- a/scenery-phet/js/accessibility/group-sort/view/GroupSortInteractionView.ts (revision 8124a3a8728a7647543e585b2b9636f085074b62) +++ b/scenery-phet/js/accessibility/group-sort/view/GroupSortInteractionView.ts (date 1724080687636) @@ -198,6 +198,8 @@ const focusListener = { focus: () => { + model.interruptMouseInputEmitter.emit(); + // It's possible that getGroupItemToSelect's heuristic said that there is nothing to focus here if ( selectedGroupItemProperty.value === null ) { selectedGroupItemProperty.value = options.getGroupItemToSelect(); @@ -223,6 +225,8 @@ }, down: () => { + model.interruptKeyboardInputEmitter.emit(); + // When the group is clicked, reset the interaction state to stop keyboard input logic. We only support // one mode of input at a time. model.resetInteractionState(); @@ -358,6 +362,15 @@ } } ); + const interruptListener = () => { + deltaKeyboardListener.interrupt(); + grabReleaseKeyboardListener.interrupt(); + }; + model.interruptKeyboardInputEmitter.addListener( interruptListener ); + model.disposeEmitter.addListener( () => { + model.interruptKeyboardInputEmitter.removeListener( interruptListener ); + } ) + if ( options.numberKeyMapper ) { const numbersKeyboardListener = new KeyboardListener( { fireOnHold: true, @@ -380,8 +393,15 @@ } } ); primaryFocusedNode.addInputListener( numbersKeyboardListener ); + + const interruptNumbersListener = () => { + numbersKeyboardListener.interrupt(); + }; + model.interruptKeyboardInputEmitter.addListener( interruptNumbersListener ); + this.disposeEmitter.addListener( () => { primaryFocusedNode.removeInputListener( numbersKeyboardListener ); + model.interruptKeyboardInputEmitter.removeListener( interruptNumbersListener ); numbersKeyboardListener.dispose(); } ); } ```
marlitas commented 2 months ago

@jessegreenberg thank you for the investigation and work here. I think it will be valuable to determine wether this is something GroupSortInteraction should handle in general and I added it as a discussion point for dev meeting. The team has decided that this is not blocking for MSaB and we will proceed by ignoring any dual mouse and keyboard bugs with QA looking out for the following:

QA, if any of the above occur please flag that as it is much more critical and may need to be addressed.

Thanks!

Nancy-Salpepi commented 2 months ago

I'm not seeing any of the scenarios in https://github.com/phetsims/mean-share-and-balance/issues/329#issuecomment-2297731877. Closing.