refactor!: use inputmode instead of number type #3071

web-padawan · 2021-11-17T13:24:06Z

Description

Updated to use <input type="text"> but not extend vaadin-text-field for now (IMO, that should be done separately, because it would also mean having to bring back pattern and preventInvalidInput properties and tests for them).
Implemented validation logic based on AbstractNumberField because we no longer use <input type="number"> and can't rely on its built-in logic. Note, the Flow counterpart will still override client-side validation.

Fixes #1169
Fixes #1224

Type of change

Feature

Checklist

I have read the contribution guide: https://vaadin.com/docs-beta/latest/guide/contributing/overview/
I have added a description following the guideline.
The issue is created in the corresponding repository and I have referenced it.
I have added tests to ensure my change is effective and works as intended.
New and existing tests are passing locally with my change.
I have performed self-review and corrected misspellings.

tomivirkki · 2021-11-18T14:46:45Z

packages/number-field/src/vaadin-number-field.js

@@ -441,6 +460,16 @@ export class NumberField extends InputFieldMixin(SlotStylesMixin(ThemableMixin(E
    super._valueChanged(this.value, oldVal);
  }

+  /** @private */
+  __isValidByStep(value) {


Something fishy about the validation. With the following:
<vaadin-number-field label="Required" required step="0.3"></vaadin-number-field>
if I focus the field and hit up arrow 3 times so that the value is 0.9, the field becomes invalid when blurred.

Thanks, I will add tests for fractional steps.

@tomivirkki Fixed, and added validate() calls to existing tests for problematic values.

The following number field still claims to be invalid if I change the value with arrow keys:
<vaadin-number-field min="1" step="0.3"></vaadin-number-field>

tomivirkki · 2021-11-18T15:05:00Z

Since BigDecimalField sets the decimal separator (for the <vaadin-big-decimal-field>) based on the locale, we should eventually have the same logic in NumberField also. That would require an API for explicitly setting the decimal separator to the <vaadin-number-field> as well. Not sure if it's better to do it as part of this PR or in a separate feat PR.

web-padawan · 2021-11-22T11:59:59Z

That would require an API for explicitly setting the decimal separator to the as well.

Created an issue to handle it separately for the sake of cleaner release notes: #3096

web-padawan · 2021-11-22T14:49:25Z

We might want to change isAndroid and isIPhone helpers to make them functions to make using stubs possible.
However, if we decide to do that, let's refactor corresponding code separately as it will affect several packages.

tomivirkki · 2021-11-23T09:27:19Z

packages/number-field/src/vaadin-number-field.js

+
+/**
+ * Handle problematic values when calculating a remainder.
+ * Source: Source: https://stackoverflow.com/a/31711034


Suggested change

* Source: Source: https://stackoverflow.com/a/31711034

* Source: https://stackoverflow.com/a/31711034

tomivirkki · 2021-11-23T09:53:36Z

packages/number-field/src/vaadin-number-field.js

    ) {
-      return this.inputElement.checkValidity();
+      if (this.value == null || this.value == '') {


The input value should be sanitized so that the field wouldn't get a non-numeric value and the validation works as expected:

With the following
<vaadin-number-field step="0.5"></vaadin-number-field>

typed value field value invalid

"0.5" "0.5" false

"0,5" "0,5" true

".5" ".5" false

",5" "" false

"1-" "1-" true

It looks like there are some assumptions about localization in this table. For me 0,5 is certainly a valid value. Same for ,5, which results in an input value of 0.5, when comparing the behaviour with a native number input in Firefox.

Clarification: The table represent how the current revision of the PR works

Thanks, I will update the validation logic to cover values with , separator.

tomivirkki · 2021-11-23T10:00:58Z

packages/number-field/src/vaadin-number-field.js

-        type: Number,
-        value: 1,
-        observer: '_stepChanged'
+        type: Number
      }
    };


Let's use _enabledCharPattern in number field also to prevent user from typing arbitrary characters.

tomivirkki · 2021-11-23T10:18:15Z

packages/number-field/src/vaadin-number-field.js

    ) {
-      return this.inputElement.checkValidity();
+      if (this.value == null || this.value == '') {


Seems I can programmatically set a non-numeric value for the field as long as the value starts with a number:

sissbruecker

I'm not very enthusiastic about the keyboard layout detection feature. It looks brittle, and neither of the linked tickets are about implementing such a feature.

Also the main issue of the first ticket (#1169) is that preventing invalid inputs should work consistently between browsers, which isn't addressed at all here.

packages/number-field/src/vaadin-number-field.js

sissbruecker · 2021-11-23T17:08:57Z

packages/number-field/src/vaadin-number-field.js

+        return super.checkValidity();
+      } else {
+        // Mimic validation logic provided by native `<input type="number">` as we don't use it.
+        return !(this.value > this.max || this.value < this.min || !this.__isValidByStep(this.value));


This does not cover the native input validation logic regarding localization. Setting a min of 1.5 and entering a value of 1,3, which is an internal value of 1.3, does not mark the input invalid.

Also value is still a string, while min and max are numbers, which indicates an implicit type coersion in the comparison. Value should be parsed to a number instead, which of course requires us to take the users locale into account.

I also could not find any tests regarding this validation. I assume the previous version relied on the fact that the browser implemented this check. Now that we are doing this on our own we would need tests to cover it.

Thank you for very valid points regarding the implementation. I will update the PR accordingly tomorrow.

sonarcloud · 2021-11-23T17:54:46Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.7% Duplication

web-padawan · 2021-11-24T13:46:22Z

Short summary from a meeting where we discussed the ticket that this PR is targeting:

We agreed that this PR already took more time than originally planned and turned out to be complex. We knew that there are unknowns when taking it into progress.
For now we close this PR as the research part is done. We keep the original ticket open but remove a11y label from it, as it’s not related to any TetraLogical issues and that’s where our focus should be.
We use the new issue [number-field] Consider using input type="text" for number and integer fields #3102 as an epic to discuss all the aspects of switching number field to use input type="text".
This change will not land in Vaadin 23 but we consider it an important improvement worth handling in Vaadin 24+, but it has to be planned accordingly before we can start working on it.

web-padawan force-pushed the refactor/inputmode branch from 5e43116 to 30bef49 Compare November 18, 2021 11:34

web-padawan marked this pull request as ready for review November 18, 2021 11:35

web-padawan requested review from vursen and tomivirkki November 18, 2021 11:35

tomivirkki reviewed Nov 18, 2021

View reviewed changes

web-padawan force-pushed the refactor/inputmode branch from e8d4a47 to e67a53a Compare November 22, 2021 11:29

web-padawan mentioned this pull request Nov 22, 2021

[number-field] Add internal API to provide decimal separator #3096

Open

web-padawan requested a review from tomivirkki November 22, 2021 12:17

web-padawan force-pushed the refactor/inputmode branch from 6e07b9d to 3b83493 Compare November 22, 2021 14:13

tomivirkki reviewed Nov 23, 2021

View reviewed changes

sissbruecker reviewed Nov 23, 2021

View reviewed changes

packages/number-field/src/vaadin-number-field.js Outdated Show resolved Hide resolved

packages/number-field/src/vaadin-number-field.js Outdated Show resolved Hide resolved

sissbruecker reviewed Nov 23, 2021

View reviewed changes

web-padawan mentioned this pull request Nov 23, 2021

[number-field] Consider detecting inputmode based on device (iPhone, Android) #3100

Open

web-padawan added 10 commits November 23, 2021 19:42

refactor: use inputmode instead of number type

6c2ae44

fix: update integer-field to make it work

53ae1d5

test: update snapshots

8aabbed

test: update number-field tests

9108771

chore: update dev page to add RTL example

8462d19

refactor: remove obsolete SlotStylesMixin

e23f4e7

fix: handle step for problematic values

b5a196d

style: remove useless return statement

34bde4c

test: remove no longer user mixin from typings

0a1d7a4

test: add unit tests for wheel listener

53f554d

web-padawan force-pushed the refactor/inputmode branch from 3b83493 to 682f4cc Compare November 23, 2021 17:45

refactor: remove inputmode device detection

5b5fdde

web-padawan force-pushed the refactor/inputmode branch from 682f4cc to 5b5fdde Compare November 23, 2021 17:50

web-padawan closed this Nov 24, 2021

vaadin-miki mentioned this pull request Apr 21, 2023

Allow setting "inputmode" attribute on text and number fields vaadin-miki/super-fields#471

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor!: use inputmode instead of number type #3071

refactor!: use inputmode instead of number type #3071

web-padawan commented Nov 17, 2021 •

edited

Loading

tomivirkki Nov 18, 2021 •

edited

Loading

web-padawan Nov 18, 2021

web-padawan Nov 22, 2021

tomivirkki Nov 22, 2021

tomivirkki commented Nov 18, 2021

web-padawan commented Nov 22, 2021

web-padawan commented Nov 22, 2021

tomivirkki Nov 23, 2021

tomivirkki Nov 23, 2021

sissbruecker Nov 23, 2021

tomivirkki Nov 23, 2021 •

edited

Loading

web-padawan Nov 23, 2021

tomivirkki Nov 23, 2021

tomivirkki Nov 23, 2021

sissbruecker left a comment

sissbruecker Nov 23, 2021

web-padawan Nov 23, 2021

sonarcloud bot commented Nov 23, 2021

web-padawan commented Nov 24, 2021

	* Source: Source: https://stackoverflow.com/a/31711034
	* Source: https://stackoverflow.com/a/31711034

typed value	field value	invalid
"0.5"	"0.5"	false
"0,5"	"0,5"	true
".5"	".5"	false
",5"	""	false
"1-"	"1-"	true

refactor!: use inputmode instead of number type #3071

refactor!: use inputmode instead of number type #3071

Conversation

web-padawan commented Nov 17, 2021 • edited Loading

Description

Type of change

Checklist

tomivirkki Nov 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomivirkki commented Nov 18, 2021

web-padawan commented Nov 22, 2021

web-padawan commented Nov 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomivirkki Nov 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sissbruecker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Nov 23, 2021

web-padawan commented Nov 24, 2021

web-padawan commented Nov 17, 2021 •

edited

Loading

tomivirkki Nov 18, 2021 •

edited

Loading

tomivirkki Nov 23, 2021 •

edited

Loading