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

Behaviour of value attribute #122

Closed
raxell opened this issue Jul 30, 2019 · 17 comments
Closed

Behaviour of value attribute #122

raxell opened this issue Jul 30, 2019 · 17 comments
Assignees

Comments

@raxell
Copy link
Member

raxell commented Jul 30, 2019

The value attribute has an ad hoc expression since it is not recomended to set it with setAttribute.
But the behaviour of value is different from that of normal attributes, when passing a falsy expression the attribute is not removed. This can be a problem when using it on non-form elements (example).

Is this the expected behaviour?

There are conflicting informations about falsy values, here it is said that falsy values do not remove attributes, but the tests checks for the opposite:
https://github.com/riot/dom-bindings/blob/9697d1fb7429c3a0b494c0ee6b472b470a054434/test/expressions/attribute.spec.js#L33-L45

@GianlucaGuarini
Copy link
Member

@raxell riot/riot#1453 is an issue referring to an old Riot.js version 2015. This repository is brand new created explicitly for Riot.js 4. Unit test behave consistently with the new documentation.

Regarding the value attributes on non form attribute we could probably handle them differently here but it would mean also adding more magic to the source code generating probably more problems than the ones we are supposed to solve

@raxell
Copy link
Member Author

raxell commented Aug 1, 2019

@raxell riot/riot#1453 is an issue referring to an old Riot.js version 2015. This repository is brand new created explicitly for Riot.js 4. Unit test behave consistently with the new documentation.

Oh you're right, I thought the latest comments of april was about v4.

So value is never added to dom for input elements, it is just setted with el.value = . But it is a valid attribute for other standard html elements, so I think is mandatory to handle it. If can be classified as bug I can create an issue in https://github.com/riot/riot
If it's ok for you I'll try to address it in the next days, I still have to see how the compiler work

@GianlucaGuarini
Copy link
Member

I will migrate this issue to the compiler. We would need to patch the dom-nodes first to list all the input nodes.

@GianlucaGuarini GianlucaGuarini transferred this issue from riot/dom-bindings Aug 2, 2019
GianlucaGuarini added a commit to riot/dom-nodes that referenced this issue Aug 2, 2019
@raxell
Copy link
Member Author

raxell commented Aug 2, 2019

More than the inputs elements the ones that must use the value expression binding are those that have defaultValue in their DOM interface. According to the spec those are input, textarea and output.
So any element other than those 3 can use normal attribute expression, do you agree with that?

GianlucaGuarini added a commit that referenced this issue Aug 2, 2019
@raxell
Copy link
Member Author

raxell commented Aug 2, 2019

The patch seems wrong to me, the problem with <progress> remains

@GianlucaGuarini
Copy link
Member

could you please make a PR on the dom-nodes repo? I have now prepared the hooks in the compiler.

@GianlucaGuarini
Copy link
Member

@raxell I have updated the list again. Could you please have a look at it?

@raxell
Copy link
Member Author

raxell commented Aug 2, 2019

The list can be found here, textarea and output does not have value.
But having the attr value does not mean that value expression should be used. I'll send a PR in a few mins so you can see what I mean

@GianlucaGuarini
Copy link
Member

Droppint the support of the value directive on textarea tags is considered a breaking change. It ain't gonna happen

@GianlucaGuarini
Copy link
Member

List of the additional elements having the value attribute by specs:

I honestly I don't get either what's the problem with the <progress> tag since it works out of the box.

I think I am going to list all of them and close this issue that for me is only related to html tags that don't allow the use of the value attribute

@raxell
Copy link
Member Author

raxell commented Aug 2, 2019

Just a sec, if you continue to make changes I'll never end to rebase ahahah
This issue is a matter of using node.value only for elements like input and textarea, for progress can be used node.setAttribute('value', 'x') since it does not have defaultValue

@GianlucaGuarini
Copy link
Member

the point is: why should I handle differently DOM elements that allow the use of the value property?

@raxell
Copy link
Member Author

raxell commented Aug 2, 2019

Cause having the attribute and setting the value with .value are two different things. Why the value expression has been made? To handle setting the value property on inputs/textareas I suppose, since setAttribute overrides defaultValue. But for elements other than those two can be used setAttribute as with any other attribute (e.g classes)

@raxell
Copy link
Member Author

raxell commented Aug 2, 2019

The thing is

<progress max="10" value="{progressValue}"></progress>

Here if the value is binded using a value expression the removal of the attribute can only be made by manipulaiting the dom by hand. If it would be binded using a normal attribute expression passing a falsy value would remove the value attr making progress indeterminate (loading animation).

Example

@GianlucaGuarini
Copy link
Member

thank you for your example. I still don't understand which kind of problem we are trying to solve.
A progress tag has always a value otherwise it does not make sense to use it and it can be removed with an if directive. There is no benefit of adding the value attribute to the DOM node.

@raxell
Copy link
Member Author

raxell commented Aug 3, 2019

A progress tag has always a value

No, "If there is no value attribute, the progress bar is indeterminate; this indicates that an activity is ongoing with no indication of how long it is expected to take". If you look at the example, the first progress bar starts with an indeterminate state (like "still don't know how much time it will take"), while the second is just initialized at 0%, those are two different things.

value attributes are just normal attributes that can be set using setAttribute as any other. The only two exceptions are input and textarea that need to use .value. NB: textarea does not have a value attribute, it has a value property, doing (just HTML):

<textarea value="example text"></textarea>

Doesn't make sense, will not set any text in the textarea.
Setting textarea.value from js will not set anything in the DOM (from the inspector console you will not see any value attribute added), same for input. Instead, setting progress.value from js will change the attribute in the DOM (from the inspector console you will see that value will be added/changed), so this is equivalent to using setAttribute.

To me the problem seems clear, I don't know if I'm wrong with something, is there anyone else that can review this and say if it make sense?
Anyway the final decision is up to you, all started from this stackoverflow question.

@raxell
Copy link
Member Author

raxell commented Aug 3, 2019

Sidenote: it's not necessary to remove value to have progress in an indeterminate state, having <progress value=""> is the same. But when using progress.value = '' the browser cast the empty string to 0 since it want a double

GianlucaGuarini added a commit that referenced this issue Aug 3, 2019
GianlucaGuarini added a commit that referenced this issue Aug 3, 2019
GianlucaGuarini added a commit that referenced this issue Aug 3, 2019
* master:
  fixed: eslint warning
  4.3.6
  updated: changelog
  closes #122
  closes #122
  added: test for #121
  added: other tests
  fix: Chained object access as text expression
  fixes riot/riot#2737
  4.3.5
  updated: fix css unicode properties
  4.3.4
  updated: changelog
  updated: attempt to fix riot/riot#2714
  4.3.3
  added: changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants