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

Providing better way(s) to control the checkedness of a checkbox #205

Closed
SFBdragon opened this issue Sep 13, 2024 · 6 comments · Fixed by #206
Closed

Providing better way(s) to control the checkedness of a checkbox #205

SFBdragon opened this issue Sep 13, 2024 · 6 comments · Fixed by #206

Comments

@SFBdragon
Copy link
Contributor

In summary, the current state of dealing with the checkedness of checkbox input elements in percy is unsatisfactory. Using the checked attribute is misleading (as it only controls default checkedness, not checkedness), and percy offers no easy way to control the actual checkedness of an element (instead, one would need to manually call set_checked and use e.prevent_default() in an onclick handler or similar workarounds).

There is a decision to be made, assuming that we're in agreement about rectifying the situation. Should we enforce specifying the checkedness of a checkbox? I've highlighted this question in bold below, and discussed the motivations and implications in the section "Should we provide a way to specify default checkedness?".

I've implemented most of the changed already, the design just needs to be finalized before I finish it and open a PR.


This situation is a bit confusing to write about so I'm going to define a few terms:

  • percy-checked refers to the HTML attribute-like value specified using html-macro, e.g. let vnode = html! { <input type="checkbox" checked=true> };
  • checkedness refers to the actual visual state of a useragent-rendered checkbox, unless otherwise specified. This corresponds to elem.checked() but not necessarily to elem.has_attribute("checked")
  • checked attribute refers to the HTML attribute checked which determines the default checkedness of the element as described in the standard. The checkbox's checkedness corresponds to the presence of the checked attribute until the dirty flag is set
  • The dirty checkedness flag refers to the standard's definition, which is set upon user interaction or when changing the checkedness of a checkbox using elem.set_checked(boolean_val) (if boolean_val == elem.checked() the dirty flag is not set). When the dirty flag is set, the checkedness no longer corresponds to the default checkedness, i.e. it no longer corresponds to the presense of the checked HTML attribute.

Here's a concrete example of these concepts using percy:

let vnode = html! {<input checked=true>};
let mut events = VirtualEvents::new();
let (input_node, enode) = vnode.create_dom_node(&mut events);
events.set_root(enode);

let input_elem = input_node.dyn_ref::<HtmlInputElement>().unwrap();
assert!(input_elem.checked());
assert!(input_elem.default_checked());
input_elem.remove_attribute("checked").unwrap(); // change the default checkedness
assert!(!input_elem.checked()); // dirty flag is not set, so checkedness should update
assert!(!input_elem.default_checked());
input_elem.set_checked(true); // sets dirty flag and changes checkedness

// dirty flag is set, so checkedness no longer corresponds to default checkedness
// set_checked doesn't change the default checkedness!
assert!(!input_elem.default_checked()); 
assert!(input_elem.checked());

Currently, percy-dom provides percy-checked. This is the extent of how percy allows for controlling the checkedness of a checkbox input element. percy-checked currently controls whether percy sets or removes the checked attribute from the HTML elements, which determines the default checkedness of the checkbox. It does NOT specify the checkedness of the checkbox (though if the dirty flag is not set, the checkedness changes if the default checkedness changes).

So if you write html! {<input checked=true>}, you specify the default checkedness only. This leads to confusiion, as developers tend to assume that this would set the checkedness too, and naively it might seem that it does until you interact with the checkbox, at which point it will uncheck, despite continuing to render html! {<input checked=true>}.

Notably, there is no way to specify the checkedness of a checkbox. If the user clicks on your checkbox, percy no longer offers a way to control the checkbox. Developers using percy may use elem.set_checked(boolean_val) before/after each render to make up for this, which is not ideal and is easily-forgotten boilerplate required for every checkbox.

In summary, the problems are:

  1. html-macro doesn't allow for expressing the checkedness of the checkbox.
  2. percy-checked misleads developers of its actual function: default checkedness.

This motivates a change to percy-dom and html-macro.


Regarding solving issue (1.):

Is specifying the checkness useful?

Any time a backend exists that maintains a boolean value that's displayed by the checkbox, it would be better to render the client backend's value, rather than the useragent-checkbox state.

  • If the backend and UI both have independently maintained states, desyncs/bugs are almost inevitable.

