lustre-labs / lustre

An Elm-inspired framework for building HTML templates, single page applications, and server-rendered components in Gleam!
https://hexdocs.pm/lustre
MIT License
727 stars 52 forks source link

disabled=true property toggles on every event #125

Closed pizzathemeguy closed 1 month ago

pizzathemeguy commented 1 month ago

When an element has the attribute attribute.disabled(True) it toggles between disabled and enabled on every event. I suspect this will happen with all the as_property attributes, but I've not tested anything other than disabled.

e.g. In this example every time the "raise event" button is pressed the "I should always be disabled" button toggles between disabled/enabled.

import lustre
import lustre/attribute
import lustre/effect
import lustre/element
import lustre/element/html
import lustre/event

pub type Model =
  Nil

fn init(_flags) -> #(Model, effect.Effect(Msg)) {
  #(Nil, effect.none())
}

pub type Msg {
  UserPressedButton
}

fn update(model: Model, msg: Msg) -> #(Model, effect.Effect(Msg)) {
  case msg {
    UserPressedButton -> {
      #(model, effect.none())
    }
  }
}

pub fn view(_model: Model) -> element.Element(Msg) {
  html.div([], [
    html.button([event.on_click(UserPressedButton)], [
      element.text("raise event"),
    ]),
    html.div([], [
      html.button([attribute.disabled(True)], [
        element.text("I should always be disabled"),
      ]),
    ]),
  ])
}

pub fn main() {
  let app = lustre.application(init, update, view)

  let assert Ok(_) = lustre.start(app, "#app", Nil)
  Nil
}

from what I could tell was happening in vdom.ffi.mjs createElementNode():

  1. attr.as_property on line 235 is true
  2. the call to if (canMorph) prevAttributes.delete(name); that happens for as_property=False attributes is not called
  3. on line 303 the attribute gets removed with el.removeAttribute(attr);

I didn't have a chance to dig into the code more to see if adding if (canMorph) prevAttributes.delete(name); to the as_property=True condition would have other downstream effects, but that at the very least fixed the toggling button I was seeing.

hayleigh-dot-dev commented 1 month ago

Ah thank you for investigating this. I think this might be a regression: I'll take a look :)

hayleigh-dot-dev commented 1 month ago

Hey there I think I've fixed this, could you give 4.2.2 a try and see if the issue is resolved?

pizzathemeguy commented 1 month ago

Gave 4.2.2 a try in the example I posted as well as the project I originally noticed the behavior in; both looked good. Appreciate it, thank you!