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(vdom): don't update value property if it hasn't changed #602

Merged
merged 1 commit into from
Jun 10, 2021

Conversation

glennsl
Copy link
Contributor

@glennsl glennsl commented Jun 8, 2021

This addresses a problem we encountered with number inputs in Firefox, where HtmlInputElement.value will only return a valid number. or the empty string, instead of the actual contents of the input. This causes two problems in typical use, 1) it clears the input if the user enters an invalid character, and 2) it's not possible to enter the decimal point key-by-key since entering e.g. 12. will have value return 12, thereby deleting the decimal point.

The workaround implemented here is to simply not update the DOM if value seems unchanged. In the problematic case above it has, of course, and this will cause the input to become out of sync with the model. But the root of the problem is that it already is, and there seems to be no way to get the actual contents of the input. So it seems to me that the lesser evil is to accept that instead of forcing synchronization by clearing the input.

Note that this is also what Elm does, and has done for years. It was implemented five years ago to address a different issue (that seems to be addressed with significantly more complex logic further down in this function, which we might now remove?). It therefore feels like a relatively safe change, or at least one that has been thoroughly tested in real world scenarios.

There is one known problem with this, however, also only in Firefox. If the input has invalid contents it's no longer possible to clear it by programmatically setting value to the empty string, since it will already appear empty and therefore not actually do anything. This is still unresolved in Elm.

Another option to address this is to implement a more general opt-in mechanism for property-setting, but that seems like a much more invasive change. Although if we look to Elm again, it seems like most attributes are actually implemented using the equivalent property instead of the attribute.

@glennsl
Copy link
Contributor Author

glennsl commented Jun 8, 2021

Here's a simple example to test the issue:

use seed::{prelude::*, *};

fn init(_: Url, _: &mut impl Orders<Msg>) -> Model {
    Model::default()
}

type Model = String;

enum Msg {
    SetValue(String),
    Clear,
}

fn update(msg: Msg, model: &mut Model, _: &mut impl Orders<Msg>) {
    match msg {
        Msg::SetValue(value) => *model = value,
        Msg::Clear => *model = String::from(""),
    }
}

fn view(model: &Model) -> Node<Msg> {
    div![
        input![
            attrs! {
                At::Type => "number",
                At::Value => model,
            },
            input_ev(Ev::Input, Msg::SetValue),
        ],
        button![ev(Ev::Click, |_| Msg::Clear), "Clear"],
        model,
    ]
}

#[wasm_bindgen(start)]
pub fn start() {
    App::start("app", init, update, view);
}

@MartinKavik
Copy link
Member

Thanks for the detailed explanation!

@MartinKavik MartinKavik merged commit 6ec9874 into seed-rs:master Jun 10, 2021
@glennsl glennsl deleted the feat/vdom/property branch June 10, 2021 18:31
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