Change html-macro's checked attribute to specify checkedness
#206
Conversation
…, rather than default checkedness
There was a problem hiding this comment.
Thanks for adding a new book chapter.
Thanks for getting this working.
Minor feedback around tweaking the language in the book chapter.
Minor feedback around adding a diff.rs test.
Other than that looks good to me. Thanks for putting this together. Very solid.
|
Just noting this quickly as it came up in the notification:
Percy only updates the element if it has changed.
This isn't the case here. We don't want to let the checkbox value change if the user clicks on it but app state didn't change. So `.checked` is always set when applying `patch`, regardless of whether the value was updated or not.
…-------- Original Message --------
On 2024/09/24 18:49, Chinedu Francis Nwafili wrote:
@chinedufn commented on this pull request.
---------------------------------------------------------------
In [crates/percy-dom/src/patch/apply_patches.rs](#206 (comment)):
> @@ -241,7 +241,13 @@ fn apply_element_patch(
}
}
AttributeValue::Bool(val_bool) => {
- if *val_bool {
+ // Use `set_checked` instead of `{set,remove}_attribute` for the `checked` attribute.
> If we reverted the behavior, there would be no way to specify the default checkedness besides resetting it after every percy update.
Percy only updates the element if it has changed.
So, if your element always looks like this:
html
!
{
<input
type
=
"checked"
checked=
true
/>
}
then it will always be checked.
So, there is a way to specify the default checkedness. Just set the attribute and never change it. percy will never touch it again if the VirtualNode never changes.
Not saying that relying on this is intuitive. Just mentioning so that we're designing on top of truthful ground.
---------------------------------------------------------------
> The application has a map that holds option-value pairs, it starts off empty. The GUI has a form that holds a bunch of options (with default states)
If you are running client-side code you should be expected to render declaratively.
Your state should mimic what you see.
If you want to deviate from that model then don't create the <input> element using percy.
Just create it yourself and append it to your percy element.
fn
render
(
)
->
VirtualNode
{
html
!
{
<div id=
'
container-
for
-non-percy-elements
'
></div>
}
}
fn
attach_form
(
)
{
let
form = document
.
create_element
(
"form"
)
;
let
input = document
.
create_element
(
"input"
)
;
// ...
let
container = document
.
get_element
(
"container-for-non-percy-elements"
)
;
container
.
append_child
(
form
)
;
}
What I'm proposing above is:
-
if you are running percy client side, we expect that you are using percy's html! macro / VirtualNode::* APIs declaratively.
- As in, whatever your html! says is what the user will see
-
if you want to deviate from that model, do some outside of percy. Create the elements yourself, then append them yourself to a percy element.
- So, say you're at a company that has been using a jQuery form library for 20 years. Cool, no problem. Just call a function to append that to your percy element
- percy won't touch those elements. it doesn't know about them. it only knows about it's VirtualNodes
- unless of course you delete the <div id='container-for-non-percy-elements'></div> that you created using percy
- so, in short, you can embed whatever you want inside of your percy-managed elements. If you need to do weird stuff, embed the elements yourself.
- then it just becomes our job to provide examples and guides that arm people with the knowledge of when and how to leave percy to do edge case things. Using regular old HTML primitives.
- because people will generally think that they have to use the library for everything. You don't. You can side-step it whenever you want. So, for extreme edge cases like this, we shouldn't make percy account for it. The user can just side-step percy.
> A good default for CSR (follow the application state, lock the GUI state to what is explicitly specified using html!)
> isn't a good default for SSR (let the browser do its thing, don't try to lock the GUI state).
I addressed here why this isn't true for complex applications:
[#205](#205)
But, in short, for complex applications SSR rendering must also be based on application state (otherwise you're maintaining two very different rendering environments, double the work), its just that the GUI is an HTML string.
But we want that GUI (HTML string) to be rendered in the same way that we render to the DOM.
Anything that makes CSR and SSR different should be very well justified.
> Let's assume that a developer might want to ensure the checkbox has a specific default checkedness (again, this is exclusively considering CSR, not SSR).
Create the element outside of percy and then append it to your percy-managed DOM tree.
You get exactly what you want, and percy doesn't need a complicated API for a vanishingly small edge case.
Another thing to note about the philosophy behind percy: we aren't trying to hide the DOM for you.
The fact that we give you a virtual dom is because we don't think there's a better way to manage tremendous amounts of application state.
We'd much prefer if instead of learning about virtual doms you could just learn HTML/real-DOM stuff.
So, when real-DOM stuff handles the job well we can just point users in that direction. If the use cases is common we can abstract it for convenience.
This is why I appreciate the technical details in your docs.
We don't want people to have to learn percy. We want them to learn HTML/DOM. Much more useful knowledge.
> can we assume that SSR vnodes will only be created, not diff+patch
yes.
> We just need to ensure that checked sets the HTML attribute when vnode.to_string()
Making SSR and CSR different is undesirable for reasons mentioned above. Unless there's ever a compelling reason.
> Solutions proposals:
Option 4 - percy doesn't handle default checkedness at all and if you want on the client-side the book recommends just creating the DOM element yourself using web-sys.
We some day also create an example of this in the examples/ directory.
This is better for the 99%+ of users that will never care about any of this.
And better for the <1% that do since instead of teaching them a weird percy-specific attribute we can instead teach them that "Hey .. by the way you can just jump outside percy and do real DOM stuff if you like ... here's how to do it for default checknedess".
They are now armed with knowledge that will be useful for all future edge cases that they run into. "If percy isn't designed for my strange use case, I can just write my own code to handle it!"
vs. "reading through docs learning about 20 percy-specific concepts that didn't need to exist."
I operate under the assumption that our users would prefer to do things themselves correctly in a knowledge-transferable way than to have us give them a weird abstraction.
They just need us to point them at how to do it.
—
Reply to this email directly, [view it on GitHub](#206 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/ALAPHRYNWFIS7FTDUMXTP7LZYGJ2TAVCNFSM6AAAAABOU73KSSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGMRVHA2TCMZSHA).
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
|
Update, should submit the changes tomorrow. Currently getting a small append-my-checkbox-with-default-checkedness-to-dom example working, everything else has been changed/rectified. |
|
@chinedufn This commit addresses the above review items. There are some new elements worth reviewing here
I'm anticipating that you'd like some changes to the example but I'm not sure what those would be. |
examples/append-to-dom/src/lib.rs
Outdated
| #[wasm_bindgen_test] | ||
| pub fn main() { |
There was a problem hiding this comment.
Let's make sure to demonstrate the following:
- Doing all of this using the
html!macro - Doing all of this without using the
html!macro - Using
.special_attributes.on_create_elementto demonstrate how to ensure that the unmanaged element gets inserted afterpercycreates the DOM node.- useful when you aren't appending to the root node since you won't have a
div_elem: web_sys::Elementon hand like you do in this example
- useful when you aren't appending to the root node since you won't have a
Let's make the example append the input to a non-root node. This is the more common scenario. Or, feel free to demonstrate both.
I'm imagining that this fn main can call some different functions that each demonstrate some subset of the above list + the things that you are already demonstrating.
There was a problem hiding this comment.
Let's make the example append the input to a non-root node
Turns out this forces a mix of the above points such that there's only really one good way of doing it... sort of. Not using html! at all is possible and isn't demonstrated, but I don't think showing the example without html! is adding anything but verbosity, as html! currently isn't being used for anything that can't be easily manually constructed with VElements.
I can remove the use of html! if you feel strongly about it, or alternatively add another version of the function that doesn't use html!.
I recommend checking the new example first and seeing if you feel its necessary, once I commit it. (I'm not exactly sure why you want two versions.)
There was a problem hiding this comment.
Only using Hybrid approach sounds okay.html! sounds good.
Agree that we shouldn't mix concepts unless it truly demonstrates something important.
We can always have a future example that shows how to create nodes with the macro.
|
Done reviewing. Nice work. Left a bunch of minor feedback. |
|
|
||
| --- | ||
|
|
||
| ### Why Does `percy` Override Default Checkedness? |
There was a problem hiding this comment.
So, if you're using
percy-domstrictly for server-side rendering, with no client-side logic, this would be a regression.
I'm having doubts that this was the right direction. Why would anyone ever use percy-dom for server-side rendering? The server shouldn't be manipulating a DOM server-side. That should be actively discouraged, right?
Therefore percy-dom has no reason to be overriding the default checkedness except to match virtual-node's behavior, but why should it? They're doing two different things. One is encoding a virtual node tree as HTML, the other is creating, diffing, and patching DOM nodes based on two different virtual node trees.
At this point, this example seems like a whole lot of faffing because we're doing the wrong thing.
Proposal:
- Let's be explicit that
percy-domis not for server-side rendering. - Let's have
percy-domexplicitly do what makes sense for the context of the browser:checkedmanipulates theHTMLInputElement.checkedproperty - it doesn't mess around with default checkedness as there's no reason to do so. People writing typical/idiomaticpercyapps should never need to worry about it. And if for some reason they do, they shouldn't have to work around this strange behavior.
What we gain from keeping this behavior:
percy-dommaintains closer HTML in the browser as it does to if you were to callvirtual_node.to_string()(I don't see why this is useful, and can't think of a way this would cause issues. Can you?)
What is gained from tossing this behavior:
- Nobody needs to worry about what
percy-domdoes to default checkedness: nothing. percy-domcan avoid dealing with default checkedness entirely, as it's not useful for application-state-driven GUIs (which should always specify checkedness according to app state).percy-domdoesn't have an expectation of being used for something it shouldn't be used for.
There was a problem hiding this comment.
I don't remember where I wrote that quote, so I don't understand what you're referring to.
The server shouldn't be manipulating a DOM server-side.
A DOM does require on the browser in any way. We should not couple these unrelated concepts.
Whether percy can manipulate a DOM should never depend on whether it is running in a browser.
I'll name some reasons:
-
testing. It is an eventual goal that to be able to test the vast majority of your
percyapplication outside of a browser. So, instead of usingweb-sys, we would usegeneric-dom-traitsthat usedweb-sysin a browser and
something else outside of the browser.
This would enable fast testing of things like "what happens when we click on this element but the onclick handler callsevent.prevent_default()".
For instance, we might use in-process-memory DOM implementation that was reliable enough that you trusted the results, and fast enough that one wouldn't hesitate to write as many test cases as they wanted- then a majority of an application's tests could be fast, and a minority that really needed to be in a browser could bear that overhead
-
simulation / benchmarking / etc
- if you want to run your application in an environment that has a DOM, you should be able to. We should not arbitrarily limit your application to only be able to run inside of probably-slow, and probably-cumbersome-to-use web browser
it doesn't mess around with default checkedness as there's no reason to do so
It comes down to the framing. You're framing it as being about "default checkedness".
In that context, yes we currently have no reason to believe that we need percy-dom to do anything to the "default checkedness".
However, if this is framed around "setting the checked attribute" and "the user thinks that they're setting the checked attribute", then that changes things.
So, the question becomes, would a user ever want to set the "checked" attribute?
If <input foo="bar" /> sets the foo attribute, a user might expect <input checked=true /> to set the "checked" attribute.
If we pointed the user at an application and asked "what do you think happens when we render <input checked=true /> they should probably say "The checked attribute gets set".
In practice this would only impact the user if they were doing input.getAttribute('checked') and got back null instead of "true".
It's entirely possible that roughly 0 people will ever do this.
And, if they do, they can open an issue and we can do further design work.
So, I'm okay with, for now, having percy-dom only control the checked property, and not the checked attribute.
Proposal
Let's be explicit that percy-dom is not for server-side rendering.
Don't do this. Instead, we can say that if a user truly wants to set the "checked" attribute they can use .setAttribute on the node themselves https://developer.mozilla.org/en-US/docs/Web/API/Element/setAttribute .
checked manipulates the HTMLInputElement.checked property - it doesn't mess around with
default checkednesschecked attribute (words swapped by me)
Sounds good.
There was a problem hiding this comment.
Sounds good.
Neat.
I'll delete the example as it's no longer illustrative of doing something reasonable. If users want to set the default checkedness of an element, they should just get the DOM element and add/remove the attribute. It can be grabbed from the commit history if its useful later?
It's also worth mentioning that the situation with value is exactly the same as with checked, and having different behavior for them would be strange, at least long-term. If nothing explodes by releasing this change out into the wild, I think changing how percy-dom treats value should be changed to match checked. For now, the book entries reflect the difference.
Let's be explicit that percy-dom is not for server-side rendering.
I will leave it out. It was only in service of motivating the checked VDOM attribute behavior.
However, if this is framed around "setting the checked attribute" and "the user thinks that they're setting the checked attribute", then that changes things.
It's unfortunate.
- The core issue is the HTML standard. The
checkedHTML attribute should be calleddefaultCheckedor something to distinguish it. - Secondly, using a macro called
html!with a HTML-like format to manipulate a DOM. It's misleading because an HTML document and a DOM have aliased but distinct concepts, such as thecheckedattribute and thecheckedproperty. ...It's a small enough issue that the choice might be worth the benefit of the intuitive markup.
Unless we remove the checked attribute from percy (and use something like is_checked and default_checked just to make it harder to assume what they mean) I think we're going to need to bite the bullet that devs will sometimes assume that percy-dom does the wrong thing - with respect to percy's intended utility.
But at least percy-dom will be doing the sensible thing (as far as we can tell, at least), so it's "just" a learning speed bump. At least percy isn't hard to use once the dev (hopefully) reads the book section to figure out what's going on.
Some leftover thoughts:
A DOM does require on the browser in any way. We should not couple these unrelated concepts.
Whether percy can manipulate a DOM should never depend on whether it is running in a browser.
I agree. We're talking past each other though. I don't think this is relevant to what I'm proposing.
Testing, simulation, benchmarking, etc. in environments without a browser seem neat. I just don't see what this has to do with server-side rendering though.
Server-side rendering is: generating a static HTML document and sending it off to the client. Manipulating a DOM is not useful in this process. Manipulating a virtual DOM is useful. But percy already has a distinction between virtual-node+html-macro and percy-dom such that manipulating a virtual DOM server-side to generate HTML has no dependency on the idea of actually manipulating a real or simulated or mocked DOM.
There was a problem hiding this comment.
I'll delete the example
Unless it would be hard, let's just change it to be a embed-non-percy-node example where we show you how to create your own node that percy doesn't touch.
Can just delete all of the checkedness stuff and instead just make the example create a <div hello="world" /> or something using web-sys and insert that into
a percy contain .on_create_element.
If nothing explodes by releasing this change out into the wild, I think changing how percy-dom treats value should be changed to match checked
Sounds good.
Secondly, using a macro called html! with a HTML-like format to manipulate a DOM
html! macro is describing the DOM that you want.
Then percy-dom manipulates it.
The macro isn't manipulating the DOM, just declaring one.
So, it's designed to stay close to how a user would expect to be able to declare a DOM tree. Since HTML is likely the most common way to way to define a DOM tree, it's designed to feel like HTML.
This lets users avoid learning percy and instead just learn common specs/concepts. Transferable knowledge.
Unless we remove the checked attribute from percy (and use something like is_checked and default_checked just to make it harder to assume what they mean)
I think we're going to need to bite the bullet that devs will sometimes assume that percy-dom does the wrong thing - with respect to percy's intended utility.
rough idea
I could see a potential future (would require design work) where the html! macro gave a compile time error if you used checked.
It would tell you to use is_checked or how to set the attribute if you really wanted to do that.
That wouldn't help users that use VirtualNode directly, however.
But at least percy-dom will be doing the sensible thing (as far as we can tell, at least), so it's "just" a learning speed bump
Yeah, especially if the macro compile time error points you to the docs.
I agree. We're talking past each other though
Yeah I only realized that after I continued reading your comment
Server-side rendering is: generating a static HTML document and sending it off to the client. Manipulating a DOM is not useful in this process.
Here's what I had in mind:
-
user starts some sort of simulation application where they are running their percy application in an emulated browser
-
users triggers a bunch of clicks / input events / etc
-
user now wants to take the DOM tree from that emulated browser and turn it into a string
If you're really on default checkedness that DOM tree will be different from your virtual tree. Your virtual tree doesn't know whether the element is checked. Only the emulated DOM has info about whether the input is checked.
I haven't thought enough about this to know if it relates to our convo, so I'm more so communicating that I'm not certain that we should operate as if server-side rendering of a "real DOM" will never be useful.
|
If it's decided that #206 (comment) is a misguided take, then this is ready for a review of the new example and README, as they've been entirely rewritten. |
There was a problem hiding this comment.
Two tiny pieces of feedback.
Then you can address the open conversation about the checked attribute however you want #206 (comment)
Then just tell me to merge and I'll merge.
Thanks for nailing this all down.
|
All done. Removed setting default checkedness, deleted the example. I have not removed the behavior of setting the default value (regarding the Double checked the PR, looks good to merge. Thank you very much for your responsiveness and assistance! |
|
I replied suggesting that we keep the example but tweak it to only show embedding a non-percy node. #206 (comment) If you're up for it an think it's a good idea, let's do that then merge. If you're tired of this PR, or think it's a bad idea, we can merge. You can tell me when to merge. Thanks. |
|
Changing the example, shouldn't take long 👍 |
|
Cool. I'll merge after that without reading it so that we don't go back and forth on what I can only imagine would be trivial feedback at this point. I'll release this as a patch release since the number of people relying on default checked-ness is very likely to be 0. If you have a strong issue with that, let me know. Otherwise, I'll proceed with a patch release. Reason for patch is that I consider this to be a bug and think it better for users' applications to pick up this fix whenever they update their Some risk that there's someone out there that is truly relying on the current behavior. I'm essentially accessing that risk at roughly 0%. Hmm... I started thinking about how to justify my "roughly 0%" claim and I didn't have a good justification. So, let's just do a minor release. If this was impacting an existing users' applications then we would've heard of it by now. So, no good reason to risk breaking someone's application without them intentionally upgrading to the latest I'll publish this as |
|
Don't merge yet. |
Sounds best to me. I've bumped the version in this PR. |
|
Done making changes. Ready to merge 👍 |
|
Published as |
This PR changes the meaning of
checkedas specified in thehtml!macro.Originally, specifying
checkedwould add or remove thecheckedHTML attribute. This changed the default checkedness of the input element, but may or may not actually change the rendered state of the element.Now, specifying
checkeddirectly specifies the rendered state of the element. (The default checkedness is not modified, and can be set manually.)Motivation
This change makes it easier to do the more commonly-desired task of specifying the rendered state of the checkbox (likely according to application state) rather than merely the default checkedness.
This change mitigates shooting ourselves in the foot by assuming
checkedsets a rendered checkbox's checkedness.Further discussion is available in the issue #205 as well as in the new book entry and tests
https://github.com/SFBdragon/percy/tree/checked_sets_checkedness/book/src/html-macro/special-attributes
https://github.com/SFBdragon/percy/blob/checked_sets_checkedness/crates/percy-dom/tests/checked_attribute.rs
This resolves #205 for the time being.