Fix MDN reference for kind attribute

[zoechi]

Description

Just a code comment fix.

I think this is a copy-paste error and instead of InputElement it should be TextTrack
because that's the only reference for a kind attribute I could find in MDN

Fixes # (issue)

Checklist:

• I have ran ./ci/run_stable_checks.sh
• I have performed a self-review of my own code
• I have added tests that prove my fix is effective or that my feature works

[siku2]

<track> is a child element of <audio> or <video>, it has nothing to do with <input>.

The kind field represents the type attribute. It uses a different name because type is a keyword in Rust.
We could use this PR to instead document this better because it isn't very clear at all.
Everywhere "kind" is used we should probably mention that it's an alias for "type".

Here's the relevant code that uses the kind field to set the input's type attribute:

yew/yew/src/virtual_dom/vtag.rs

Lines 296 to 311 in cdded9a

	if let Some(change) = self.diff_kind(ancestor) {
	let kind = match change {
	Patch::Add(kind, _) | Patch::Replace(kind, _) => kind,
	Patch::Remove(_) => "",
	};
	cfg_match! {
	feature = "std_web" => ({
	//https://github.com/koute/stdweb/commit/3b85c941db00b8e3c942624afd50c5929085fb08
	//input.set_kind(&kind);
	let input = &input;
	js! { @(no_return)
	@{input}.type = @{kind};
	}
	}),
	feature = "web_sys" => input.set_type(kind),
	}

I think "kind" is used because of stdweb, but maybe it would make sense to add a set_type method which aliases set_kind, @jstarry?

[zoechi]

@siku2 just found out while looking through tests
https://github.com/yewstack/yew/blob/master/yew/src/virtual_dom/vtag.rs#L884
I'll update the PR

[zoechi]

This is mentioned 2-3 times

    /// ... We use workarounds for:
    /// `type/kind`, `value` and `checked`.

I searched a while but wasn't able to figure out what exactly these workarounds are.
Any idea how or where to find out?

[siku2]

@zoechi, I think that could be made a bit clearer too...
If you look at the fields of the VTag struct you can see that it contains fields for kind, value, and checked:

yew/yew/src/virtual_dom/vtag.rs

Lines 40 to 69 in cdded9a

	pub struct VTag {
	/// A tag of the element.
	tag: Cow<'static, str>,
	/// A reference to the `Element`.
	pub reference: Option<Element>,
	/// List of attached listeners.
	pub listeners: Listeners,
	/// List of attributes.
	pub attributes: Attributes,
	/// List of children nodes
	pub children: VList,
	/// Contains a value of an
	/// [InputElement](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input).
	pub value: Option<String>,
	/// Contains
	/// [kind](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input#Form_%3Cinput%3E_types)
	/// value of an `InputElement`.
	pub kind: Option<String>,
	/// Represents `checked` attribute of
	/// [input](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input#attr-checked).
	/// It exists to override standard behavior of `checked` attribute, because
	/// in original HTML it sets `defaultChecked` value of `InputElement`, but for reactive
	/// frameworks it's more useful to control `checked` value of an `InputElement`.
	pub checked: bool,
	/// A node reference used for DOM access in Component lifecycle methods
	pub node_ref: NodeRef,
	/// Keeps handler for attached listeners to have an opportunity to drop them later.
	captured: Vec<EventListener>,
	pub key: Option<String>,

These attributes require special handling so they're stored outside of the attributes field where normal attributes go. They also have special setter methods like set_value and set_kind.
Right now there's nothing stopping you from using add_attribute("value", "something") instead of set_value("something") though, which is what the comment is trying to make you aware of.

[zoechi]

I was searching for an explanation why these properties require special handling
that led to the decision to add special treatment.
I think some hints about "why" would be helpful in these doc comments.

[siku2]

Oh right, sorry. I misunderstood.

The documentation for <input>'s value property mentions:

When specified in the HTML, this is the initial value, and from then on it can be altered or retrieved at any time using JavaScript to access the respective HTMLInputElement object's value property.

There is a difference between the HTML attribute value and the corresponding JS property value. The attribute doesn't change when the actual value of the <input> changes, only the property does.
Likewise, in order to change the current value of the <input> element we have to set the property. The attribute only controls the initial value.

As for checked, it already has a comment explaining it:

yew/yew/src/virtual_dom/vtag.rs

Lines 60 to 62 in cdded9a

	/// It exists to override standard behavior of `checked` attribute, because
	/// in original HTML it sets `defaultChecked` value of `InputElement`, but for reactive
	/// frameworks it's more useful to control `checked` value of an `InputElement`.

It's basically the same reason as for value.

I don't actually know why type needs special treatment... As far as I know type is a reflected property (i.e. it mirrors the attribute) so it doesn't need to be changed through the property.

[teymour-aldridge]

Everywhere "kind" is used we should probably mention that it's an alias for "type".

Maybe this should go in the module/crate documentation (just once, rather than many times).

[zoechi]

@siku2 I'm not able to make progress here, but I think that it would be great to have this properly solved and documented and if there is special behavior that also properly tested.

In vtag.rs it is mentioned twice

        // ... For example I interpret `checked`
        // attribute as `checked` parameter, not `defaultChecked` as browsers do

How could I (referring to "I interpret" from the quote above) deviate from the default browser behavior when all virtual_dom does is to pass HTML which is then interpreted by the browser.

There is a difference between the HTML attribute value and the corresponding JS property value. The attribute doesn't change when the actual value of the changes, only the property does.
Likewise, in order to change the current value of the element we have to set the property. The attribute only controls the initial value.

Perhaps I'm missing something here but all virtual_dom does is generating HTML which only deals with attributes (not properties).
At least I was not able to find anything in the source that refers to DOM object properties.

So all special behavior I can find here is that checked is a boolean attribute
(but not listed in tag_attributes.rs BOOLEAN_SET)

What I can imagine this all referring to is virtual_dom reading from the DOM when diffing, but I wasn't yet able to find out if and where this could happen.

[siku2]

Perhaps I'm missing something here but all virtual_dom does is generating HTML which only deals with attributes (not properties).

Yew's virtual_dom doesn't generate any html. Yew manipulates the document purely using js DOM calls (document.createElement and such).
Attributes, for instance, are added to an element here:

yew/yew/src/virtual_dom/vtag.rs

Lines 252 to 269 in cdded9a

	let changes = self.diff_attributes(ancestor);
	// apply attribute patches including an optional "class"-attribute patch
	for change in changes {
	match change {
	Patch::Add(key, value) | Patch::Replace(key, value) => {
	element
	.set_attribute(&key, &value)
	.expect("invalid attribute key");
	}
	Patch::Remove(key) => {
	cfg_match! {
	feature = "std_web" => element.remove_attribute(&key),
	feature = "web_sys" => element.remove_attribute(&key).expect("could not remove attribute"),
	};
	}
	}
	}

Notice how it uses the following bit of code to set an attribute:

element
  .set_attribute(&key, &value)
  .expect("invalid attribute key");

"element" here refers to the actual HtmlElement from the DOM and set_attribute calls setAttribute on said element.

Everything in the virtual_dom is done like this.

When looking at the code watch out for the following:

• "diff" functions are used by Yew to find out what/whether something has changed.
• "apply" functions are the ones that then apply these changes to the page. You'll notice that these make calls to the diff functions.
• The VDiff trait is the core of this all. Look at how VTag implements VDiff

[zoechi]

@siku2 thanks a lot. I now understand much better what's going on.

To me it now looks like (web_sys only)

If it's a

• button
  • HtmlButtonElement.set_type() is used for kind
• input
  • InputElement.set_type() is used for kind
  • InputElement.set_value() is used for value
  • InputElement.set_checked() is used for checked
• textarea
  • TextAreaElement.set_value is used for value

So the actual "workarounds" are mostly about how these methods in web_sys differ from set_attribute.

For a consumer of Yew this is only relevant when the imperative VTag:::new(...). is used, because with html!(...) the macros take care of of parsing the properties and assigning it to the appropriate struct fields.

Would it make sense to add assertions in add_attribute for these being set there instead of by set_value, set_kind, and set_checked?

[teymour-aldridge]

When you say "assertions" do you mean using the assert!/assert_eq! macro?

[zoechi]

When you say "assertions" do you mean using the assert! macro?

Actually, I don't know yet what ways Rust offers for that.

What I meant is something that warns or panics perhaps only in development to not pay the performance cost in production.

So according to https://doc.rust-lang.org/std/macro.assert.html probably debug_assert!

[teymour-aldridge]

My feeling is that we could leverage the type system to achieve this. This means you get compile-time errors instead of runtime errors.

[siku2]

If it's a

* button
  
  * `HtmlButtonElement.set_type()` is used for `kind`

* input
  
  * `InputElement.set_type()` is used for `kind`
  * `InputElement.set_value()` is used for `value`
  * `InputElement.set_checked()` is used for `checked`

* textarea
  
  * `TextAreaElement.set_value` is used for `value`

So the actual "workarounds" are mostly about how these methods in web_sys differ from set_attribute.

That's correct, yes. As a side note, these methods element.set_type(<value>) are just Rust representation of the property setters' element.type = <value> but I think that was already clear.

---

Would it make sense to add assertions in add_attribute for these being set there instead of by set_value, set_kind, and set_checked?

I'm definitely in favour of something like that. @teymour-aldridge, I don't think we should use assert! (or anything else that panics) for this though.
I think we should check the key against these special attributes and delegate the call to the appropriate setter instead. So that set_attribute("vallue", "a") calls set_value("a").

[teymour-aldridge]

@siku2 I agree that we don't want anything panicing here!

[teymour-aldridge]

I think we could have an enum containing attributes.

enum Attribute {
  SomethingValid(<value>),
 <... so on so forth>
  CustomAttribute(String, String)
}

[teymour-aldridge]

Something like that.

[siku2]

Also I think it's important to add that there should be a hidden implementation that doesn't do any of this for the macro. We don't need the performance penalty there since we can already do the sorting at compile time.

[siku2]

@teymour-aldridge the user could still do CustomAttribute("value", "Hello World!") with this approach though. I guess it would be harder to make that mistake accidentally, but what if someone tried to do it dynamically?

[zoechi]

If it can properly be solved using the type system, I'm all for it.
If it's a developer error I think panic should be fine. This way the developer can get the proper information about what he did wrong and how to fix it.
If we want to make it possible to use set_attribute anyway even though it's not advicable, then a panic is of course not the way to go.
With automatic sorting the user pays the performance penalty without getting the information how to improve.

[siku2]

@zoechi I think the performance penalty of something like this is negligible:

pub fn add_attribute<T: ToString>(&mut self, name: &str, value: &T) {
  match name {
      "value" => self.set_value(value),
      "kind" => self.set_kind(value),
      _ => self.attributes.insert(name.to_owned(), value.to_string()),
  }
}

I'm not absolutely certain about this but I think Rust is able to remove the match entirely for the very common case where name is a literal.
Keep in mind that the macro would use a hidden method like __unchecked_add_attribute to do this without any performance hit whatsoever.

I'm all for using Rust's type system but I don't see how it could be used here without making the API awkward to use.

[zoechi]

@siku2 I think you're right. Looks simple enough, even with an additional check if it's an InputElement if this is desirable.

[zoechi]

I was wondering about

As a side note, these methods element.set_type() are just Rust representation of the property setters' element.type =

Are there any plans to add add_property in addition to add_attribute?
So, instead of workarounds to have attribute values be passed to properties, make a direct connection and let the developer decide if he wants to set an attribute or a property.
The set_checked, set_kind, set_value are such things already, but they are a very limited set and it's not too obvious.

[siku2]

@zoechi That's actually a really good idea.
In js-sys this is possible using Reflect::get(&target, &property_key) and Reflect::set(&target, &property_key, &value). Not sure if stdweb provides similar functionality but it would certainly be possible using inline js.

What would be a good use case for this though? Yew already has other means of accessing elements (using NodeRef for example).
It might be possible to use this approach to clean up the internal code though.

[zoechi]

What would be a good use case for this though?

Most properties reflect to attributes. There are some exceptions.

The main difference I'm aware of is that properties take/provide values in proper types (string, number, bool, object) while attributes only support string.

I'll sleep it over and hope to be wiser tomorrow about the consequences/advantages this could have for Yew.

[jstarry]

I think this conversation is great, I would love if we had more clarity on how Yew handles attributes vs properties. @zoechi do you mind writing up a new issue for this?

As for this PR, I'm in favour of removing the "kind" alias for input "type". It's confusing and non-standard.

[zoechi]

@jstarry Sure, I'll summarize in a new issue.
We can use set_type for the setter but the struct needs some different name because ’type` is a keyword.

[zoechi]

I'm going to close this PR and create an issue instead.

[teymour-aldridge]

Cool.