As an example I've been personally dealing with, a remotely-persisted boolean value's checkbox's checkedness should correspond to the boolean value of the remote data, insofar as is possible (i.e. what the client knows of the server state). Clicking the checkbox should not update the checkedness immediately. Checkedness should only change once the server lets the client know that the authoritative value (the server's value) was successfully changed (if the user loses connection before the request to change the bool was sent, then clicking the checkbox ideally wouldn't change its checkedness).

I think it's best to be able to control checkedness easily when using percy. I will discuss how to go about that further on.


Regarding solving issue (2.):

Assuming we allow developers to set default checkedness: percy-checked probably should be renamed from checked to something else, such as default_checked which clearly communicates its behavior. html! {<input default_checked=true>} Notably React takes this approach of having a defaultChecked attribute.

Should we provide a way to specify default checkedness?

This is assuming we allow developers to specify default checkedness. When is specifying default checkness useful?

  • The default checkedness only matters if the checkbox checkedness isn't being explicitly specified, and the dirty checked flag is not set. The dirty flag is set upon the user changing the checkbox state, or a form reset triggering the checkbox to reset, details of the reset algorithm can be read here.
  • If checkedness is always specified or concretely implied (e.g. hold the last checkedness value, or false if never specified), it's not useful.

If the checkbox is being useful; i.e. it's value might be used in some way while it's being used or interacted with AND checkedness is not being explicitly specified, it means that the developer is implicitly or explicitly having the user agent's checkbox state be authoritative.

There are a couple of dubious "benefits" to this with negative implications for nontrivial projects:

  • It may be easier for small projects to avoid maintaining extra state, and instead rely on the useragent to maintain the boolean value in the frontend. This is often an antipattern though, especially for any project that isn't trivial, as this leads to strongly coupling the backend to the frontend as well as spaghetti code.
  • It's easier to take advantage of some browser features, like resetting a form (which reverts the checkbox to its default state) will work automatically without any backend code. Again, because the backend is treating the frontend value as authoritative, this approach leads to hair loss for the same reasons mentioned previously.

So the choice is, do we expect all apps using percy to have a fully authoritative backend that handles all desired functionality of the checkbox explicitly? (It's checkedness, form resetting.) Or do we allow for developers to make the checkbox state authoritative?

Notably, this is a WASM Rust project, which sees a lot less "I just want to bodge something together quickly" than Javascript does. The best choice for a VDOM Javascript framework may not be the best choice for a Rust framework, as the projects that use JS and HTML directly without a backend, where specifying the default checkedness is easier than the checkedness.

[Vague] Usage information

Explicitly specifying defaultChecked or writing related code even just with web frameworks that have it, or when writing functions that do something related, returns 110k results using GitHub code search. https://github.com/search?q=defaultChecked+language%3AJavaScript+&type=code&p=2 This does not include anyone who sets the default checkedness by setting/removing the checked HTML attribute.

A similar seach for "default_checked" and "defaultChecked" return about 450 results in total.

Note that most of these results are not clearly related to this issue explicitly. It's noise. It's also heavily confounded by many variables, obviously. I'm not sure how to best assess this. My experience with web development is limited.


If we do NOT allow for specifying the default checkedness easily AND allow the developer to NOT specify checkedness:

A. percy chooses a default and let the user agent set the checkbox as it would normally. This default can be overridden with elem.set_attribute("checked", "") and elem.remove_attribute("checked").
B. percy always uses the last-specified checkedness, and applies its own default if checkedness has not yet been specified.

These are the two most sensible possibilities I can think of.

(A) is not a good choice. It makes setting the default harder than necessary, while still having all of the pitfalls of the checkbox state either being out-of-sync with the authoritative value or authoritative itself.

(B) is similarly awkward. It might go out-of-sync with the authoritative value or be authoritative itself. Developers would be relying on percy to maintain the checkedness value. It also makes setting the default harder than necessary.

I don't think NOT allow for specifying the default checkedness easily AND allow the developer to NOT specify checkedness has any benefits over allowing the developer to specify default checkedness. Therefore I don't think it should be considered.


If we do NOT allow for specifying the default checkedness easily AND do force the developer to specify checkedness, then the behavior of the checkbox is simple. We just need to ensure that all <input type="checkbox> elements have a defined checkedness in their definition.

This is definitely the right choice for my use of percy, but it would inconvenience developers who want to use the useragent checkbox state as authoritative, presumably because it's easier for a tiny project.

This is a tradeoff percy needs to make: does it force the checkedness to be explicitly specified or not?

  • If specifying checkedness is enforced, then having a way to specify default checkedness is unnecessary.
  • If specifying checkedness is NOT enforced, then we want to provide a way to specify default checkedness.

Explicitly specifying checkedness

Given that we want to explicitly specify checkedness with html-macro+perdy-dom there are a couple implementation details to be explicit about:

  • If the user clicks on a checkbox, the browser will change its state. However, the developer might either 1) have not have patched the VDOM yet 2) might have not changed the checkedness when they do patch. Therefore, we need to force the checkbox to maintain it's value. There are some possibilities I'm aware of, all requiring adding some code to onchange: event.prevent_default() or using JS's timeout with a duration of zero to quickly reset the checkbox to the patched state. I'm not aware of the tradeoffs between these two approaches precisely.
  • If we patch a checkbox with a new value, we need to explicitly call set_checked as adding/removing the checked HTML attribute only changed the default checkedness, not the checkbox's checkedness.
