Fixes to select and ngOptions - "required" validation and behavior with empty / unknown option #15922
Conversation
Narretz
force-pushed
the
fix-select-ngoptions-required-clean
branch
from
ab0ab63 to
5eff109
Compare
| @@ -449,12 +449,12 @@ var ngOptionsDirective = ['$compile', '$document', '$parse', function($compile, | |||
| if (!multiple) { | |||
|
|
|||
| selectCtrl.writeValue = function writeNgOptionsValue(value) { | |||
| var selectedOption = options.selectValueMap[selectElement.val()]; | |||
| var selectedOption = selectElement[0].options[selectElement[0].selectedIndex]; | |||
There was a problem hiding this comment.
Why do you need to skip jqLite/jQuery here?
There was a problem hiding this comment.
We don't. I just didn't think to use jQuery here. How do you get the selected option with jquery?
There was a problem hiding this comment.
Hmm, I was just comparing it to the previous code which used options.selectValueMap. Could you explain the change?
There was a problem hiding this comment.
Ah, I see. Regular select was using the same technique - getting the option directly from the DOM, so I made it consistent. I don't think there's a huge difference, it just felt right to look at the actual DOM
|
I am not sure that (a) is the desired behaviour here. For me required error implies that the input is missing a value. In this scenario it has a value but it is an invalid one. It feels like saying that a number input box has the required error if its value is higher than the maximum allowed. |
|
@petebacondarwin Hmm ... I implemented this based on the assumptions / opinions found here: #13172 I know, it's been a while :D I agree that "unknown" is different than "empty" - the latter specifically means the select is "unselected", so setting required error seems solid. For unknown, I guess it depends on the app logic if it fulfills the required condition. The thing is that selects basically only have required validation, because the app is usually responsible for giving the select list. I don't insist on this change, but I think we should provide a way to manipulate this behavior. The custom isEmpty method can be implemented by a 3rd party directive, but for that we need to expose / document properties of the select controller, mainly ctrl.unknownOption ... and maybe some others. This would obviously put us in a position where we expose more API that we have to maintain :o But it would also allow people to create a different error, e.g. "unmatched value" that is different than "required". |
src/ng/directive/select.js
Outdated
| !!selectCtrl.unknownOption.parent().length || | ||
| // When the empty option is selected from the scope, then the viewValue can be different | ||
| // than '', and the originalIsEmpty fn will not catch this case | ||
| (selectCtrl.hasEmptyOption && |
There was a problem hiding this comment.
Note that this condition only affects empty option required validation, not unknown option - basically, it's a separate fix - commit message needs to be updated.
|
OK, I think exposing some more public API to empower developers is a reasonable goal here. |
…r "unknown" option
…ch empty option When a regular / ngOptions select has an explicit *empty* option, this option can be selected by the user and will set the model to `null`. It is also selected when the model is set to `null` or `undefined`. When the model is set to a value that does not match any option value, and is also not `null` or `undefined`, the *unknown* option is inserted and selected - this is an explicit marker that the select is in an invalid / unknown state, which is different from an allowed empty state. Previously, regular selects followed this logic, whereas ngOptions selects selected the empty option in the case described above. This patch makes the behavior consistent between regular / ngOptions select - the latter will now insert and select the unknown option. The order of the options has been fixed to unknown -> empty -> actual options.
Narretz
force-pushed
the
fix-select-ngoptions-required-clean
branch
from
5eff109 to
f6021e9
Compare
|
I've removed the change about required and the unknown option, and expanded on the tests around this behavior. PR is ready to review. |
|
I'll review tomorrow morning |
|
Please merge when you have a moment @Narretz |
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
bugfixes
What is the current behavior? (You can also link to an open issue here)
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change?
Possibly? For (a), the following situation is possible: The model is programatically set to a value that is no longer in select options, and the select is required. With this patch, the user cannot keep the unmatched value, because of the required error, which considers this value invalid (just realized this, so it's not in the commit message).
For (b), it's unlikely that anyone relied on the fact that the empty option is selected instead of the unmatched option - the fixd behavior was also present in regular select since the beginning.
Please check if the PR fulfills these requirements
Other information: