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

VoiceOver input isn't working with iOS 14 for published sims #1094

Closed
jessegreenberg opened this issue Oct 15, 2020 · 39 comments
Closed

VoiceOver input isn't working with iOS 14 for published sims #1094

jessegreenberg opened this issue Oct 15, 2020 · 39 comments

Comments

@jessegreenberg
Copy link
Contributor

From phetsims/molecules-and-light#376

Activating many things with VoiceOver on iOS 14 is currently broken for many sims that are published. Strangely, it is not broken on master. The last sim to be published with support for VoiceOver is molecules-and-light, SHAs for that were taken in June 10, 2020. So some change between now and then introduced something that fixed this. We should identify what this is so we can hopefully patch it into published sims.

@jessegreenberg jessegreenberg self-assigned this Oct 15, 2020
@jessegreenberg jessegreenberg changed the title VoiceOver input isn't working with iOS 14 VoiceOver input isn't working with iOS 14 for published sims Oct 15, 2020
@jessegreenberg
Copy link
Contributor Author

This issue is almost identical to #1001. In iOS 14 VoiceOver is sending both pointer events AND click events to the element under the virtual cursor so things are getting pressed more than once. For checkboxes this means that they are checked and immediately unchecked again.

In #1001 we fixed the issue by preventing dispatch of pointer events if the target of the event was within the PDOM. But that isn't working now because the target of the pointer event is just the Display div, like "real" pointer events.

Still not sure what changed between now and June 10 in PhET code to fix this, will search for that.

@jessegreenberg
Copy link
Contributor Author

OK - for some reason #1086 seems to have fixed this issue, putting commits f453d07 and phetsims/joist@6d462da into molecules-and-light 1.5 SHAs fixes the issue.

@jessegreenberg
Copy link
Contributor Author

jessegreenberg commented Oct 15, 2020

Before #1086, order of things was

  • down DOM event
    • Set Display.focus = null in Input.js
    • dispatch down scenery event, calling listeners
  • click DOM event
    • dispatch scenery click event, calling listeners

After #1086, order of things is

  • down DOM event
    • dispatch down scenery event
      • listener on the down event sets Display.focus to null - this interrupts other listeners on down, preventing them from firing
  • click DOM event
    • dispatch scenery click event, calling listeners

Italic part identifies why that change fixes this. But I don't think it is really a fix, but an accidental side effect.

@jessegreenberg
Copy link
Contributor Author

jessegreenberg commented Oct 15, 2020

I have access to both iOS 13.6 and 14.0. I verified that in iOS 13.6 VoiceOver sends only a MouseEvent of type click targeted at the PDOM element (for a button). In iOS 14.0, VoiceOver sends both a PointerEvent to the location of the PDOM element (targeted at the sim div) and a MouseEvent of type click targeted at the PDOM element.

I inspected the PointerEvent and saw now way to distinguish it and filter it out like we did in #1001. I am really not sure what is best. Right now I am thinking we could

@jessegreenberg
Copy link
Contributor Author

After sleeping on it I think https://stackoverflow.com/questions/8643739/cancel-click-event-in-the-mouseup-event-handler/8927598 may be our best bet if it works. We would prevent MouseEvents if we receive a PointerEvent that lands within the display bounds. If we get a PointerEvent outside of display bounds, it was likely from JAWS/NVDA/mac VoiceOver and we don't want to prevent the click. If any of those AT change and start sending PointerEvents at the right location, we can just respond to those and not respond to click, and the change should still work.

@jessegreenberg
Copy link
Contributor Author

This addition, added on a pointer event does work in both master and the release branch:

        const captureClickListener = event => {
          event.stopPropagation(); // Stop the click from being propagated.
          console.log( 'click captured' );
          window.removeEventListener( 'click', captureClickListener, true ); // cleanup
        };
        window.addEventListener( 'click', captureClickListener, true );

We need to make sure that it is only added once. We don't want to introduce a memory leak by adding this listener every down and never removing it.

@jessegreenberg
Copy link
Contributor Author

Here is the patch that would be needed for sims that support mobile accessiblity which include
gravity-force-lab
gravity-force-lab-basics
molarity
molecules-and-light
ohms-law

Index: js/input/Input.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/input/Input.js	(revision 1652425f3236dbd7bccb70247bf7987cf662de16)
+++ js/input/Input.js	(date 1603478487387)
@@ -745,6 +745,33 @@
 
       // Add a listener to the document body that will capture any keydown for a11y before focus is in this display.
       document.body.addEventListener( 'keydown', this.handleDocumentKeydown.bind( this ) );
