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

Dedicated API for controlled inputs #79

Closed
raquo opened this issue Jan 19, 2021 · 2 comments
Closed

Dedicated API for controlled inputs #79

raquo opened this issue Jan 19, 2021 · 2 comments

Comments

@raquo
Copy link
Owner

raquo commented Jan 19, 2021

See gitter for previous discussion and some proposed solutions.

Basically, if you have a "controlled" input element (to borrow a React.js term), and you want to change its value (e.g. remove non-digits) after the user types, the most straightforward way to do that in Laminar doesn't work if input value is feeding from a signal or a var. Two compounding reasons for this:

  1. you can't preventDefault onInput, it's not a cancelable event in the JS DOM, and

  2. signal doesn't emit a value that is == to previous value.

So when user types in an "invalid" character like "n", the browser updates the input value, but the desired input value, controlled by the signal, doesn't change (if it was "12" and the user typed an invalid character like "n", it will remain "12"), so the signal doesn't emit anything, and so what the user typed is allowed to remain in the input, and the input's value diverges from the Var that is supposed to feed it.

I'm trying to find a reasonable solution to this. My latest thinking is, broadly speaking, to force-set the input's value to the controlling signal's current state after the onInput event has "finished processing" (unless at this point the input's value already matches the controlling signal's current value of course, in which case we don't need to do anything).

I think, if we define "finished processing" as "the transaction in which onInput event happened, and any descendant transactions created from it have completed", which in turn probably resolves to "wait until the transaction queue is empty" in case of transactions started from browser events, this should net us the right outcome. I'm still experimenting, and might run into issues, e.g. with multiple onInput listeners (because each onInput listener will create a separate transaction...), but at least it's looking promising.

After looking into this, I am rather annoyed by the status quo, so I'll try to get this into 0.12.0, although I might publish 0.12.0-m1 before that.


If you're wondering how React does controlled inputs, their state does not have a == check, which is a prerequisite for this problem. Also their event system is custom built so they have more control over it (Laminar just uses native DOM events). But if you're curious you can start here I think.

@raquo
Copy link
Owner Author

raquo commented Feb 10, 2021

Alright, after a lot of experimentation, looks like the following API will probably make it:

// Example
val textState = Var("")
val boolState = Var(false)
div(
  // Text input
  input(
    controlled(
      value <-- textState.signal,
      onInput.mapToValue.map(_.filter(Character.isDigit)) --> textState.writer
    )
  ),
  // Checkbox
  input(
    typ("checkbox"),
    controlled(
      checked <-- boolState.signal,
      onClick.mapToChecked --> boolState.writer
    )
  )
)

What it does is make sure the value property always matches textState.signal, so basically behaves like React controlled components. This requires some magic (not too bad tho), so the new API is scoped to this controlled construct that accepts two parameters. Only one controlled block per element is allowed. Existing usage patterns of Laminar will be still available and unaffected.

I'm also considering adding a custom onEdit key so that users don't need to think whether they need to listen to onClick or onInput or onChange on checkboxes and stuff, but not sure if I'll be able to get it to work. Maybe later.

@raquo
Copy link
Owner Author

raquo commented Feb 12, 2021

Explanation for why a dedicated controlled block is needed from gitter:

Consider this:

val bus = new EventBus[String]
input(
  value <-- bus.events,
  inContext { thisNode =>
    onInput.mapTo(thisNode.ref.value).filter(v => v.forall(Character.isDigit)) --> bus.writer
  }
)

Ultimately what we want is for the input's value to match the value in bus.events at all times. But we can't really achieve it this way.

If you enter a non-digit character like m, the filter will block it, and bus.writer will never receive the event, so it will never emit anything (even though EventBus doesn't do the == checks). But since you can't preventDefault onInput events, that m will remain in the input box.

Given this, well, we could detect such cases, when onInput fires but is blocked by a filter, but... What if the filter is on the observer side, e.g. if we have onInput.mapTo(thisNode.ref.value) --> bus.writer.filter(...)? Well we could look at what observable value <-- is listening to, and reset the input's value to the last emitted value from that observable.

So I actually had a PoC setup with this logic, and it works, except if you have more than one onInput listener. Because if you do the required operations for one of those listeners, the second listener will not be able to read the value that the user inputted (because it's only available as thisNode.ref.value, which we overrode after the first listener fired (see the logic above). And this is a problem because... how is Laminar supposed to know which one of your onInput listeners it should process first? Order of onInput modifiers matters here in a way that isn't obvious to users not familiar with these internals.

So instead, the new API will do all this magic only on the one onInput listener that the user will put inside of the controlled() block. Other onInput listeners will then fire only after this main onInput listener is processed. So they will still see thisNode.ref.value after it's processed – so they won't see the m the user typed if it was filtered out, but at least the execution is very predictable in this sense.

Also, this will be a smoother migration for users since their existing components will continue working as-is. Later as we get more experience with the controlled API we might be able to find a nice way to do it without a dedicated block, but so far I looked very hard and I haven't found it, the edge cases are just unacceptable.

raquo added a commit that referenced this issue Feb 16, 2021
raquo added a commit that referenced this issue Feb 16, 2021
Lots of changes, have not documented everything yet.
raquo added a commit that referenced this issue Feb 26, 2021
@raquo raquo closed this as completed in 4efa422 Feb 26, 2021
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

1 participant