@chinedufn
Copy link
Owner

Generally speaking, percy prioritizes making hard projects easier, sometimes at the expense of making easy projects harder.

For instance, most virtual-dom libraries allow you to attach some internal state to your view components.
percy has no such concept.
It heavily discourages per-component state and instead encourages users to have a single place that all of their State is stored.
This makes it much easier to simulate an entire application programmatically, since you can easily setup the application in any State that you wish
by interacting with that one State object.

I'm not going into details here since I plan to some day document these ideas more thoroughly in the percy repository.
My main point is that we generally won't prioritize the needs of the simple project if it would harm the experience of maintaining a complex project.


If we do NOT allow for specifying the default checkedness easily AND do force the developer to specify checkedness,
then the behavior of the checkbox is simple. We just need to ensure that all elements have a defined checkedness in their definition.

If a user needs to control checkedness they will set a checked property themselves.

One benefit to enforcing the checked property it would be catching typos where they types checked_oops=false.
But, this typo-catching could also be enforced by just not allowing html! macro users to use non-standard attributes unless they begin with "data-my-custom-attribute".

So, unless there is another benefit other than catching typos, forcing checked to be specified would only be useful for a first-time user with little HTML experience
that knows to add type="checked" to their checkbox element but doesn't know how to actually set the value.
The compile time error would guide them towards adding a checked={bool} property.

If this is a real segment of percy users, it is likely tiny enough to be negligible.
Therefore, we would only want this if there were no trade-offs.

There is a trade-off.

By not forcing a checked property to be specified, a user could implement default_checked themselves:

<input
    type="checked"
    // Set the default checkness to true
    on_create_element=|elem| {
        elem.set_checked("checked", true)
    }
    // Handle user clicking to change the checkness
    onchange=|e| {
        let checked = e.target().checked();
    }
/>

Whereas if we forced a checked property then the above default_checked implementation wouldn't be possible.


I spent a few minutes thinking and couldn't come up with a legitimate use case for allowing users to implement default_checked themselves using
on_create_element.

So, it's possible that there aren't any legitimate use cases for it.

Still, since forcing checked to be provided doesn't offer any meaningful benefits over just banning non-standard attributes (or at least somehow banning them by default)
(as explained above, the benefit to requiring checked would be preventing typos, but banning unrecognized attribs by default would also prevent typos),
it is best to allow this default checked implementation to be possible.

So, we should not require checked to be provided.

Therefore, we need to force the checkbox to maintain it's value

We should start by just making this the user's responsibility.

The user already needs an onchange handler, so they can easily add e.prevent_default() to it.

Now, of course the problem is that it's easy to forget to do that.

So, we'd want to eventually either design a way for it to be impossible for the user to forget to take full control of checkedness, or to have automatically
have percy enforce the checkedness.

