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

KeyboardFuzzer isn't fuzzing sliders #1145

Closed
zepumph opened this issue Jan 27, 2021 · 14 comments
Closed

KeyboardFuzzer isn't fuzzing sliders #1145

zepumph opened this issue Jan 27, 2021 · 14 comments
Assignees

Comments

@zepumph
Copy link
Member

zepumph commented Jan 27, 2021

From #1130, I think it in part has to do with changes since bd4c071. I fuzzed for about a minute and all RaP hands were still at the same value. It seems to treat other values quite well though. Tagging @jessegreenberg so that he is aware.

@zepumph
Copy link
Member Author

zepumph commented Jan 27, 2021

Notes:

I found that the following debuggers had the following notes:

Index: scenery/js/accessibility/KeyboardFuzzer.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery/js/accessibility/KeyboardFuzzer.js b/scenery/js/accessibility/KeyboardFuzzer.js
--- a/scenery/js/accessibility/KeyboardFuzzer.js	(revision ce1154be2f17a5ff15a2165617ca51c9750ae760)
+++ b/scenery/js/accessibility/KeyboardFuzzer.js	(date 1611711202849)
@@ -177,6 +177,9 @@
         // get active element, focus might have changed in the last press
         const elementWithFocus = document.activeElement;
 
+        if ( elementWithFocus.tagName.toUpperCase() === 'INPUT' && elementWithFocus.getAttribute( 'type' ).toUpperCase() === 'RANGE' ) {
+           debugger; // Hit many times
+        }
         if ( keyboardTestingSchema[ elementWithFocus.tagName.toUpperCase() ] ) {
 
           const randomNumber = this.random.nextDouble();
@@ -211,6 +214,9 @@
    * @private
    */
   triggerDOMEvent( event, element, keycode ) {
+    if ( element.tagName.toUpperCase() === 'INPUT' && element.getAttribute( 'type' ).toUpperCase() === 'RANGE' ) {
+      debugger; // hit all the time
+    }
     const eventObj = new KeyboardEvent( event, {
       code: keycode,
       which: keycode,
Index: js/input/Input.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/input/Input.js b/js/input/Input.js
--- a/js/input/Input.js	(revision ce1154be2f17a5ff15a2165617ca51c9750ae760)
+++ b/js/input/Input.js	(date 1611711386122)
@@ -682,6 +682,8 @@
         sceneryLog && sceneryLog.Input && sceneryLog.Input( 'keydown(' + Input.debugText( null, event ) + ');' );
         sceneryLog && sceneryLog.Input && sceneryLog.push();
 
+        debugger; // never hit
+
         const trail = this.updateTrailForPDOMDispatch( event );
         this.dispatchA11yEvent( trail, 'keydown', event, true );
 Index: sun/js/accessibility/AccessibleValueHandler.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/sun/js/accessibility/AccessibleValueHandler.js b/sun/js/accessibility/AccessibleValueHandler.js
--- a/sun/js/accessibility/AccessibleValueHandler.js	(revision 03187fbb29ec42fa8851b06898a202502e0008f4)
+++ b/sun/js/accessibility/AccessibleValueHandler.js	(date 1611710710410)
@@ -502,6 +502,8 @@
         const code = domEvent.keyCode;
         this._shiftKey = domEvent.shiftKey;
 
+        debugger; // never hit!
+
         // if we receive a keydown event, we shouldn't handle any input events (which should only be provided
         // directly by an assistive device)
         this.blockInput = true;

I found that this HTML snippet does work, so my guess is that the issue is in how we clear the listeners or utilize the keyup listener firing immediately.

<input data-trail-id="1127-1128-1504-1510-1505-1615-1511-1512" id="1127-1128-1504-1510-1505-1615-1511-1512"
       data-focusable="true" class="pdom-sibling" type="range" aria-orientation="vertical"
       aria-valuetext="hands, at challenge ratio&#8202;&#8202;&#8202;&#8202;&#8202;&#8202;&#8202;&#8202;&#8202;&#8202;"
       min="0.010000000000000222" max="1" step="0.01" aria-valuenow="0.2"
       style="top: 0px; left: 0px; width: 1px; height: 1px;">

<script>

  document.getElementById('1127-1128-1504-1510-1505-1615-1511-1512').addEventListener('keydown', ()=>{
    debugger; // we get this!
  });

  const eventObj = new KeyboardEvent( 'keydown', {
    code: 33,
    which: 33,
    shiftKey: 33,
    altKey: 33,
    ctrlKey: 33
  } );

  document.getElementById('1127-1128-1504-1510-1505-1615-1511-1512').dispatchEvent( eventObj );
</script>

@jessegreenberg, I don't think that you have to have focus in order to send an event and have a listener fire on it, as the above snippet doesn't have focus. Can you think of what the issue is here? I'll keep poking around

@zepumph
Copy link
Member Author

zepumph commented Jan 27, 2021

Input.clickAction is getting hit often, so I wonder if this is an issue with the guts of triggerDOMEvent after all.

@zepumph
Copy link
Member Author

zepumph commented Jan 27, 2021

I wonder if this has something to do with things:

https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/dispatchEvent#notes

Unlike "native" events, which are fired by the DOM and invoke event handlers asynchronously via the event loop, dispatchEvent() invokes event handlers synchronously. All applicable event handlers will execute and return before the code continues on after the call to dispatchEvent().

At least it is good to know that this is synch, so that I can test accordingly. This means that triggering this DOM event should directly and immediately fire the listener on the display.

@zepumph
Copy link
Member Author

zepumph commented Jan 27, 2021

I wonder if KeyboardEvent bubbles as expected? I will check in the HTML snippet.

@zepumph
Copy link
Member Author

zepumph commented Jan 27, 2021

Ahh! It doesn't bubbles!

@zepumph
Copy link
Member Author

zepumph commented Jan 27, 2021

triggerDOMEvent was doing nothing until the above commit, because it wasn't bubbling up to the Display listener.

Still though, sliders are broken, and will continue to be, because the event we create doesn't set keyCode, at least in chrome. Thus this is blocked by #1053, and until our code listens to code, instead of keyCode, this is likely to persist.

@jessegreenberg
Copy link
Contributor

I think proceeding with #1053 is the right way forward, but would it have also been fixed if the keyCode was defned when constructing the KeyboardEvent?

new KeyboardEvent( event, {
      bubbles: true,
      code: keycode,
      which: keycode,
      keycode: keycode,
      //...
    } );

@zepumph
Copy link
Member Author

zepumph commented Jan 27, 2021

Yes, I tried that, as well as as directly setting it after creation. I think for security reasons perhaps, it didn't work for me:

image

@zepumph
Copy link
Member Author

zepumph commented Jan 27, 2021

ArggG! The above is not correct. Note there is a casing problem. I'm sorry. I'll commit that now, and we will have working sliders.

@zepumph
Copy link
Member Author

zepumph commented Jan 27, 2021

image

@zepumph
Copy link
Member Author

zepumph commented Jan 27, 2021

Working sliders (and other keydown/up event pairs) after the above. Thanks for the help @jessegreenberg!

@zepumph
Copy link
Member Author

zepumph commented Jan 27, 2021

Anything else here?

@jessegreenberg
Copy link
Contributor

Oh I see - I messed it up in #1145 (comment) as well. We have a lot of keycode in KeyboardFuzzer, maybe we should replace all of them throughout. I might do that and add keycode to bad-text, would that be OK with you?

@jessegreenberg
Copy link
Contributor

Alright, done in the above commits - and I verified fuzzBoard is fuzzing sliders so closing.

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

No branches or pull requests

2 participants