From 21aa003b73f588f1e56863b77d69cbc1c25b8ddf Mon Sep 17 00:00:00 2001 From: Martin Staffa Date: Sat, 6 Aug 2016 11:43:46 +0200 Subject: [PATCH 1/4] test(validators): remove bad practice from test --- test/ng/directive/validatorsSpec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/ng/directive/validatorsSpec.js b/test/ng/directive/validatorsSpec.js index e7a0a13198d6..ca6992d4ea78 100644 --- a/test/ng/directive/validatorsSpec.js +++ b/test/ng/directive/validatorsSpec.js @@ -351,7 +351,7 @@ describe('validators', function() { it('should accept values of any length when maxlength is non-numeric', function() { - var inputElm = helper.compileInput(''); + var inputElm = helper.compileInput(''); helper.changeInputValueTo('aaaaaaaaaa'); $rootScope.$apply('maxlength = "5"'); From d7d172cb20fa7eb5a3304049a79c5f6f9bd392f7 Mon Sep 17 00:00:00 2001 From: Martin Staffa Date: Fri, 5 Aug 2016 16:14:13 +0200 Subject: [PATCH 2/4] fix(input[range]): correctly initialize with interpolated min / max values The interpolation directive only sets the actual element attribute value after a digest passed. That means previously, the min/max values on input range were not set when the first $render happened, so the browser would not adjust the input value according to min/max. This meant the range input and model would not be initialzed as expected. With this change, input range will set the actual element attribute value during its own linking phase, as it is already available on the attrs argument passed to the link fn. Fixes #14982 --- src/ng/directive/input.js | 81 +++++++++++---------- test/ng/directive/inputSpec.js | 129 +++++++++++++++++++++++++++++---- 2 files changed, 159 insertions(+), 51 deletions(-) diff --git a/src/ng/directive/input.js b/src/ng/directive/input.js index 1658dc5580e6..bc8204b3011b 100644 --- a/src/ng/directive/input.js +++ b/src/ng/directive/input.js @@ -1547,10 +1547,12 @@ function rangeInputType(scope, element, attr, ctrl, $sniffer, $browser) { numberFormatterParser(ctrl); baseInputType(scope, element, attr, ctrl, $sniffer, $browser); - var minVal = 0, - maxVal = 100, - supportsRange = ctrl.$$hasNativeValidators && element[0].type === 'range', - validity = element[0].validity; + var supportsRange = ctrl.$$hasNativeValidators && element[0].type === 'range', + minVal = supportsRange ? 0 : undefined, + maxVal = supportsRange ? 100 : undefined, + validity = element[0].validity, + minAttrType = isDefined(attr.ngMin) ? 'ngMin' : isDefined(attr.min) ? 'min' : false, + maxAttrType = isDefined(attr.ngMax) ? 'ngMax' : isDefined(attr.max) ? 'max' : false; var originalRender = ctrl.$render; @@ -1563,6 +1565,42 @@ function rangeInputType(scope, element, attr, ctrl, $sniffer, $browser) { } : originalRender; + if (minAttrType) { + ctrl.$validators.min = minAttrType === 'min' && supportsRange ? + // Since all browsers set the input to a valid value, we don't need to check validity + function noopMinValidator() { return true; } : + // ngMin doesn't set the min attr, so the browser doesn't adjust the input value as setting min would + function minValidator(modelValue, viewValue) { + return ctrl.$isEmpty(viewValue) || isUndefined(minVal) || viewValue >= minVal; + }; + + setInitialValueAndObserver(minAttrType, 'min', minChange); + } + + if (maxAttrType) { + ctrl.$validators.max = maxAttrType === 'max' && supportsRange ? + // Since all browsers set the input to a valid value, we don't need to check validity + function noopMaxValidator() { return true; } : + // ngMax doesn't set the max attr, so the browser doesn't adjust the input value as setting max would + function maxValidator(modelValue, viewValue) { + return ctrl.$isEmpty(viewValue) || isUndefined(maxVal) || viewValue <= maxVal; + }; + + setInitialValueAndObserver(maxAttrType, 'max', maxChange); + } + + function setInitialValueAndObserver(actualAttrName, htmlAttrName, changeFn) { + // e.g. max === max + if (actualAttrName === htmlAttrName) { + // interpolated attributes set the attribute value only after a digest, but we need the + // attribute value when the input is first rendered, so that the browser can adjust the + // input value based on the min/max value + element.attr(htmlAttrName, attr[htmlAttrName]); + } + + attr.$observe(htmlAttrName, changeFn); + } + function minChange(val) { if (isDefined(val) && !isNumber(val)) { val = parseFloat(val); @@ -1577,8 +1615,8 @@ function rangeInputType(scope, element, attr, ctrl, $sniffer, $browser) { var elVal = element.val(); // IE11 doesn't set the el val correctly if the minVal is greater than the element value if (minVal > elVal) { - element.val(minVal); elVal = minVal; + element.val(elVal); } ctrl.$setViewValue(elVal); } else { @@ -1587,23 +1625,6 @@ function rangeInputType(scope, element, attr, ctrl, $sniffer, $browser) { } } - var minAttrType = isDefined(attr.ngMin) ? 'ngMin' : isDefined(attr.min) ? 'min' : false; - if (minAttrType) { - ctrl.$validators.min = isDefined(attr.min) && supportsRange ? - function noopMinValidator(value) { - // Since all browsers set the input to a valid value, we don't need to check validity - return true; - } : - // ngMin doesn't set the min attr, so the browser doesn't adjust the input value as setting min would - function minValidator(modelValue, viewValue) { - return ctrl.$isEmpty(viewValue) || isUndefined(minVal) || viewValue >= minVal; - }; - - // Assign minVal when the directive is linked. This won't run the validators as the model isn't ready yet - minChange(attr.min); - attr.$observe('min', minChange); - } - function maxChange(val) { if (isDefined(val) && !isNumber(val)) { val = parseFloat(val); @@ -1627,22 +1648,6 @@ function rangeInputType(scope, element, attr, ctrl, $sniffer, $browser) { ctrl.$validate(); } } - var maxAttrType = isDefined(attr.max) ? 'max' : attr.ngMax ? 'ngMax' : false; - if (maxAttrType) { - ctrl.$validators.max = isDefined(attr.max) && supportsRange ? - function noopMaxValidator() { - // Since all browsers set the input to a valid value, we don't need to check validity - return true; - } : - // ngMax doesn't set the max attr, so the browser doesn't adjust the input value as setting max would - function maxValidator(modelValue, viewValue) { - return ctrl.$isEmpty(viewValue) || isUndefined(maxVal) || viewValue <= maxVal; - }; - - // Assign maxVal when the directive is linked. This won't run the validators as the model isn't ready yet - maxChange(attr.max); - attr.$observe('max', maxChange); - } } diff --git a/test/ng/directive/inputSpec.js b/test/ng/directive/inputSpec.js index 7b177f33740d..6d7b194a708c 100644 --- a/test/ng/directive/inputSpec.js +++ b/test/ng/directive/inputSpec.js @@ -2877,7 +2877,7 @@ describe('input', function() { expect(inputElm.val()).toEqual('50'); }); - it('should set model to 50 when no value specified', function() { + it('should set model to 50 when no value specified and default min/max', function() { var inputElm = helper.compileInput(''); expect(inputElm.val()).toBe('50'); @@ -2887,7 +2887,7 @@ describe('input', function() { expect(scope.age).toBe(50); }); - it('should parse non-number values to 50', function() { + it('should parse non-number values to 50 when default min/max', function() { var inputElm = helper.compileInput(''); scope.$apply('age = 10'); @@ -2949,8 +2949,20 @@ describe('input', function() { describe('min', function() { if (supportsRange) { + + it('should initialize correctly with non-default model and min value', function() { + scope.value = -3; + scope.min = -5; + var inputElm = helper.compileInput(''); + + expect(inputElm).toBeValid(); + expect(inputElm.val()).toBe('-3'); + expect(scope.value).toBe(-3); + expect(scope.form.alias.$error.min).toBeFalsy(); + }); + // Browsers that implement range will never allow you to set the value < min values - it('should validate', function() { + it('should adjust invalid input values', function() { var inputElm = helper.compileInput(''); helper.changeInputValueTo('5'); @@ -2964,6 +2976,22 @@ describe('input', function() { expect(scope.form.alias.$error.min).toBeFalsy(); }); + it('should set the model to the min val if it is less than the min val', function() { + scope.value = -10; + // Default min is 0 + var inputElm = helper.compileInput(''); + + expect(inputElm).toBeValid(); + expect(inputElm.val()).toBe('0'); + expect(scope.value).toBe(0); + + scope.$apply('value = 5; min = 10'); + + expect(inputElm).toBeValid(); + expect(inputElm.val()).toBe('10'); + expect(scope.value).toBe(10); + }); + it('should adjust the element and model value when the min value changes on-the-fly', function() { scope.min = 10; var inputElm = helper.compileInput(''); @@ -2997,8 +3025,9 @@ describe('input', function() { }); } else { + // input[type=range] will become type=text in browsers that don't support it + it('should validate if "range" is not implemented', function() { - // This will become type=text in browsers that don't support it var inputElm = helper.compileInput(''); helper.changeInputValueTo('5'); @@ -3012,6 +3041,34 @@ describe('input', function() { expect(scope.form.alias.$error.min).toBeFalsy(); }); + it('should not assume a min val of 0 if the min interpolates to a non-number', function() { + scope.value = -10; + var inputElm = helper.compileInput(''); + + expect(inputElm).toBeValid(); + expect(inputElm.val()).toBe('-10'); + expect(scope.value).toBe(-10); + expect(scope.form.alias.$error.min).toBeFalsy(); + + helper.changeInputValueTo('-5'); + expect(inputElm).toBeValid(); + expect(inputElm.val()).toBe('-5'); + expect(scope.value).toBe(-5); + expect(scope.form.alias.$error.min).toBeFalsy(); + + scope.$apply('max = "null"'); + expect(inputElm).toBeValid(); + expect(inputElm.val()).toBe('-5'); + expect(scope.value).toBe(-5); + expect(scope.form.alias.$error.max).toBeFalsy(); + + scope.$apply('max = "asdf"'); + expect(inputElm).toBeValid(); + expect(inputElm.val()).toBe('-5'); + expect(scope.value).toBe(-5); + expect(scope.form.alias.$error.max).toBeFalsy(); + }); + it('should validate even if the min value changes on-the-fly', function() { scope.min = 10; var inputElm = helper.compileInput(''); @@ -3074,6 +3131,7 @@ describe('input', function() { scope.min = 20; scope.$digest(); expect(inputElm).toBeInvalid(); + expect(inputElm.val()).toBe('15'); scope.min = null; scope.$digest(); @@ -3094,6 +3152,17 @@ describe('input', function() { if (supportsRange) { // Browsers that implement range will never allow you to set the value > max value + it('should initialize correctly with non-default model and max value', function() { + scope.value = 130; + scope.max = 150; + var inputElm = helper.compileInput(''); + + expect(inputElm).toBeValid(); + expect(inputElm.val()).toBe('130'); + expect(scope.value).toBe(130); + expect(scope.form.alias.$error.max).toBeFalsy(); + }); + it('should validate', function() { var inputElm = helper.compileInput(''); @@ -3108,9 +3177,16 @@ describe('input', function() { expect(scope.form.alias.$error.max).toBeFalsy(); }); - it('should set the model to the max val if it is more than the max val', function() { - scope.value = 90; - var inputElm = helper.compileInput(''); + it('should set the model to the max val if it is greater than the max val', function() { + scope.value = 110; + // Default max is 100 + var inputElm = helper.compileInput(''); + + expect(inputElm).toBeValid(); + expect(inputElm.val()).toBe('100'); + expect(scope.value).toBe(100); + + scope.$apply('value = 90; max = 10'); expect(inputElm).toBeValid(); expect(inputElm.val()).toBe('10'); @@ -3164,6 +3240,34 @@ describe('input', function() { expect(scope.form.alias.$error.max).toBeFalsy(); }); + it('should not assume a max val of 100 if the max attribute interpolates to a non-number', function() { + scope.value = 120; + var inputElm = helper.compileInput(''); + + expect(inputElm).toBeValid(); + expect(inputElm.val()).toBe('120'); + expect(scope.value).toBe(120); + expect(scope.form.alias.$error.max).toBeFalsy(); + + helper.changeInputValueTo('140'); + expect(inputElm).toBeValid(); + expect(inputElm.val()).toBe('140'); + expect(scope.value).toBe(140); + expect(scope.form.alias.$error.max).toBeFalsy(); + + scope.$apply('max = null'); + expect(inputElm).toBeValid(); + expect(inputElm.val()).toBe('140'); + expect(scope.value).toBe(140); + expect(scope.form.alias.$error.max).toBeFalsy(); + + scope.$apply('max = "asdf"'); + expect(inputElm).toBeValid(); + expect(inputElm.val()).toBe('140'); + expect(scope.value).toBe(140); + expect(scope.form.alias.$error.max).toBeFalsy(); + }); + it('should validate even if the max value changes on-the-fly', function() { scope.max = 10; var inputElm = helper.compileInput(''); @@ -3245,22 +3349,21 @@ describe('input', function() { describe('min and max', function() { - it('should keep the initial default value when min and max are specified', function() { + it('should set the correct initial value when min and max are specified', function() { scope.max = 80; scope.min = 40; var inputElm = helper.compileInput(''); - expect(inputElm.val()).toBe('50'); - expect(scope.value).toBe(50); + expect(inputElm.val()).toBe('60'); + expect(scope.value).toBe(60); }); - it('should set element and model value to min if max is less than min', function() { scope.min = 40; var inputElm = helper.compileInput(''); - expect(inputElm.val()).toBe('50'); - expect(scope.value).toBe(50); + expect(inputElm.val()).toBe('70'); + expect(scope.value).toBe(70); scope.max = 20; scope.$digest(); From 45f474bd83b5a453563bcc040773508278e92bb7 Mon Sep 17 00:00:00 2001 From: Martin Staffa Date: Sun, 7 Aug 2016 12:46:50 +0200 Subject: [PATCH 3/4] fix(input[range]): set correct value when max < min --- src/ng/directive/input.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/ng/directive/input.js b/src/ng/directive/input.js index bc8204b3011b..02dd7f6ba3a4 100644 --- a/src/ng/directive/input.js +++ b/src/ng/directive/input.js @@ -1640,7 +1640,8 @@ function rangeInputType(scope, element, attr, ctrl, $sniffer, $browser) { // IE11 doesn't set the el val correctly if the maxVal is less than the element value if (maxVal < elVal) { element.val(maxVal); - elVal = minVal; + // IE11 and Chrome don't set the value to the minVal when max < min + elVal = maxVal < minVal ? minVal : maxVal; } ctrl.$setViewValue(elVal); } else { From 250801c681ff2a3efdacba91ca34c971eae54ffa Mon Sep 17 00:00:00 2001 From: Martin Staffa Date: Thu, 11 Aug 2016 19:29:56 +0200 Subject: [PATCH 4/4] fix(input[range]): remove support for ngMin / ngMax Since ng prefixed attributes do not set their corresponding element attribute, the range input would always have min = 0, and max = 100 (in supported browsers), regardless of the value in ngMin/ngMax. This is confusing and not very useful, so it's better to not support them at all. --- src/ng/directive/input.js | 50 +++++++++----------- test/ng/directive/inputSpec.js | 85 ---------------------------------- 2 files changed, 21 insertions(+), 114 deletions(-) diff --git a/src/ng/directive/input.js b/src/ng/directive/input.js index 02dd7f6ba3a4..ee506a589efe 100644 --- a/src/ng/directive/input.js +++ b/src/ng/directive/input.js @@ -1061,8 +1061,11 @@ var inputType = { * Angular will also update the model value. * * Automatic value adjustment also means that a range input element can never have the `required`, - * `min`, or `max` errors, except when using `ngMax` and `ngMin`, which are not affected by automatic - * value adjustment, because they do not set the `min` and `max` attributes. + * `min`, or `max` errors. + * + * Note that `input[range]` is not compatible with`ngMax` and `ngMin`, because they do not set the + * `min` and `max` attributes, which means that the browser won't automatically adjust the input + * value based on their values, and will always assume min = 0 and max = 100. * * @param {string} ngModel Assignable angular expression to data-bind to. * @param {string=} name Property name of the form under which the control is published. @@ -1070,14 +1073,6 @@ var inputType = { * than `min`. Can be interpolated. * @param {string=} max Sets the `max` validation to ensure that the value entered is less than `max`. * Can be interpolated. - * @param {string=} ngMin Takes an expression. Sets the `min` validation to ensure that the value - * entered is greater than `min`. Does not set the `min` attribute and therefore - * adds no native HTML5 validation. It also means the browser won't adjust the - * element value in case `min` is greater than the current value. - * @param {string=} ngMax Takes an expression. Sets the `max` validation to ensure that the value - * entered is less than `max`. Does not set the `max` attribute and therefore - * adds no native HTML5 validation. It also means the browser won't adjust the - * element value in case `max` is less than the current value. * @param {string=} ngChange Angular expression to be executed when the ngModel value changes due * to user interaction with the input element. * @@ -1551,8 +1546,8 @@ function rangeInputType(scope, element, attr, ctrl, $sniffer, $browser) { minVal = supportsRange ? 0 : undefined, maxVal = supportsRange ? 100 : undefined, validity = element[0].validity, - minAttrType = isDefined(attr.ngMin) ? 'ngMin' : isDefined(attr.min) ? 'min' : false, - maxAttrType = isDefined(attr.ngMax) ? 'ngMax' : isDefined(attr.max) ? 'max' : false; + hasMinAttr = isDefined(attr.min), + hasMaxAttr = isDefined(attr.max); var originalRender = ctrl.$render; @@ -1565,20 +1560,20 @@ function rangeInputType(scope, element, attr, ctrl, $sniffer, $browser) { } : originalRender; - if (minAttrType) { - ctrl.$validators.min = minAttrType === 'min' && supportsRange ? + if (hasMinAttr) { + ctrl.$validators.min = supportsRange ? // Since all browsers set the input to a valid value, we don't need to check validity function noopMinValidator() { return true; } : - // ngMin doesn't set the min attr, so the browser doesn't adjust the input value as setting min would + // non-support browsers validate the range function minValidator(modelValue, viewValue) { return ctrl.$isEmpty(viewValue) || isUndefined(minVal) || viewValue >= minVal; }; - setInitialValueAndObserver(minAttrType, 'min', minChange); + setInitialValueAndObserver('min', minChange); } - if (maxAttrType) { - ctrl.$validators.max = maxAttrType === 'max' && supportsRange ? + if (hasMaxAttr) { + ctrl.$validators.max = supportsRange ? // Since all browsers set the input to a valid value, we don't need to check validity function noopMaxValidator() { return true; } : // ngMax doesn't set the max attr, so the browser doesn't adjust the input value as setting max would @@ -1586,17 +1581,14 @@ function rangeInputType(scope, element, attr, ctrl, $sniffer, $browser) { return ctrl.$isEmpty(viewValue) || isUndefined(maxVal) || viewValue <= maxVal; }; - setInitialValueAndObserver(maxAttrType, 'max', maxChange); + setInitialValueAndObserver('max', maxChange); } - function setInitialValueAndObserver(actualAttrName, htmlAttrName, changeFn) { - // e.g. max === max - if (actualAttrName === htmlAttrName) { - // interpolated attributes set the attribute value only after a digest, but we need the - // attribute value when the input is first rendered, so that the browser can adjust the - // input value based on the min/max value - element.attr(htmlAttrName, attr[htmlAttrName]); - } + function setInitialValueAndObserver(htmlAttrName, changeFn) { + // interpolated attributes set the attribute value only after a digest, but we need the + // attribute value when the input is first rendered, so that the browser can adjust the + // input value based on the min/max value + element.attr(htmlAttrName, attr[htmlAttrName]); attr.$observe(htmlAttrName, changeFn); } @@ -1611,7 +1603,7 @@ function rangeInputType(scope, element, attr, ctrl, $sniffer, $browser) { return; } - if (supportsRange && minAttrType === 'min') { + if (supportsRange) { var elVal = element.val(); // IE11 doesn't set the el val correctly if the minVal is greater than the element value if (minVal > elVal) { @@ -1635,7 +1627,7 @@ function rangeInputType(scope, element, attr, ctrl, $sniffer, $browser) { return; } - if (supportsRange && maxAttrType === 'max') { + if (supportsRange) { var elVal = element.val(); // IE11 doesn't set the el val correctly if the maxVal is less than the element value if (maxVal < elVal) { diff --git a/test/ng/directive/inputSpec.js b/test/ng/directive/inputSpec.js index 6d7b194a708c..e3c71c4c1b27 100644 --- a/test/ng/directive/inputSpec.js +++ b/test/ng/directive/inputSpec.js @@ -3105,49 +3105,6 @@ describe('input', function() { } }); - describe('ngMin', function() { - - it('should validate', function() { - var inputElm = helper.compileInput(''); - - helper.changeInputValueTo('1'); - expect(inputElm).toBeInvalid(); - expect(scope.value).toBeFalsy(); - expect(scope.form.alias.$error.min).toBeTruthy(); - - helper.changeInputValueTo('100'); - expect(inputElm).toBeValid(); - expect(scope.value).toBe(100); - expect(scope.form.alias.$error.min).toBeFalsy(); - }); - - it('should validate even if the ngMin value changes on-the-fly', function() { - scope.min = 10; - var inputElm = helper.compileInput(''); - - helper.changeInputValueTo('15'); - expect(inputElm).toBeValid(); - - scope.min = 20; - scope.$digest(); - expect(inputElm).toBeInvalid(); - expect(inputElm.val()).toBe('15'); - - scope.min = null; - scope.$digest(); - expect(inputElm).toBeValid(); - - scope.min = '20'; - scope.$digest(); - expect(inputElm).toBeInvalid(); - - scope.min = 'abc'; - scope.$digest(); - expect(inputElm).toBeValid(); - }); - }); - - describe('max', function() { if (supportsRange) { @@ -3303,48 +3260,6 @@ describe('input', function() { } }); - describe('ngMax', function() { - - it('should validate', function() { - var inputElm = helper.compileInput(''); - - helper.changeInputValueTo('20'); - expect(inputElm).toBeInvalid(); - expect(scope.value).toBeUndefined(); - expect(scope.form.alias.$error.max).toBeTruthy(); - - helper.changeInputValueTo('0'); - expect(inputElm).toBeValid(); - expect(scope.value).toBe(0); - expect(scope.form.alias.$error.max).toBeFalsy(); - }); - - it('should validate even if the ngMax value changes on-the-fly', function() { - scope.max = 10; - var inputElm = helper.compileInput(''); - - helper.changeInputValueTo('5'); - expect(inputElm).toBeValid(); - - scope.max = 0; - scope.$digest(); - expect(inputElm).toBeInvalid(); - - scope.max = null; - scope.$digest(); - expect(inputElm).toBeValid(); - - scope.max = '4'; - scope.$digest(); - expect(inputElm).toBeInvalid(); - - scope.max = 'abc'; - scope.$digest(); - expect(inputElm).toBeValid(); - }); - - }); - if (supportsRange) { describe('min and max', function() {