-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Make <textarea>
a void element
#3465
base: master
Are you sure you want to change the base?
Conversation
Visit the preview URL for this PR (updated for commit 66e8ad3): https://yew-rs--pr3465-textarea2void-gtm0ku6i.web.app (expires Mon, 30 Oct 2023 20:02:30 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 |
Size Comparison
✅ None of the examples has changed their size significantly. |
Benchmark - SSRYew Master
Pull Request
|
Why not recommend setting |
We wouldn't recommend setting |
The See this playground |
But the paragraph I've added to "html/elements" docs is specifically about the peculiarity of |
Wait, why does |
The special handling predates the |
But then is it really special for |
It's special because the value is always set as property, not attribute, for input |
Does that make a difference in case of |
Setting a
Special handling for |
Which would fail to compile, it's customary to write | ||
|
||
```html | ||
<textarea ~defaultValue="default value"></textarea> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<textarea ~defaultValue="default value"></textarea> | |
<textarea defaultvalue="default value" /> |
I am not in favour of promoting the property syntax for elements. It should only be reserved for cases where it is niche and should not / cannot be handled by the renderer (e.g.: custom elements). html!
should work with built-in elements without users directly touching its properties.
I think we should make a special handing to assign defaultvalue
(html!
attribute) to value
at the first time it is mounted.
I feel that there might be edge cases for ~defaultValue
(property) when it is combined with Suspense as the element might be re-mounted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And what would be the disadvantages to mimicking the actual HTML specification and make <textarea>
accept children?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The childNodes for an HTMLTextAreaElement
indicates its default value and not value.
Once the user starts to type in a textarea, normal means Yew uses to manipulate the DOM (e.g.: HTMLTextAreaElement.insertBefore
) has no effect anymore. Manipulating childNodes does not reflect on the page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we could, for example, not do anything with <textarea>
's children after it's inserted into the DOM, i.e. not reconcile it, which also might improve performance, besides, we already have special handling for <textarea>
, it even has a separate variant for it in the VTagInner
enum. It's just that going against the HTML spec would only unnecessarily complicate the framework from the perspective of the users, I believe that when it comes to HTML elements, we should strive to keep their usage in Yew as close to their actual definition in the standard as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just that going against the HTML spec would only unnecessarily complicate the framework from the perspective of the users
The specification says that HtmlTextAreaElement's default value can be assigned via the defaultValue
property. Since elements in Yew are created via document.createElement
, we are not against the specification.
Specifically not reconciling textarea is only going to make users confused as it is not obvious to novice users that textarea is only defining its default value and value updates will need to be done via value attribute. It was also not obvious to me until after some previous discussion about making textarea to use children prop happened in the past.
This pull request makes it a compile error and suggests ~defaultValue
to users, which is a better user experience IMO.
In addition, React and Vue both do not accept children for textarea. So we are consistent with the behaviour of major frameworks. This will ease the learning curve for users from these frameworks.
React: https://react.dev/reference/react-dom/components/textarea#props
Vue: https://vuejs.org/guide/essentials/forms.html#multiline-text
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious about the use case where the textarea is needed and isn't controlled
packages/yew/src/virtual_dom/vtag.rs
Outdated
.render() | ||
.await; | ||
|
||
assert_eq!(s, r#"<textarea value="teststring"></textarea>"#); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert_eq!(s, r#"<textarea value="teststring"></textarea>"#); | |
assert_eq!(s, r#"<textarea>teststring</textarea>"#); |
Since we are rendering this as HTML, <textarea>
does not accept value attribute.
If both value and default value are set, the value prop should have priority and the defaultvalue prop should have no effect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we provide the value during SSR then? In CSR we manipulate the element directly for that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have submitted a review suggestion below, which should be cover both defaultvalue and value.
The component only gets 1 chance to declare HTML, which should match the initial render of CSR.
When both defaultvalue and value are set in CSR, default value never shows and value is shown instead, as value is set.
SSR should match this behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In practice, it doesn't make sense to set both default value and value as the defaultvalue is never shown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If both are set, we should console.warn and lint in the macro. The lint can be in a future PR but I would like to have the console.warn with this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might not be able to create a lint on the macro level.
One can create a component library and it would make sense for them to passthrough both value and defaultvalue.
There's definitely something wrong with the benchmarking CI action |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should fix the SSR behaviour of textarea.
See above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some remaining style fixes.
Otherwise, LGTM.
Co-authored-by: Kaede Hoshikawa <futursolo@users.noreply.github.com>
Co-authored-by: Kaede Hoshikawa <futursolo@users.noreply.github.com>
are there still any blockers to merging this? |
Description
Fixes #3296 by raising an error when trying to provide children to
<textarea>
and asserting the correct way to provide default value to it in the docs, which is thedefaultvalue
attribute also added with this PR.Documents the peculiarities of
<textarea>
and the void elements in general.Checklist