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

Documentation: Component Focus/Input handling #163

Open
ChrisArasin opened this issue Sep 18, 2024 · 9 comments
Open

Documentation: Component Focus/Input handling #163

ChrisArasin opened this issue Sep 18, 2024 · 9 comments

Comments

@ChrisArasin
Copy link

ChrisArasin commented Sep 18, 2024

Going by this documentation, I would expect that if a child component doesn't handle an input event, and its parent does, then the child would lose focus (firing the unfocus hook), and parent would receive focus. This does not seem to be what happens.

https://lightningjs.io/v3-docs/blits/components/user_input.html#event-handling-chain

In a Blits app, there is always one Component that has the focus. By default, this will be the root Application component.

When a component handles a key press by having a corresponding function specified, said component receives focus, and the event handling chain stops by default. However, if you want the input event to propagate up the hierarchy further, you can move the focus to the parent element and pass the InputEvent object on in that function call.

@michielvandergeest
Copy link
Collaborator

Hey @ChrisArasin, yes you are right!

Your understanding of the documentation is correct, but it currently doesn't seem to behave like that. The input handler on the parent is executed, but it doesn't move the focus along with it. It stays with the child, which is unexpected of course.

I took a quick look and it seems like a oneliner fix. We should make sure to properly test this, but I think we should be able to get this fixed and released fairly quickly.

Thanks for reporting this! :)

@michielvandergeest
Copy link
Collaborator

@ChrisArasin I've opened up a PR with a fix for this. It was originally a one liner, but I ended up making it a bit more robust overall 😅

We'll be testing the changes throughly. Would appreciate if you could test on your end as well if this fixes the issue.

Thanks!

@ChrisArasin
Copy link
Author

ChrisArasin commented Sep 20, 2024

Ok, with this change I do see focus moving up. So this now behaves as expected.

One thing I'm seeing, that may just be an anti-pattern:

  • Parent component always passes focus to a child
  • but both the parent and child have a key handler (say for direction press right)
  • the child's right handler passes the event to the parent after handling to do something at the parent level

so parent component:

 hooks: {
    focus() {
      // always pass focus to child
      const focusItem = this.$select('targetChild').$focus()
      if (focusItem && focusItem.$focus) {
         focusItem.$focus()
       }
    },
  },
  input: {
    right(e) {
    //  parent right handler for side effect'
    },
  },

Child component:

  input: {
    right(e) {
     // do something in child
      this.parent.$focus(e)
    },
   }

At the moment, I believe on right press I see:

  1. Child right handler fires
  2. Child unfocus
  3. Parent focus
  4. Parent focus hook fires, refocusing the child
  5. rather than the right event being called on the parent, it re-fires on the child

@ChrisArasin
Copy link
Author

ChrisArasin commented Sep 20, 2024

Thinking about it more, that example is pretty contrived. Was generally thinking of the times in Lightning2 when you may handle a keypress but allow the event to propagate. I'm guessing with the way blits handles focus you would probably just want to avoid implementations like that.

@michielvandergeest
Copy link
Collaborator

michielvandergeest commented Sep 20, 2024

Hey @ChrisArasin yeah I've been thinking exactly the same thing while I was testing more examples with this change.

I think passing on the focus to the parent like we fixed now makes sense. And with that, unfocusing the child, also seems natural.

I'm now in doubt whether or not that should (re) invoke the parent focus event. I'm leaning towards not invoking it, because the parent is still in focused state as part of the focus path ..

That behavior would also work in the use case you described, if I understood correctly.

I'll put this up for discussion with the rest of the team. But happy to get your thoughts and input on this as well!

@ChrisArasin
Copy link
Author

ChrisArasin commented Sep 20, 2024

Yeah, don't have a strong opinion yet but my sense would be if unfocus doesn't fire on the parent when focus is passed from the parent to the child, then you wouldn't re-fire focus when the parent receives focus back from the child.

I also could see something more similar to L2 where the child can allow the input event to propagate without explicitly passing focus back up, but that is maybe just because I'm used to L2.

@michielvandergeest
Copy link
Collaborator

if unfocus doesn't fire on the parent when focus is passed from the parent to the child, then you wouldn't re-fire focus when the parent receives focus back from the child.

Yes, that's my thinking as well. The parent isn't unfocused when its child gets focused cause it's part of the focus chain. So re-focusing is not logical when focus is passed back from child.

I do prefer the explicit and intentional focus delegation, compared to assuming focus to bubble up until passed false (or true - I can never remember 😆) to break the propagation as it was done in L2

@ChrisArasin
Copy link
Author

until passed false (or true - I can never remember 😆)

lol try both every time

@michielvandergeest
Copy link
Collaborator

Just wanted to give a quick update on this issue: I'm not sure the provided fix in the PR make sense, after all. It can lead to undesired focus jumps, when a parent handles a certain input event, but you actually want the child to remain focus after handling that event. Much like the pattern you described.

We're doing some re-evaluation to see what the most logical approach is. Also considering mouse pointer support (#131) which we have started on working on.

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