Input numbers leave unprocessed expressions after upgrade to 2.6.0 #1957

Closed
cbxp opened this Issue Aug 23, 2016 · 13 comments

Projects

None yet

3 participants

@cbxp
cbxp commented Aug 23, 2016

Chrome masks this bug because it doesn't allow non-numbers in input[type=number] fields, but you can clearly see in the Inspector that values contain unprocessed expressions for falsy values (even zero).

Open in IE to fully see the extent of the problem.

http://plnkr.co/edit/Ic2hW5DXtgVD71pVTyR7?p=preview

  • Question
  • Bug
  • Discussion
  • Feature request
  • Tip
  • Enhancement
  • Performance
@cbxp
cbxp commented Aug 23, 2016

This is critical for our app, so we have to downgrade to 2.5.0 :-(

@GianlucaGuarini GianlucaGuarini added the bug label Aug 23, 2016
@rsbondi
Contributor
rsbondi commented Aug 23, 2016

This works if you put in riot-value={expr}, the compiler did this for type=number at one point, I can't seem to find where it was removed. @aMarCruz ?

@cbxp
cbxp commented Aug 23, 2016

It was not removed according to the change log since 2.5.0, most likely
some refactoring has broken the old behaviour

On Tue, 23 Aug 2016, 19:45 Richard Bondi, notifications@github.com wrote:

This works if you put in riot-value={expr}, the compiler did this for
type=number at one point, I can't seem to find where it was removed.
@aMarCruz https://github.com/aMarCruz ?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#1957 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABmoKZZkAsxo1jlqwBBNFK9FFxJOuyb_ks5qiyOMgaJpZM4JqzCq
.

@GianlucaGuarini
Member

I think this bug probably was introduced with 38ed5fb

@rsbondi
Contributor
rsbondi commented Aug 24, 2016

@GianlucaGuarini you are correct, that is the change.

We originally did this in the compiler here and updating to replace type="number" to riot-type="number" in the compiler should allow this change to work and preserve cursor as intended. I am sure there is a cleaner way than the original to do this.

A type number input can only accept numeric values so the string expression value="{ value }" is invalid at parse. Currently it evaluates to type="{'number'}" but it seems the type expression is evaluated before the value expression

@GianlucaGuarini GianlucaGuarini added a commit that referenced this issue Aug 24, 2016
@GianlucaGuarini GianlucaGuarini closes #1957 3fae681
@cbxp
cbxp commented Aug 25, 2016

Unfortunately, the issue still persist with riot 2.6.1, even the original plunkr example still shows {value} inside of the input in IE...

@GianlucaGuarini
Member

Use 0 instead of null, it does not make any sense to set null value to an input type number

@cbxp
cbxp commented Aug 25, 2016

Null is the same as undefined - no value.
Works for other inputs, why should it leave the expression inside of the
field? Leaving unprocessed expressions anywhere seems very wrong...

On Thu, 25 Aug 2016, 11:10 Gianluca Guarini, notifications@github.com
wrote:

Use 0 instead of null, it does not make any sense to set null value to an
input type number


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#1957 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABmoKQXJKeP2_AE6EC6B8FV2QKFjTqzEks5qjU4RgaJpZM4JqzCq
.

@GianlucaGuarini
Member

@cbxp I am afraid that clearing the input values in case of expressions returning null or undefined could be a breaking change. I will fix this behavior in riot@3.0.0. For now just use 0 for input type number with no values. In any case this is a IE bug and not really related to riot

@cbxp
cbxp commented Aug 29, 2016

But shouldn't riot always clear its expressions from the output? Expression
returning null should result in empty string being rendered, shouldn't it?

On Sat, Aug 27, 2016 at 4:26 PM Gianluca Guarini notifications@github.com
wrote:

@cbxp https://github.com/cbxp I am afraid that clearing the input
values in case of expressions returning null or undefined could be a
breaking change. I will fix this behavior in riot@3.0.0. For now just use
0 for input type number with no values. In any case this is a IE bug and
not really related to riot


You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#1957 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABmoKU1FfnkSMOgr_k6cSiD2LHIkKrQuks5qkDr8gaJpZM4JqzCq
.

@cbxp
cbxp commented Sep 14, 2016

Just a reminder: the original plnkr test case still fails in IE.

I have debugged IE and found where the problem is:
For input type=number, HTMLInputElement.value returns an empty string if it actually contains the riot expression. HTMLInputElement.getAttribute('value') returns the real text there.

The error is on line 108 of update.js.

The point is, even if value is null, riot should still set it for the first time. Currently it skips it altogether, which is a bug.

@GianlucaGuarini GianlucaGuarini removed the fixed label Sep 14, 2016
@GianlucaGuarini
Member

@cbxp for now just use riot-value instead http://plnkr.co/edit/g2F7cUz3iK3it37Lg8hx?p=preview this seems to be really another stupid ie issue

@GianlucaGuarini GianlucaGuarini added this to the 3.0.0 milestone Sep 14, 2016
@cbxp
cbxp commented Sep 15, 2016

Yes, riot-value works as a workaround, but it would be better to have a
fix...

On Wed, 14 Sep 2016, 22:14 Gianluca Guarini, notifications@github.com
wrote:

@cbxp https://github.com/cbxp for now just use riot-value instead
http://plnkr.co/edit/g2F7cUz3iK3it37Lg8hx?p=preview this seems to be
really another stupid ie issue


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1957 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABmoKS3iIosO454rivkFGLn3HRUlN7L0ks5qqEd0gaJpZM4JqzCq
.

@GianlucaGuarini GianlucaGuarini added a commit that closed this issue Nov 22, 2016
@GianlucaGuarini GianlucaGuarini closes #1957 17be353
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment