PolymerElements / iron-input

An input with data binding
https://webcomponents.org/element/PolymerElements/iron-input
33 stars 45 forks source link

Misuse of "pattern" attribute #60

Closed MeTaNoV closed 8 years ago

MeTaNoV commented 8 years ago

<iron-input> inherits from <input> a pattern attributes that is supposed to be used to validate the (whole) value of the input, but in <iron-input>, it is used like allowedPattern to test the validity of single character entered or composing the value, like found here:

    get _patternRegExp() {
      var pattern;
      if (this.allowedPattern) {
        pattern = new RegExp(this.allowedPattern);
      } else if (this.pattern) {
        pattern = new RegExp(this.pattern);
      } else {
        switch (this.type) {
          case 'number':
            pattern = /[0-9.,e-]/;
            break;
        }
      }
      return pattern;
    },

One cannot see the effect in the test case, since the character also passed the more general pattern... As a consequence, the pattern is not used to validate the value as a whole which should be done in the validate function. I would also recommend to rename allowedPattern to allowedCharacters to avoid the confusion.

MeTaNoV commented 8 years ago

Here is a JSBIN that shows the issue.

MeTaNoV commented 8 years ago

@notwaldorf @morethanreal , kindly PTAL ! :smile_cat:

notwaldorf commented 8 years ago

I think there's a bit of misunderstanding with regards to what "validation" or "validity" means that I'd like to clear up, to make sure we're talking about the right problem. Validation is what happens when you call validate() on an iron-input (which essentially calls validity.valid on the input), and it has nothing to do with prevent-invalid-input (validation still works fine, see: which it does, see https://github.com/PolymerElements/iron-input/blob/master/iron-input.html#L238)

I do agree that we shouldn't be using pattern in the prevent-invalid-input bit, since that's causing confusion, so your PR makes sense. We can't rename the property (because it would break anyone that's currently using it), but would you mind adding a giant explanation comment to the property, explaining this? Perhaps that allowedPattern isn't a regex, it's a set of characters that are allowed, and things like {a} won't work?

Thanks!

MeTaNoV commented 8 years ago

Validation is indeed working fine, my bad is the Issue description. The PR and test case are only reflecting the core of the issue regarding pattern misuse though. I modified it according to your feedback! Thanks again for the review!