Checkboxes check state can't be overwritten by server reliably

[benwilson512]

Environment

• Elixir version (elixir -v): Erlang/OTP 22 [erts-10.6.2] [source] [64-bit] [smp:8:8] [ds:8:8:10] [async-threads:1] [hipe]

Elixir 1.10.0 (compiled with Erlang/OTP 22)

• Phoenix version (mix deps): phoenix branch 1.4
• Phoenix LiveView version (mix deps): 0e91c2576255124c912c7cdc5f94d2fae871d5ad (master HEAD)
• Operating system: OS X

Actual behavior

See: https://github.com/benwilson512/bug_example/blob/master/lib/demo_web/bad.ex

On page load there are two checkboxes, one checked, one unchecked. Clicking the submit button inverts the checked state of each. If you click on a check box, that checkbox is no longer affected by clicking the submit button, but the other is. Whichever is clicked last is unable to pick up changes.

It feels like a focus issue based on that description, but adding a text field or some other input to capture focus doesn't change anything. Perhaps related to https://github.com/phoenixframework/phoenix_live_view/issues/611 ?

Expected behavior

I should be able to override the checked state.

[snewcomer]

@benwilson512 I think I might have an idea what is wrong. Looking at another minor fix as well so hopefully can have it fixed. In the meantime, I linked an issue that is related...

[josevalim]

@benwilson512 can you please check if the merged PR address this and if not, please let us know?

[benwilson512]

Hi Jose, I do not see this issue fixed, nor what I think is a related issue where selects spaz out in firefox each time the page updates.

Checkbox issue: https://github.com/benwilson512/bug_example/blob/master/lib/demo_web/bad.ex Select in firefox issue: https://github.com/benwilson512/bug_example/blob/select-variant/lib/demo_web/bad.ex

[benwilson512]

Looking at the checkbox issue in firefox is actually very interesting, because when you click "invert" after touching one of the check boxes, the touched check box border flashes, and its value (as in chrome) doesn't change. This is true even if you switch focus to a text box on the page. I spent some time saturday trying to trace this stuff down but I'm pretty out of my depths on this sadly.

I'm around today if you want help reproducing.

[snewcomer]

Note: the merged Pr is only part of the issue. Will try and get to the remaining today!

[benwilson512]

Noted, thanks for the update!

[snewcomer]

@benwilson512 This is an interesting situation! Initial thoughts include - Are we straying a bit from how a native checkbox in forms work? Should we be explicitly updating the checked attribute on "click"?

Checkboxes get initial state from their "attribute" and then throughout the lifecycle of the form, have there "property" (idl property) toggled (which is another form of state). That is...the checked "attribute" (which is different than "property") is only the initial state and not indicative if the checkbox is currently checked.

So the boundary of what an LV example should do is at most "reset" the default state of the checkbox with the server response on "submit". However, updating the checked attribute on "change" seems to be circumventing (unnecessarily?) the built in state management of the checkbox "attribute" and "property".

So if I remove the "change" handler, everything seems to work. But perhaps you have a different situation you are trying to solve? Do you have further thoughts?

[benwilson512]

Hey @snewcomer thanks for giving this a look.

Some of this has to do with how we're using check boxes as basically "buttons" by having a label with an icon around the checkbox, and then hiding the actual checkbox input. We could achieve the same thing with phx-click on tags, but if you do that, you have to embed a bunch of values on the tag so that you can have it correlate to the relevant part of the form, whereas a checkbox will be nested in the right place to begin with. Using a checkbox also has the perk of storing the form state in the dom, so we get the new phoenix auto recovery for free. If using an tag we'd have to add an additional hidden input to capture whatever change it triggers.

Some of this though feels like a straight up bug, I'll describe it here and then try to update the example to be more compelling. If you have:

<%= for thing <- @things do %>
<div>
checkbox for thing
other form stuff for thing
</div>
<% end %>

If I have two things, and I check thing 1's checkbox and then in handle_event remove thing 1 from the @things list, thing2's checkbox will show as checked now. I'm not sure why, and even adding ids to the divs don't help. We've had to put id="unique-stuff-here-<%= @counter %>" on every checkbox, and then we increment @counter on every form change to ensure that the checkboxes don't steal their checked property from the wrong place.

check boxes as basically "buttons"

Seems like a cool use case! I'm trying to figure out if it s a valid one based on how LV and JavaScript interact && the differences between checked attribute and property. Specifically, this conditional won't actually update the checked property after "change". It just updates the attribute (the default value)

<%= if @check1, do: "checked" %>

Normally toggling a checkbox with a click updates the property only.

Invert a few times (toggling the attribute and in this case the property), click so it turns on (turn on the property and in this specific example the attribute as well), and invert again so the attribute should be off . Notice that it is still "on" b/c even though we turned "off" the attribute, the only way to turn the checkbox truly "off" is with JavaScript and updating the property: input.checked = false. Basically a non programmatic action is not overridden with a programmatic action not using JS.

// document.querySelector('[name="check1"]').checked // true
// document.querySelector('[name="check1"]').getAttribute('checked') // null

We've had to put id

I'd be curious to hear more about this!

Lastly I have a PR that may? solve your issue. I'm still trying to determine if this is safe or not...

https://github.com/patrick-steele-idem/morphdom/pull/177

[chrismccord]

I think LV should set the property based on what we receive from the server, effectively making the attribute coupled to the property on updates. @snewcomer can you think of a reason not to do this? We'd simply set the property onElUpdated so easy for us to handle on the LV side without modify morphdom behavior. Thoughts?

[snewcomer]

@chrismccord This would technically be a breaking change? Those with <input type="checkbox" /> would have their possible checked state overridden?

If that is a route you definitely want to go, I'm ok with it! Small surface area so I can keep exploring other options in the meantime and if a better fix comes up, we can integrate that.

[chrismccord]

The programming model of LV demands that the user maintain form input state on the server based on form data it receives on the client so I wouldn't consider it a breaking change. If the untracked state was held on the client, it worked by accident before.

[benwilson512]

I can confirm that #627 fixed our issue, thank you!