This comment has been minimized.

If possible, I'd be inclined to include the minlength and maxlength attrs here too for the built-in browser behavior. Otherwise this technique isn't actually equivalent to the template-driven approach.

This comment has been minimized.

Glad you found the accessibility issues, but the entire point of model-driven is that the knowledge of the validation is only in the code. That way the validation can be adjusted based on the user or on the state (different validation for add vs edit for example). So we don't want to put the validation attributes into the HTML in the model-driven cases. I'm putting together an issue for Angular Forms to suggest that aria attributes are automatically added to the HTML based on the code.

This comment has been minimized.

Because the validation is defined in the code, it could be changed based on the user or situation. For example, a field may be required for a normal user, but not an admin user. When these validation rules are in the code, there is no easy way to put the validation aria attributes in the HTML and have them work properly. (I supposed property binding could work ... but is messy in this case.)
Just as the dev puts the basic attributes in the HTML but the validation rules in the code, it would make sense for the dev to put the basic aria attributes in the HTML, but the validation aria info would be defined in the code. Angular should then add the appropriate validation and aria attributes to the HTML automatically.

This comment has been minimized.

edited

We have only three ARIA attributes to consider in most scenarios if developers use the native form elements.

aria-required: If we use HTML5 then the required attribute is enough so we only need to add this if we want to be seriously backwards compatible.

aria-invalid: True or false based on the current validity. Which can easily be sourced from the control object.

Sure, these two can be added by Angular. But we still miss one.

aria-describedby: needs to contain, at any moment, the ID or list of IDs of any visible form validation error message elements. But we also need to edit this list if we want to put in, say, a descriptor field. And say we want to hide the descriptor field when the error messages are visible this has to also be possible. I dunno how reasonable it is to let Angular do this as we will need to provide something like a map of generated/hardcoded field ID's linked to validation errors and have an ngClass type of function to add and remove IDs to the ones already there. Having IDs in this list that are not present on the page, even if it is hidden, creates accessibility violations, so it has to be dynamic.

Given this, I still would think that it should be Angular's job to add the actual validation attributes to the form control, like maxlength, and required and pattern and the developers job to add the two (maybe three) aria attributes.

Unless it is super easy for them to solve the ID linkup of aria-describedby otherwise there will be the situation where only half of the ARIA attrs get added and the dev has to do the other one and manage it.

This comment has been minimized.

Does this technique produce rendered HTML equivalent to the template-driven solution? If not, I think that needs to be addressed in some way - either by updating the code so both techniques produce the same HTML or mentioning it in here somewhere.

This comment has been minimized.

Added some a11y comments. A lot of these are just issues in the existing TOH code, so I'm not sure if they should be addressed as new cookbooks are added or cleaned up later. Thoughts? CC @AlmeroSteyn@wardbell

This comment has been minimized.

A good thing we do in the cookbooks is to add a "Learn more at XXX" with certain things.

I missed it for two topics: What's dirty and what's touched. That explanation of why we want it is good, but a "learn more" and link to the appropriate part of the form guide would be lovely.

Same happens with "controls, controlGroup", a learn more would be nice.

On the other hand, you mention twice that "template-driven" can't be unit tested. Why? I haven't try but it should be posible by changing some values, triggering changes and checking the if model has errors.

You mention that we may call buildForm at ngOnInit. I would do that directly. It is just 2-3 lines of code and sounds better :)

This comment has been minimized.

I could not find anywhere else that we really covered control and controlgroup since we don't have a cookbook or anything on model-driven forms. (I had suggested that this cookbook cover forms and validation ... but the recommendation was to focus on just the validation.) Do you know of something that is just in an unpublished branch?

This comment has been minimized.

edited

How important is the change to use ngInit? I didn't use it here because I wanted to keep this simple and focused on validation ... not on explaining how to implement an interface and handle lifecycle hooks.

This comment has been minimized.

@naomiblack I would really love to do this ... especially since I've spent some time with Kara going through the changes. But I am heading to Japan this afternoon and am not taking a computer with me. So I won't be able to do this until I get back at the end of the month.

This comment has been minimized.

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

1 similar comment

This comment has been minimized.

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

This comment has been minimized.

I really enjoyed reviewing this cookbook. It brought some nagging questions into sharper relief.

The more I looked at it, the more I felt that we remain uncomfortably unclear on the ReactiveForms value proposition.

Frankly, I fail to see anything particularly reactive in the Reactive form approach. We need some insight here that perhaps @kara can supply.

I dispelled the myth that the template approach involves more, ugly HTML. It seemed to me that myth arises from primitive examples rather than the template approach per se.

Yes, the primitive examples bake error logic and messages into the HTML as we see in the first example. But this is not an intrinsic defect of the template approach. In my last commit I showed an intermediate Template 2 form that _uses the same validation message code as in the Reactive form example_.

I aligned code and html for Template 1, Template 2, and Reactive 3 examples such that the reader can see the progression and true differences.

I am troubled by the claim that the reactive approach is easier to test. There isn't any evidence in support of the notion in this example (although there is an opening for a testability claim in my comments below).

AFAIK, the substantive differences between template and reactive approaches are:

Reactive defines the form controls in code within the component class; layout and styling remain in html.

Reactive defines the validation functions in code within the component class rather than as HTML validation attributes.

My intuition is that developers will find #2 to be the most significant. It also creates an architectural dilemma.

OTOH, having validation in code rather than attributes has advantages:

richer validation logic is possible

the validation logic could be re-factored into the model (or model metadata) rather than hard-coded in the component.

the validation logic can be tested independently of the component's interaction with its template, especially if refactored into the model.

OTOH, the glaring negative is lack of ARIA support. It's easy but tedious to add that support manually (I updated the sample with a hint in that direction). Ideally the Angular library can do more to be helpful.

This comment has been minimized.

edited

Talked to Kara and got some additional clarification on required and ARIA.

I removed the isRequired function which is tough to talk about and not recommended by her. She says we should keep the required attribute and add Validator.required. Do both. We'll only need the validator function in a future version of forms; it will add the required attribute for us.

This comment has been minimized.

edited

I thought a separate cookbook on custom validation might be useful. It could cover some specific cases that people often ask about ... such as cross-field validation (such as a start and end date) and async validation (such as checking that a userName already exists).

This comment has been minimized.

Now I'm torn. I think it is important to see where the true differences are w/r/t validation in the two approaches. I thought custom validation might have been one of them. It would have been a shame not to show it.

Now that I know there is symmetry (if not parity) on that front, I'm not sure what to do. Pulling custom validation out seems to undersell the story.

I agree there is more to cover ... cross-field, async, who knows what else. Can't do it all in here.

This comment has been minimized.

Decided that we should show custom validation so that readers know it is possible. By putting the validation logic in its own file (in the shared folder), we postpone discussion to the bottom of the cookbook so that it doesn't distract from the main story.

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.