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

Add ability to listen for keyboard events outside of the PDOM #1048

Closed
jessegreenberg opened this issue Apr 21, 2020 · 12 comments
Closed

Add ability to listen for keyboard events outside of the PDOM #1048

jessegreenberg opened this issue Apr 21, 2020 · 12 comments
Assignees

Comments

@jessegreenberg
Copy link
Contributor

jessegreenberg commented Apr 21, 2020

For phetsims/scenery-phet#571. We want to be able to listen to keyboard events when focus is outside of the PDOM.

One way to do this is to listen for events is to attach the Display.keyStateTracker to document.body (See KeyStateTracker.attachToBody).

Another way to do this would be to attach DOM input listeners for PDOM to Display.domElement instead of Display.accessibleDOMElement.

Maybe there are other ways too, I haven't thought through the implications yet.

@jessegreenberg jessegreenberg self-assigned this Apr 21, 2020
@jessegreenberg
Copy link
Contributor Author

jessegreenberg commented Apr 21, 2020

One way to do this is to listen for events is to attach the Display.keyStateTracker to document.body (See KeyStateTracker.attachToBody).

This is working well. aqua tests passing.

Another way to do this would be to attach DOM input listeners for PDOM to Display.domElement instead of Display.accessibleDOMElement.

This isn't working. When the browser is active but focus isn't in the PDOM, listeners have to be on document.body to receive input. And when this happens, clicking on the sim is sending 'click' events through the PDOM input system. I don't think we want scenery to dispatch 'click' events from pointer input.

@jessegreenberg
Copy link
Contributor Author

This is working well. aqua tests passing.

But scenery tests are failing, would need to investigate.

But @zepumph and I also just discussed and he recommended another approach:
Add a subset of set of Input.A11Y_EVENT_TYPES to the document.body, and dispatch down the scene graph with SceneryEvent.type names like 'global-keydown' (or something). This way we can listen go global key events using the typical scenery Input system and without using KeyStateTracker's Emitters.

@jessegreenberg
Copy link
Contributor Author

dispatch down the scene graph with SceneryEvent.type names like 'global-keydown' (or something)

I don't see this working yet. On document.body, there is no target in the scene graph and therefore no trail to dispatch to.

@jessegreenberg
Copy link
Contributor Author

jessegreenberg commented Apr 22, 2020

The way to do a "global" event a la dispatch would mean something like getting all leaf trails in the scene graph and dispatching a SceneryEvent to all of them.

@jessegreenberg
Copy link
Contributor Author

#1048 (comment)

Is possible, but feels excessive. MAL has 209 leaf trails and we would be dispatching to all of these with bubbling every event. This also isn't the correct behavior yet. A node that has more than one leaf trail through it will receive "global" events multiple times as we dispatch an event to each one. Would be good to brainstorm a bit more, but I am coming back to the first solution in #1048 (comment).

@jessegreenberg
Copy link
Contributor Author

jessegreenberg commented Apr 22, 2020

I pushed the change to attach the KeyStateTracker to the body so that it can be used for keyboard events outside of the PDOM. Unit tests and aqua fuzzing is passing.

I am not sure if this is the best solution yet, I would like to discuss with @zepumph again at the next a11y-dev meeting.

@pixelzoom
Copy link
Contributor

Changes to PDOMPointer that reference this issue in their commit message are causing CT failures for multiple sims, see phetsims/scenery-phet#605.

@jessegreenberg
Copy link
Contributor Author

Thanks for pointing out, the error is actually related to #921.

@jessegreenberg
Copy link
Contributor Author

Reviewed again with @zepumph on 10/27/2020 -

KeyStateTracker.attachToDocument should be renamed to KeyStateTracker.attachToWindow.
Display should not have a static KeyStateTracker. instead, we will create a new singleton called globalKeyStateTracker that will create a KeyStateTracker and attach it to the window upon initialization.

One of the reasons to move the KeyStateTracker out of Display is that then we can avoid having all sims create this listener unless they need it. Since all Sims use a KeyboardFuzzer, we are going to avoid using globalKeyStateTracker there, and it will create its own KeyStateTracker instance.

@zepumph
Copy link
Member

zepumph commented Dec 3, 2020

We ran into this while working on #1119, I'll take this on.

@zepumph
Copy link
Member

zepumph commented Dec 3, 2020

@jessegreenberg please review.

I tested with MAL global keys, as well as poking around with zooming on my own, fuzzing, and PhET-iO studio. Anything else here?

@jessegreenberg
Copy link
Contributor Author

globalKeyStateTracker looks great, as do its usages. Thanks!

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

3 participants