+
+      // Prevent click events from reaching the sim if we receive a Pointer event within
+      // the display bounds. Some screen readers send both 'click' and 'pointer' events
+      // when the user activates a component while others do not. If both are sent, scenery will
+      // dispatch events for both of them, resulting in double activations. To prevent this,
+      // we stop all click events if we receive pointer events within display bounds. Some screen readers
+      // dispatch events outside of display bounds, and we still want to dispatch click in this case
+      let hasClickCaptureListener = false;
+      const clickCaptureListener = event => {
+        event.stopPropagation();
+        window.removeEventListener( 'click', clickCaptureListener, true );
+        hasClickCaptureListener = false;
+      };
+
+      this.display.addInputListener( {
+        up: event => {
+
+          // only capture the click event if a pointer event went down within the display
+          const inDisplay = this.display.bounds.containsPoint( event.pointer.point );
+          if ( inDisplay && !hasClickCaptureListener ) {
+
+            // listener added to the capture phase so click is stopped before bubbling phase
+            window.addEventListener( 'click', clickCaptureListener, true );
+            hasClickCaptureListener = true;
+          }
+        }
+      } );
     }
   }

@jessegreenberg
Copy link
Contributor Author

@zepumph and I reviewed the change and determined it just wasn't a good thing to move forward with. First of all, it doesn't work on master with the change identified in #1094 (comment). But there also isn't a guarantee that click events will follow up events, and so we could miss out on legitimate click events this way.

Ideally, we want to respond to the click event for all things a11y rather than block the click event and respond to the mouse/touch events. That is how PDOM input was designed. Unclear how to make this happen.

@jessegreenberg
Copy link
Contributor Author

This was demonstrated in phetsims/faradays-law#208. This issue should block publication of sims with description until resolved.

@zepumph
Copy link
Member

zepumph commented Dec 12, 2020

Let me know how I can help!

@KatieWoe
Copy link

I'm seeing this sort of issue in phetsims/qa#582 I believe. At least it is the same sort of issue in phetsims/faradays-law#208.
The lock ratio checkbox is difficult to check and the actual option is before the option that is selected if you try to click the box directly.
https://drive.google.com/file/d/1Wd3ulZWJ_RwSC4ryV3HvduS20KH4nTC_/view?usp=sharing

@jessegreenberg
Copy link
Contributor Author

Thanks @KatieWoe - yup, thats this issue.

@KatieWoe
Copy link

More serious for phetsims/qa#582, I am completely unable to open the My Challenge box

jessegreenberg added a commit that referenced this issue Jan 12, 2021
…nd only apply workaround when positioning siblings, see #1094
@jessegreenberg
Copy link
Contributor Author

@zepumph and I hopefully finished this in the above commits, addressing the remaining things from #1094 (comment).

@terracoda we think this is ready for review. Can you please try out a sim on master with iOS VoiceOver and make sure things are working as expected?

@zepumph
Copy link
Member

zepumph commented Jan 15, 2021

I think this should still block RaP until we can get at least a minor review by @terracoda.

@terracoda terracoda removed their assignment Jan 17, 2021
@terracoda
Copy link

terracoda commented Jan 18, 2021

Apologies, I was working on this yesterday and didn't post my results.

Most interactions (left and right hand sliders, radio buttons, lock ratio checkbox, reset all button) are working fine with and without VO enabled.

The sim screen buttons in the navbar are working fine.

The Ratio Challenges and Ranges comboboxes are not working when VO is enabled. The pop-up buttons do not receive focus at all, so it is impossible to pop-up the list of options.

We might need a new issue for this. I am looking for an already open issue.

@terracoda
Copy link

@jessegreenberg, I have verified that this is a common code issue. I tested Molarity and it has the same issue.

Can we close this issue? And focus on phetsims/ratio-and-proportion#335, or maybe it makes sense to move phetsims/ratio-and-proportion#335 to a common code repo?

@terracoda
Copy link

Ok, there is already an issue for the combo box issue. It is #1137

Since work on the combobox will happen there, I think this issue can be closed. Assigning to @jessegreenberg for closing.

@jessegreenberg
Copy link
Contributor Author

Thanks @terracoda sounds great - with #1094 (comment) I think this issue can be closed and we will continue in #1137

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