Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

feat(input): use ValidityState for required state #7123

Closed
wants to merge 2 commits into from
Closed

feat(input): use ValidityState for required state #7123

wants to merge 2 commits into from

Conversation

starr0stealer
Copy link

Change NgModelController $isEmpty method to consider HTML5 constraint validation
in supporting browsers, fallback to normal empty data check

Change NgModelController $isEmpty method to consider HTML5 constraint validation
in supporting browsers, fallback to normal empty data check
@mary-poppins
Copy link

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

  • Uses the issue template (#7123)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

@@ -1613,7 +1613,12 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$
* @returns {boolean} True if `value` is empty.
*/
this.$isEmpty = function(value) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changing $isEmpty is not the right approach because it gets overwritten a lot. Also, the patch needs tests

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I chose to update NgModelController $isEmpty because I found that only checkboxInputType and ngListDirective had overrides to this method, which neither override (or input type) appeared to be lacking in validation logic. I originally implemented the logic in requiredDirective, but I felt that bloated the directive signature with the isObject(validity) check in order to support fallback for browsers that don't implement the HTML5 validation.

I am all ears on a more recommended placement of this change.

In regards of tests, I was unsure of any additional tests to be created as I seen 'required' already existed and passed still. Nor did I see any tests comparing the angular result to the ValidityState object

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thats true, it is a stupidly hard thing to test. Mock element might be capable though.

However I don't think putting this in $isEmpty is the right approach. The check for a validity state only needs to happen once at compile time, and would look like this:

var validity = element.prop('validity');
if (!isObject(validity)) {
  validity = {
    valid: true
  };
}

This should be good enough, because then the test can simply be

if (validity.valueMissing || ctrl.$isEmpty(value))

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your right about the object check only once at compile time, I completely over looked that. Your recommended changes also fixes my concerns with method bloat. Hopefully I fully understand what you meant, because I updated requiredDirective which now looks like

var requiredDirective = function() {
  return {
    require: '?ngModel',
    link: function(scope, elm, attr, ctrl) {
      if (!ctrl) return;
      attr.required = true; // force truthy in case we are on non input element

      var validity = elm.prop('validity');
      if (!isObject(validity)) {
        validity = {
          valid: true
        };
      }

      var validator = function(value) {
        if (attr.required && (validity.valueMissing || ctrl.$isEmpty(value))) {
          ctrl.$setValidity('required', false);
          return;
        } else {
          ctrl.$setValidity('required', true);
          return value;
        }
      };

      ctrl.$formatters.push(validator);
      ctrl.$parsers.unshift(validator);

      attr.$observe('required', function() {
        validator(ctrl.$viewValue);
      });
    }
  };
};

@starr0stealer
Copy link
Author

This PR refers to changes spoken about in #4857, as well as a step forward in resolving #6818

I have an open issue submitted with the Chromium project that once fixed would fully resolve #6818

Google Chromium Issue https://code.google.com/p/chromium/issues/detail?id=363816

Change NgModelController $isEmpty method to consider HTML5 constraint validation
in supporting browsers, fallback to normal empty data check
var validator = function(value) {
if (attr.required && ctrl.$isEmpty(value)) {
if (attr.required && (validity.valueMissing || ctrl.$isEmpty(value))) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless Ian Hixon agrees to alter the spec (and in turn, fixes get added to browsers), what we really need to check for here is !validity.badInput --- because if badInput is set, then we're really suffering from bad input and not a "real" value missing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'd also need to change textInputType() to run call setViewValue even if the value remains the same, if there is a validity state

@starr0stealer
Copy link
Author

Closing this PR because I am adding check for validity.badInput, which would make this a fix instead of a feature request.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants