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 a focusin event that bubbles when focus changes #920

Closed
mbarlow12 opened this issue Jan 7, 2019 · 10 comments
Closed

Add a focusin event that bubbles when focus changes #920

mbarlow12 opened this issue Jan 7, 2019 · 10 comments

Comments

@mbarlow12
Copy link
Contributor

After the changes from phetsims/scenery-phet@2eead2b, phetsims/scenery-phet@cb7d61c, and phetsims/scenery-phet@eeb182f, I discovered that we can no longer add event listeners (at least for focus) to NumberControls or composite components that use NumberControl.

For example, the following produces no output:

const numberControl = new NumberControl( ... );
numberControl.addInputListener( {
  focus: e => { console.log( 'numberControl focused' ); }
} );

however, within NumberControl, we can add a focus listener onto the Slider component that will be called.

@jessegreenberg and I tracked this down to the fact that we're not bubbling focus events in the scene graph. From the focusinEmitter in Input.js:

this.dispatchEvent( trail, 'focus', this.a11yPointer, event, false );

Essentially, the slider is triggering the focus event, but Scenery doesn't dispatch the event to the parent nodes.

We discussed 2 options for a fix:

  1. handling all focus events identically in the scene graph and allowing them to bubble, or
  2. adding a second focusin input event that would bubble

The main issue with option 1 is that there may be cases where allowing focus to bubble in all cases could either cause unanticipated behavior or lead to performance issues (e.g. for deeply nested nodes, checking for listeners on all parents could be an issue).

The second case would complicate the a11y API but would also mimic the DOM spec.

@mbarlow12
Copy link
Contributor Author

My inclination is to go with option 1 as it's a simple boolean switch. Additionally, since we're only using the focusin browser event, you could argue that Scenery is only aware of a bubbling focus event, so that would maintain consistent behavior between the PDOM and Scenery.

@jessegreenberg
Copy link
Contributor

I am on the fence. I agree that it will be easy for devs to forget that there is both focus and a focusin event. But after thinking about it more, I realized there may still be benefit to having both.

Lets say that focus bubbles and the NumberControl catches that event when its HSlider gets focus. In that case, NumberControl's primary sibling will NOT equal document.activeElement, it does not have focus. It seems strange that it would then trigger focus callbacks.

@jessegreenberg
Copy link
Contributor

Because of the above, I am leaning toward option 2, but would be good to discuss again at dev meeting.

@mbarlow12
Copy link
Contributor Author

@jessegreenberg would the advantage of using a focusin event be that the parent node would be aware that one of it's children is receiving focus vs receiving focus itself?

@jessegreenberg
Copy link
Contributor

Exactly! Thats what I was going for in the example in #920 (comment)

@jessegreenberg
Copy link
Contributor

Discussed during 1/7/19 dev meeting:

We will proceed with supporting both focus event and focusin event. We discussed just using focusin as well for clarity, but didn't want to limit ourselves to this as we may have cases in the future where we do not want a focus event that bubbles.

@jessegreenberg jessegreenberg changed the title Focus handling when parent nodes don't have PDOM representations Add a focusin event that bubbles when focus changes Jan 7, 2019
@mbarlow12
Copy link
Contributor Author

  • add the following to Input.js
// DOM spec is that focus events do not bubble while focusin events do
// allows for parents/composite Nodes to add listeners when children recieve focus
this.dispatchEvent( trail, 'focus', this.a11yPointer, event, false );
this.dispatchEvent( trail, 'focusin', this.a11yPointer, event, true );
  • test the bejesus out of it

@zepumph
Copy link
Member

zepumph commented Jan 8, 2019

Ideally we will add this change before the next developer meeting. Marking high priority.

@jessegreenberg
Copy link
Contributor

We need to get through #921 before we can finish this, marking this on-hold.

@zepumph
Copy link
Member

zepumph commented May 19, 2020

The usage that originally caused this issue is no longer in the project. I think that we should close this issue. I'll meet you all over in #921

@zepumph zepumph closed this as completed May 19, 2020
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

3 participants