lightning-js / blits

The Lightning 3 App Development Framework
https://blits-demo.lightningjs.io/
Apache License 2.0
67 stars 13 forks source link

Documentation: Component Focus/Input handling #163

Open ChrisArasin opened 1 month ago

ChrisArasin commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

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:

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

lol try both every time

michielvandergeest commented 1 month ago

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 (https://github.com/lightning-js/blits/issues/131) which we have started on working on.