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

Fix for hasFocus / checked binding if not applied via DataBindProvider #208

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mcselle
Copy link
Contributor

@mcselle mcselle commented Feb 28, 2025

Problem

An observable on hasFocus binding would not fire on focus changes if the binding was not applied via the dataBindProvider (e.g. LegacyBinding)

Testcase

[...]
const createElementWithHasFocusBinding  = {
  init: (element: HTMLElement, valueAccessor: () => any) => {
    let parent = $(element);
    let $hasFocus = valueAccessor();
    
    applyBindingsToNode(parent[0], { hasFocus: $hasFocus })
    }
  };
options.bindingProviderInstance.bindingHandlers.set("customBinding",  createElementWithHasFocusBinding)

testNode.innerHTML = `<input data-bind='customBinding: customProps' />`

var $hasFocus= observable(false)
applyBindings({customProps: $hasFocus}, testNode );
[...]

Fix

//  in hasFocus.ts
[...]
var modelValue = valueAccessor(isFocused, {onlyIfChanged: true});
if (isWriteableObservable(modelValue) && (modelValue.peek() !== isFocused)) {
  modelValue(isFocused);
}
[...]

Found this bug while migrating from ko to tko, we have a custom LegacyBinding that applies bindings itself.
So its not a DataBindProvider which would otherwise trigger Parser.ConvertToAccessor() and therefore override the value function.

// in parser.ts
  convertToAccessors (result, context, globals, node) {
    objectForEach(result, (name, value) => {
      if (value instanceof Identifier) {
        Object.defineProperty(result, name, {
          value: function (optionalValue, options) {
            const currentValue = value.get_value(undefined, context, globals, node)
            if (arguments.length === 0) { return currentValue }
            const unchanged = optionalValue === currentValue
            if (options && options.onlyIfChanged && unchanged) { return }
            return value.set_value(optionalValue, context, globals) // <------------------------ THIS is missing
          }
        [...]

I guess after #202 is merged it would be best to migrate the hasFocus / checked / etc. Bindings like the value Binding which already inherites from BindingHandler that basically contains the same logic as my quick fix

// in BindingHandler.ts
get value () { return this.valueAccessor() }
set value (v) {
  const va = this.valueAccessor()
  if (isWriteableObservable(va)) {
    va(v)
  } else {
    this.valueAccessor(v)
  }
}

Problem: An observable would not fire if the foucs / checked state changed if the binding was not applied via the databindProvider
Copy link
Contributor

@phillipc phillipc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work! I know it was not easy to find.

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

Successfully merging this pull request may close these issues.

2 participants