Skip to content

Conversation

@semonec
Copy link

@semonec semonec commented Oct 21, 2025

🐛 Bug Fix

Problem

The onFocus and onFocusChanged callbacks in the focus manager had issues with this binding when using arrow functions. Regular functions worked correctly with call() method, but arrow functions couldn't be bound with this context, causing inconsistent behavior.

Solution

  • Function Type Detection: Added logic to distinguish between regular functions and arrow functions using prototype property check
  • Conditional Binding:
    • Regular functions: Use call() method for proper this binding
    • Arrow functions: Call directly without this binding to avoid errors
  • Safe Function Calls: Added typeof checks to handle non-function values gracefully

Changes Made

  • Modified updateFocusPath function in src/focusManager.ts
  • Updated callback handling for onFocus, onFocusChanged, and onBlur events
  • Added comprehensive unit tests in tests/focusManager.spec.ts

Code Changes

// Before
current.onFocus?.call(current, currentFocusedElm, prevFocusedElm);

// After
if (current.onFocus && typeof current.onFocus === 'function') {
  if (current.onFocus.prototype) {
    // Regular function - can use call with this binding
    current.onFocus.call(current, currentFocusedElm, prevFocusedElm);
  } else {
    // Arrow function - call directly without this binding
    current.onFocus(currentFocusedElm, prevFocusedElm);
  }
}

Testing

  • ✅ Regular function this binding works correctly
  • ✅ Arrow function calls work without this binding errors
  • ✅ Mixed function types work properly
  • ✅ Edge cases (undefined, null, non-function values) handled gracefully
  • ✅ Parent-child focus hierarchy works correctly

Breaking Changes

None - this is a backward-compatible fix that improves existing functionality.

Related Issues

Fixes the issue where arrow functions in onFocus and onFocusChanged props would not receive proper this context, causing inconsistent behavior compared to regular functions.

…onBlur` callbacks with proper this binding for both regular and arrow functions
@chiefcll
Copy link
Contributor

I learned something new today about arrow functions...

You'd still want to change the signature of the call to
current.onFocus(current, currentFocusedElm, prevFocusedElm);

Otherwise you won't be able to change the element you have the onFocus on. I'll update the signature for next breaking release once Renderer has a v3 done...

@semonec
Copy link
Author

semonec commented Oct 21, 2025

I learned something new today about arrow functions...

You'd still want to change the signature of the call to current.onFocus(current, currentFocusedElm, prevFocusedElm);

Otherwise you won't be able to change the element you have the onFocus on. I'll update the signature for next breaking release once Renderer has a v3 done...

But if we do like that, the onFocus callback's definition would be changed.

Currently it's deifniition is onFocus(currentFocusedElm, prevFocusedElm) and the current object includes states of focusState.

So, how do you think about calling current.onFocus(current, prevFocusedElm); instead of currentFocusedElm?

@chiefcll
Copy link
Contributor

Sorry for the delay - But again I'm not sure what this gains us:

if (current.onFocus && typeof current.onFocus === 'function') {
        if (current.onFocus.prototype) {
          // Regular function - can use call with this binding
          current.onFocus.call(current, currentFocusedElm, prevFocusedElm);
        } else {
          // Arrow function - call directly without this binding
          current.onFocus(currentFocusedElm, prevFocusedElm);
        }
      }

These are the same thing. Arrow functions lock down this so this will not be the node. Best way to resolve this would be to always pass in the current node as the first argument, and then this function will always have 3 arguments. I'll work on doing this in v3 and list it in the breaking changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants