Issue with grouped checkboxes in firefox

[jamesbrobb]

Can someone comfirm this please. I'm using:

OSX 10.7.5
Firefox 33.1.1

1. Go to either

http://djangular.aws.awesto.com/form_validation/

or

http://djangular.aws.awesto.com/combined_validation/

2. Scroll to 'notify me' and select 'postcard' -- i get the 'required' error message

3. deselect 'postcard' -- i get the valid tick

4. select more than one checkbox in the group and the valid tick appears

5. deselect all and the tick stays until you reselect one

Also, once you've force submitted the form to ajax (in any browser) with no checkboxes selected, the valid tick does not appear once any checkboxes are selected.

[jrief]

Just tested with FF 29.0.1 on MacOSX Mavericks:

Its even worse: After deselecting postcard, the green tick remains. On Chrome the "x This field is required" message appears.

[jrief]

Can reproduce this error with FF 34.0

[jamesbrobb]

I'm having no joy fixing it, accept for wrapping the 'validate' function (the 'change' handler in the validateMultipleFields directive) logic in a $timeout to force a ui re-render. But this intermittently causes a flicker in FF where the error message shows first before the tick displays.

If you inspect all the values and object states (form and fields) when the 'change' handler fires they're correct. As are the css classes applied to the form and fields. It just looks like FF is rendering the page before they're fully updated.

[jamesbrobb]

I've put together a paired down plunker just to make sure that the issue occurs in isolation.

http://plnkr.co/edit/5abVpPofLytqEgE4qZlV?p=preview

This is a potential (not 100% satisfactory) fix using $timeout. The flicker occurs the first time a checkbox is clicked.

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

[jamesbrobb]

I've implemented a fix by using ng-change instead of on('change') to listen for the change in each checkbox.

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

I'm presuming the issue is caused by the fact that firefox dispatches it's 'click' and 'change' events in the opposite order to every other browser (click first) and the click is possibly causing the digest cycle to run before the values have all been updated.

I'm afraid i don't have enough experience with django forms to confidently implement this fix myself, as i'm not sure of all the combinations of elements/widgets it affects?

[jamesbrobb]

Ok, this is a pure angular based fix, no mods to the python.

I've fixed the two bugs i originally listed and fixed a couple of other issues i've come across in the djangoForms directive relating to 'rejected' error cleanup.

[jrief]