For instance:

  • We might require that either e.prevent_default() is called or e.i_am_intentionally_not_preventing_default() is called.
    This could be accomplished by requiring the user to return a type that can only be created by calling one of those two methods.

    type OnChangeHandler = Fn() -> ExplicitlyDecidedWhetherToHandleCheckedness;
    
    enum ExplicitlyDecidedWhetherToHandleCheckedness {
        Yes,
        No,
    }
    
    onchange=|e: NewTypeWrapperAroundTheEvent| {
        // returns `ExplicitlyDecidedWhetherToHandleCheckedness::Yes`
        e.prevent_default()
    }
  • Or we might decide something else ...

My main point is that we can start by making dealing with the browser automatically changing checkedness the user's responsibility,
use it ourselves a bit, get a sense of the footguns, and then figure out the best way to solve for them.

Then in the meantime percy can just document this problem and leave it as a TODO / open area of research.

Recommendations

  • make the checked attribute control the element's .checked property

  • don't force checked to be set

@SFBdragon
Copy link
Contributor Author

SFBdragon commented Sep 22, 2024

Generally speaking, percy prioritizes making hard projects easier, sometimes at the expense of making easy projects harder.

Sounds good.


So, unless there is another benefit other than catching typos, forcing checked to be specified would only be useful for a first-time user with little HTML experience

I believe it's primary benefit would be discouraging the reliance on the browser's checkedness model, which is likely to be a footgun for any project with a single place where application state is stored.

Not specifying checked as a way to opt out of the predictable model where checkedness is always specified is intuitive, however.


We should start by just making this the user's responsibility.

That works for me, turns out specifying prevent_default wasn't doing anything meaningful in our case.

I'd like to note that I think making this the default behavior would be beneficial for a similar reason as mentioned above, not allowing the checkbox's state to change if the user specified checked is the correct default for an application who's UI state should correspond to a distinct application state. Regarding the design of this, I think specifying an attribute such as something like checkedness_toggles_on_click that forces percy not to apply this behavior would be easier for the developer.

For example:

let checked_checkbox = html! {<input type="checkbox" checked=true>};
let checked_checkbox_that_may_change = html! {<input type="checkbox" checked=true checkedness_toggles_on_click>};
// This is readable. This is also likely to happen if there's a condition where `checked` is intentionally unspecified
// based on a condition, where `checkedness_toggles_on_click` behavior is expected regardless of whether `checked`
// is specified or not.
let unspecified_checkbox_that_may_change = html! {<input type="checkbox" checkedness_toggles_on_click>};
// This is the odd one out, but it doesn't seem reasonable that a developer would
// intend to maintain the checkbox in an unspecified state.
// It's much more likely that the developer did not want to control the checkbox behavior themselves,
// and so specified nothing related to the checkedness.
let unspecified_checkbox_that_may_change = html! {<input type="checkbox">};

Again, no need for this now from us, but if further consideration is made on this in the future, I would like to suggest this alternative.


I'll submit a PR with the recommended changes shortly 👍

@chinedufn
Copy link
Owner

chinedufn commented Sep 23, 2024

Definitely an interesting alternative.
Definitely makes me even more confidence that there would be some design work required to nail this down further since even off the top of our heads there have already been multiple options that were at least worth mentioning.
So, cool, yeah let's add that to the consideration stack.


Cool, planning to review today or tomorrow on Tuesday or Wednesday.

@SFBdragon
Copy link
Contributor Author

SFBdragon commented Sep 24, 2024

As notes in a comment in the PR here #206 (comment)

I wasn't accounting for server-side rendering (SSR) at all.

That's fine for the behavior that's being changed in the PR (except for a detail that's currently unresolved, as discussed in the comment linked above). But in case any of this is referred to in the future, it's worth clarifying the problems.


It's worth noting that I might be misunderstanding how SSR is typically used, as while I've read about it here and there, I've never worked with it.


This is noteworthy because the best defaults for SSR and CSR are completely different (if I'm understanding SSR correctly, and how developers would use percy to perform SSR):

  • The default for SSR should be that the user gets the HTML they expect, and the browser does with that HTML what it usually would. This means that checkboxes should switch states if onchange/onclick/etc. doesn't explicitly specify some other behavior. SSR is likely to be used when there's no substantial "application" running on the client, therefore no application state to control the checkboxes.
  • The default for CSR would preferably be that the application has complete control of the GUI state, which is fully determined by the application state. No allowance for the application's state and the GUI state to desync.

Therefore, checkedness_toggles_on_click or a similar solution to keeping the checkbox the same as the rendered state as proposed above wouldn't be a good default for SSR as-is.

So, unless there is another benefit other than catching typos, forcing checked to be specified would only be useful for a first-time user with little HTML experience

I believe it's primary benefit would be discouraging the reliance on the browser's checkedness model, which is likely to be a footgun for any project with a single place where application state is stored.

Not specifying checked as a way to opt out of the predictable model where checkedness is always specified is intuitive, however.

This is also probably the wrong default when doing SSR.


It's mildly concerning that percy would attempt to address SSR and CSR homogeneously when percy's goals for CSR (application state is king) contradicts the modus operandi for SSR: serve the HTML and keep the client-side minimal.

@chinedufn
Copy link
Owner

chinedufn commented Sep 24, 2024

The default for SSR should be that the user gets the HTML they expect, and the browser does with that HTML what it usually would.

Not necessarily.

percy isn't focused on developers that need a simple SSR solution with no client-side behavior.

It supports that, but it isn't the primary objective.

The primary objective is supporting use cases where:

  1. Server renders the HTML
  2. Server serves the HTML
  3. Client-side replaces the HTML that the server sent down with client-side rendered elements

Often referred to as "server-side rendering with client-side hydration".

SSR is likely to be used when there's no substantial "application" running on the client, therefore no application state to control the checkboxes.

We're designing for the case where SSR and client-side rendering are both used together.

For instance:

  1. Server application reads data from backend database
  2. Server application renders HTML
  3. Client receives HTML
  4. Client immediately sees content
  5. Client-side code starts running
  6. Client-side code makes HTTP requests to download data
  7. Client-side has the data it needs
  8. Client-side replaces the HTML elements that the server sent down with HTML elements rendered client-side
  9. As state changes, the client keeps the page updated

So, percy is aimed at the problem:

(rough sketch) How do I build an application that shows users content as quickly as possible, while also remaining maintainable as the application's scope grows tremendously.


The default for CSR would preferably be that the application has complete control of the GUI state, which is fully determined by the application state. No allowance for the application's state and the GUI state to desync.

The server-side rendered HTML is also controlled by application state.

We want the percy user to be able to run the same exact application code on the server, or on the client, with as close to zero changes as possible, unless there is a very good reason for a divergence.

This helps solve for "while also remaining maintainable as the application's scope grows tremendously".

when percy's goals for CSR (application state is king) contradicts the modus operandi for SSR: serve the HTML and keep the client-side minimal.

This is the modus operandi of frameworks and libraries that market the idea of "keeping things simple" by keeping all of the logic / rendering server-side.
There is currently something of a movement for simple applications that aren't "over-engineered".
percy is not part of this movement. percy is squarely aimed at very complex applications that could never be fully server-side rendered, but still want the benefits of SSR (probably either SEO or faster time to first paint).

The percy project believes that the ceiling on those kinds of applications is very low. For some folks that might be okay.

So, percy's goal is not to avoid complexity by keeping the client-side minimal.
Rather, the goal is to reduce complexity without reducing the ceiling of the amount of functionality that one can implement while the application still remains reasonably maintainable.

This is why we attempt to treat SSR and CSR the same. It reduces the amount of effort required to solve the core problem:

  • (rough sketch of core problem) How do I build an application that shows users content as quickly as possible, while also remaining maintainable as the application's scope grows tremendously.

The default for SSR should be that the user gets the HTML they expect, and the browser does with that HTML what it usually would

The default for SSR should be that it's very easy to start with SSR, and then have your client-side code pick up from there.

So, app running server-side and same application code running client-side both start with the same application state, meaning they render the same thing, meaning there is a seamless transition.


The least surprising for the user, both for SSR and CSR is:

  • checked={true,false} controls the "checked" attribute
  • checked={true,false} controls the ".checked" property
  • In case that you want to set default checkedness on the server, but then aren't using percy on the client, it means that you're accessing the element in some other way (maybe JavaScript, or jQuery or whatever) or not at all (you're just rendering a form or something).
    • You have the ability to do whatever you want to the element. If you aren't running percy client-side then you've left the percy model and can control things however you like.

@SFBdragon
Copy link
Contributor Author

SFBdragon commented Sep 24, 2024 via email

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 a pull request may close this issue.

2 participants