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

Make <textarea> a void element #3465

Merged
merged 20 commits into from
Feb 20, 2025

Conversation

its-the-shrimp
Copy link
Contributor

@its-the-shrimp its-the-shrimp commented Oct 13, 2023

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 the defaultvalue attribute also added with this PR.
Documents the peculiarities of <textarea> and the void elements in general.

Checklist

  • I have reviewed my own code
  • I have added tests

@github-actions
Copy link

github-actions bot commented Oct 13, 2023

Visit the preview URL for this PR (updated for commit 99f56b2):

https://yew-rs--pr3465-textarea2void-z2ndy58e.web.app

(expires Sun, 23 Feb 2025 19:41:53 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@github-actions
Copy link

github-actions bot commented Oct 13, 2023

Size Comparison

examples master (KB) pull request (KB) diff (KB) diff (%)

✅ None of the examples has changed their size significantly.

@github-actions
Copy link

github-actions bot commented Oct 13, 2023

Benchmark - SSR

Yew Master

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 290.826 291.034 290.913 0.065
Hello World 10 471.984 488.676 476.680 6.105
Function Router 10 1569.542 1593.987 1580.913 7.930
Concurrent Task 10 1005.269 1007.316 1006.407 0.583
Many Providers 10 1031.671 1069.627 1047.253 14.496

Pull Request

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 290.989 307.824 293.588 5.522
Hello World 10 488.552 524.525 502.927 12.968
Function Router 10 1618.343 1642.243 1627.360 7.310
Concurrent Task 10 1005.565 1006.883 1006.383 0.413
Many Providers 10 1105.536 1150.234 1126.245 17.179

@ranile
Copy link
Member

ranile commented Oct 13, 2023

Why not recommend setting value? ~defaultValue works because defaultValue is a property on HTMLTextareaElement. Using value is how you should be interacting with the text area anyway. There's no point of having a textarea that you can't read from (technically, you can, with node refs but that is not the best way of doing things)

@its-the-shrimp
Copy link
Contributor Author

We wouldn't recommend setting value because the MDN docs for <textarea> state explicitly that it doesn't work, and because children of <textarea> define not its value, but its default, initial value, which is exactly what setting ~defaultValue does as well.

@ranile
Copy link
Member

ranile commented Oct 13, 2023

The value attribute doesn't work on textarea but Yew has special handling for value for both input and textarea that makes it work (it makes value and ~value behave the same). If you want to listen to oninput on the text area and react to user input, you need to use the value property.

See this playground

@its-the-shrimp
Copy link
Contributor Author

But the paragraph I've added to "html/elements" docs is specifically about the peculiarity of <textarea>: normally it accepts children, which define its defaultValue, but in Yew it's a void element and instead of children, ~defaultValue is provided, though value should indeed also be documented, I'll add that right now.

@its-the-shrimp
Copy link
Contributor Author

Wait, why does <input>'s value need special handling if it already defines it?

@ranile
Copy link
Member

ranile commented Oct 13, 2023

Wait, why does <input>'s value need special handling if it already defines it?

The special handling predates the ~property syntax and controlled elements need it to be set as a property

@its-the-shrimp
Copy link
Contributor Author

But then is it really special for <input>?

@ranile
Copy link
Member

ranile commented Oct 13, 2023

It's special because the value is always set as property, not attribute, for input

@its-the-shrimp
Copy link
Contributor Author

its-the-shrimp commented Oct 13, 2023

Does that make a difference in case of <input>? I'm just wondering if it's worth it to explain all that in the docs or just pretend that <input> is a perfectly normal element

@futursolo
Copy link
Member

futursolo commented Oct 14, 2023

Why not recommend setting value?

Setting a value makes an element controlled. An element with defaultvalue should remain uncontrolled.

But then is it really special for <input>?

Special handling for value and checked are required as these properties are controlled. This means that values are restored to the one provided to <input /> on every render regardless of whether a value is changed. The value displayed in an input is always in sync with the state associated with the value.
For other attributes, it only updates when an attribute is changed, added or removed.

Which would fail to compile, it's customary to write

```html
<textarea ~defaultValue="default value"></textarea>
Copy link
Member

@futursolo futursolo Oct 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<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.

Copy link
Contributor Author

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?

Copy link
Member

@futursolo futursolo Oct 15, 2023

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.

Copy link
Contributor Author

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.

Copy link
Member

@futursolo futursolo Oct 15, 2023

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

Copy link
Member

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

.render()
.await;

assert_eq!(s, r#"<textarea value="teststring"></textarea>"#);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

Copy link
Contributor Author

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

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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

Copy link
Member

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.

@its-the-shrimp
Copy link
Contributor Author

There's definitely something wrong with the benchmarking CI action

Copy link
Member

@futursolo futursolo left a 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.

Copy link
Member

@futursolo futursolo left a 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.

@its-the-shrimp
Copy link
Contributor Author

are there still any blockers to merging this?

Copy link

github-actions bot commented Feb 16, 2025

Benchmark - core

Yew Master

vnode           fastest       │ slowest       │ median        │ mean          │ samples │ iters
╰─ vnode_clone  2.401 ns      │ 2.587 ns      │ 2.407 ns      │ 2.415 ns      │ 100     │ 1000000000

Pull Request

vnode           fastest       │ slowest       │ median        │ mean          │ samples │ iters
╰─ vnode_clone  2.483 ns      │ 2.55 ns       │ 2.486 ns      │ 2.489 ns      │ 100     │ 1000000000

Copy link
Member

@WorldSEnder WorldSEnder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

o/ Sorry that it took so long to merge this. Looks good to me, really like the error hint and docs.

In case anyone is curious later, this is special in yew basically because of type-safety. Arbitrary children of a <textarea /> are parsed as a string in html, but providing it as an attribute which is already stringly-typed doesn't require us to have different children types and parsing rules for different elements (which would also need to be namespace aware).

@WorldSEnder WorldSEnder merged commit 0091679 into yewstack:master Feb 20, 2025
26 checks passed
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.

Children of text areas do not get rendered
4 participants