without ['$parse', function($parse) { this file can't by uglyfied.

[jamesbrobb]

Sorry, i use ng-annotate with gulp, so completely forgot to add the uglify safe injections.

[jrief]

I need some time to understand your code. Please be patient. Made some annotations.

[jamesbrobb]

No problem, whenever you get to it. I was just cleaning up my comments from yesterday to make sure they where as clear and concise as possible.

[jrief]

Sorry, for for the delay.
Thanks for the PR.

[jrief]

BTW, PR #144 will soon be merged and we will release version 0.8.0.
Could you please add a small summary of this PR to docs/changelog.rst.

[jrief]

Today, in another project I used the form validation for multiple fields.
Something, which caused a major problem in my project, was your code modification of this PR.
You added ng-change="changed()" as an attribute to input fields. This manipulation of the DOM, in AngularJS is an anti-pattern.
I will remove this and return to a pure link function.

[jamesbrobb]

Can you be a bit more specific? What exactly was the major problem that it caused?

I didn't realise manipulating the DOM in that way was considered an anti-pattern? What makes it an anti-pattern? Can you point me in the direction of an explanation of this, thanks?

[jrief]

I don't like, that a directive adds other ng-... directives to the DOM and I strongly believe that this use case was not intended by the Angular team.

Just revered your PR from December. However, I now fixed the FF issue myself in a much cleaner and simpler way. Please recheck with your installation.

What was missing for FF, was the scope.$apply() call. I don't know why it worked on Chrome, but anyway! Adding ng-change="changed()" to the element, implicitly fired scope.$apply(), but that kind of hack is not what we should do.

[jamesbrobb]

If you look at my comments you'll see that the first quick fix i came up with for this issue, was to simply wrap the change event handlers call to validate in a $timeout. Which then safely calls scope.$apply. Which is essentially what you're doing. So i understand why the issue is occurring.

The problem i have with the quick fix of scope.$apply, is that it feels like a hack. Its just a brute force solution that fails to understand why this issue only occurs in FF, but not in chrome, safari or IE.

That aside, i can understand how you might wrongly consider my solution using ng-change to be a 'hack', as you believe it just accidentally fixes the issue. But i can assure you it was very much an intentional and considered approach.

Within angular an inputs current state is represented by its ngModel. But your logic mixes angular model/controller state and DOM state (checkbox.checked) to determine the current state of the form. Which is not the correct way to handle this scenario. You should be monitoring the checkbox's ngModel for change (thus the use of the ng-change directive) and then use those values to determine the current state of the form. The ngModel should be the single source of current state.

Regardless of how ng-change is added to the input (i'm currently looking at an alternative solution that uses ngModel.$viewChangeListeners instead - but am also wondering if this attribute couldn't just be added in the python widget?), i strongly believe that what i've outlined and initially implemented, is the correct way to deal with this scenario.

Also going forward, the current solution causes issues when ng-model-options is used on the input.

[jamesbrobb]

One other thing. The reversion has reintroduced an issue. If you submit the form via ajax with no inputs selected for 'Notify by', the rejected error message is correctly displayed.

If you then select a checkbox, the error message disappears, but the valid tick never re-appears, as the parent ngForm needs to have its 'rejected' error and $message cleared.

So at line 155, below

formCtrl.$setValidity('required', valid);

you need to add

formCtrl.$setValidity('rejected', true);
formCtrl.$message = '';

Also each child fields 'rejected' $viewChangeListeners are not removed. Although TBH i'm not 100% clear on what there exact purpose is, as the values that are cleared in the listener function, don't appear to ever actually be set anywhere?

[jrief]

The problem i have with the quick fix of scope.$apply, is that it feels like a hack. Its just a brute force solution that fails to understand why this issue only occurs in FF, but not in chrome, safari or IE.

Since I use the jQuery .on('change') event handler, which is outside the control of AngularJS, scope.$apply() IMHO is the appropriate solution.

The big problem I had with your solution was, that I added a ng-change directive to one of my Radio fields. It took me a while to find out that it is overridden by the validate-multiple-fields directive.

ngModel.$viewChangeListeners seems to be an interesting solution. Will dig into the docs for that.

Thanks for reporting the seconds issue. What's that 'rejected' property? This is not part of the form errors, so I did not understand its purpose.

[jamesbrobb]

'rejected' is part of the djangoForm factory logic. It's how server errors are displayed on each input.

[jamesbrobb]

@jrief Just to clarify why 'rejected' needs to be removed from the form in this instance.

When 'rejected' errors are applied in the djangoForm factory setErrors method, this block of logic is run, that loops the errors and uses the key to apply each error to a field.

angular.forEach(errors, function(errors, key) {
    var field;
    if (errors.length > 0) {
        if (key === NON_FIELD_ERRORS) {
            form.$message = errors[0];
            form.$setPristine();
        } else if (form.hasOwnProperty(key)) {
            field = form[key];
            field.$message = errors[0];
            field.$setValidity('rejected', false);
            field.$setPristine();
            if (isField(field)) {
                resetFieldValidity(field);
            } else {
                // this field is a composite of input elements
                angular.forEach(field, function(subField, subKey) {
                    //this can occur in 1.3 due to new 'pending' prop being undefined
                    if (subField && isField(subField)) {
                        resetFieldValidity(subField);
                    }
                });
            }
        }
    }
});

Using the example form as an example, a 'required' error is returned for the notifyme field if an ajax submission is made and nothing is supplied. But notifyme is not a field, it's an ngFormController that manages the selection of the 4 'notifyme' checkboxes.

So in this block of logic from the above code (your original code, not a later addition) , the field having everything set on it is actually the notifyme ngFormController.

field = form[key];
field.$message = errors[0];
field.$setValidity('rejected', false);
field.$setPristine();

This is the same formController that's returned by require in the validateMultipleFields directive. Which is why those properties need to be cleared in the directive to reset the state of the ngFormController. If $setValidity('rejected', true) is not called, the 'rejected' error is never removed from the notifyme ngFormController (which is why the valid tick is never displayed) and it's $invalid state is always 'true'. So its parent formController is also never valid.

You can see this on the demo form by following these steps:

http://djangular.aws.awesto.com/combined_validation/

1. Select 'Force Submission via Ajax'

• 'Notify Me' displays its 'rejected error' as my_form.notifyme.$message and my_form.notifyme.$pristine are both true

2. Select any of the 'Notify Me' checkboxes

• the error message disappears but the valid tick does not show as my_form.notifyme is still invalid due to the existing 'rejected' error. But as my_form.notifyme.$pristine is now false (due to the selection) the error message is no longer visible.

3. Fill in the rest of the form so that every field is valid

• even though the form now appears to be valid, the 'Submit via Post' and 'Submit via Ajax' buttons never become enabled.

4. Select 'Force Submission via Ajax'

• the form is successfully submitted, showing that the data was actually valid

---

Although TBH i'm not 100% clear on what there exact purpose is, as the values that are cleared in the listener function, don't appear to ever actually be set anywhere?

Returning to this point, i'm not sure what purpose it serves also adding the $viewChangeListeners (through the resetFieldValidity function) to each subField of the ngFormController. As the values that are cleared in that listener function are never actually set on each subField?

// this field is a composite of input elements
angular.forEach(field, function(subField, subKey) {
    if (subField && isField(subField)) {
        resetFieldValidity(subField);
    }
});

[jrief]

@jamesbrobb
Can you please send me your email address.

[jamesbrobb]

I've added it to my profile

[jamesbrobb]

#160 offers an alternative fix for everything mentioned in this issue