New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

f[eat|ix](select): support values added by ngValue #13962

Merged
merged 4 commits into from Jul 1, 2016

Conversation

Projects
None yet
4 participants
@Narretz
Contributor

Narretz commented Feb 6, 2016

select elements with ngModel will now set ngModel to option values added by ngValue.
This allows setting values of any type without the use of ngOptions.

Interpolations inside attributes can only be strings, but the ngValue directive uses attrs.$set,
which does not follow any type restriction. Any $observe on the value attribute will therefore receive
the original value (result of ngValue expression). However, when a user selects an option, the browser
sets the select value to the actual option's value attribute, which is still always a string.
For that reason, when option are added by ngValue, we set the hashed value of the original value in
the value attribute and store the actual value in an extra map. When the select value changes, we
read access the actual value via the hashed select value.

Closes #9842
Closes #6297

  • Test that it takes precedence over interpolated option / option text
  • Test for multiple selections
  • Fix failing Chrome tests
  • Docs

Talking points:

  • The implementation works reasonably well, although it's a bit hackyspecial
  • With ngValue, option values are hash values, not the real values (which makes sense, but might be confusing)
  • afaik angular 2 suffers from the same problem as ng1 right now
scope.selected = 'UPDATEDVALUE';
scope.$digest();
// expect(element.find('option').eq(0).prop('selected')).toBe(true); not selected in Chrome?

This comment has been minimized.

@Narretz

Narretz May 18, 2016

Contributor

I wanted to fix this, but I'm not sure it's necessary. I don't think browsers have to set selected when the value of the select changes. It's strange though that is set before changing the select value via model update.

@@ -116,9 +120,26 @@ var SelectController =
self.registerOption = function(optionScope, optionElement, optionAttrs, interpolateValueFn, interpolateTextFn) {
if (interpolateValueFn) {
if (interpolateValueFn === true) {

This comment has been minimized.

@petebacondarwin

petebacondarwin May 18, 2016

Member

I am not keen on using true as a special flag here.
Perhaps we could just add yet another parameter to the registerOption method?

This comment has been minimized.

@petebacondarwin

petebacondarwin May 18, 2016

Member

Or perhaps we don't need to do that at all, we could just check whether optionAttrs.ngValue is defined?

@petebacondarwin

This comment has been minimized.

Member

petebacondarwin commented May 18, 2016

This test fails:

      it('should work update the value as a result of changes to the options', function() {
        var A = { name: 'A'}, B = { name: 'B'}, C = { name: 'C'};
        scope.options = [A, B, C];
        scope.obj = {};
        compile(
          '<select ng-model="obj.value">' +
            '<option ng-repeat="option in options" ng-value="option">{{$index}}</option>' +
          '</select>'
        );

        var optionElements = element.find('option');
        expect(optionElements.length).toEqual(4);
        browserTrigger(optionElements.eq(0));

        optionElements = element.find('option');
        expect(optionElements.length).toEqual(3);
        expect(scope.obj.value).toBe(A);

        scope.options.shift();
        scope.$digest();

        optionElements = element.find('option');
        expect(optionElements.length).toEqual(3);
        expect(scope.obj.value).toBe(null);
      });
@Narretz

This comment has been minimized.

Contributor

Narretz commented May 18, 2016

Have you tried if that works with regular interpolated options? Because I don't think it does

@petebacondarwin

This comment has been minimized.

Member

petebacondarwin commented May 19, 2016

It certainly works with ngOptions, which is what we are trying to emulate, right?

@Narretz

This comment has been minimized.

Contributor

Narretz commented May 24, 2016

@petebacondarwin I've updated the PR again with the changes we talked about (support for disabled options, and model updates when the options change / are removed / get disabled / get added).

handleMultipleChanges = false;
self.ngModelCtrl.$setViewValue(self.readValue());
if (renderAfter) self.ngModelCtrl.$render();
});

This comment has been minimized.

@petebacondarwin

petebacondarwin May 27, 2016

Member

This pattern of running something once in the postDigest keeps appearing. I wonder if we should abstract it out into $scope?

@petebacondarwin

This comment has been minimized.

Member

petebacondarwin commented May 27, 2016

I think you might have nailed it but it is the kind of change that I want to spend some time thinking about corner cases...

@googlebot

This comment has been minimized.

googlebot commented Jun 20, 2016

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
@googlebot

This comment has been minimized.

googlebot commented Jun 20, 2016

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.

@googlebot googlebot added cla: no and removed cla: yes labels Jun 20, 2016

@petebacondarwin

This comment has been minimized.

Member

petebacondarwin commented Jun 21, 2016

I did author some of these commits and I am OK with that :-)

@@ -14,10 +14,15 @@ var SelectController =
['$element', '$scope', function($element, $scope) {
var self = this,
optionsMap = new HashMap();
optionsMap = new HashMap(),
handleMultipleDestroy = false; // Flag to run an update to the model after selected options

This comment has been minimized.

@petebacondarwin

petebacondarwin Jun 22, 2016

Member

no longer needed?

@Narretz

This comment has been minimized.

Contributor

Narretz commented Jun 23, 2016

I'll change the tests and then add some docs. Should I mentiond that performance in IE is going to be very bad with 3000+ options?

@petebacondarwin

This comment has been minimized.

Member

petebacondarwin commented Jun 23, 2016

Why not

@Narretz

This comment has been minimized.

Contributor

Narretz commented Jun 29, 2016

While fixing up the PR, I found a behavior in ngOptions and select that I find a bit confusing: You have a select that is multiple and the model contains two values, one of which is matched by an option, while the other isn't.
What happens is that the matching option gets selected, and the model keeps both values. Imo, I would expect no option to be selected, as the model isn't completely matched by the select options. Thoughts?

@Narretz

This comment has been minimized.

Contributor

Narretz commented Jun 29, 2016

So in #13172 we decided that if no option at all can be selected, then the select should be invalid (required).

Doesn't say anything about a partial mismatch, though. We could decide not to select anything when not all options are matched, but I'm not convinced that this is a good idea.

Theoretically, setting the model to match the available options is the developers responsibility, so if that happens it's your own fault. However, it's not very easy to check if a select's options will match the model. We could expose the selectCtrl.hasOption function and make it work with ngOptions, so that users could manually check if a specific $viewValue is matched by their select, see (#12915)

@Narretz

This comment has been minimized.

Contributor

Narretz commented Jun 30, 2016

I'm also fine with making a decision on this later. As far as I can tell, the described behavior hasn't bothered anyone yet.
@petebacondarwin I need your perf commit as part of the commit that handles multiple selects correctly. Because otherwise, the current behavior doesn't work as expected. If the model has two values set, but only one is matched by options, then without the "perf" fix, the option will set the model to the one value only. Delaying the update after render handles this.

Narretz and others added some commits Feb 6, 2016

feat(select): support values of any type added with ngValue
select elements with ngModel will now set ngModel to option values added by ngValue.
This allows setting values of any type (not only strings) without the use of ngOptions.

Interpolations inside attributes can only be strings, but the ngValue directive uses attrs.$set,
which does not have any type restriction. Any $observe on the value attribute will therefore receive
the original value (result of ngValue expression). However, when a user selects an option, the browser
sets the select value to the actual option's value attribute, which is still always a string.
For that reason, when options are added by ngValue, we set the hashed value of the original value in
the value attribute and store the actual value in an extra map. When the select value changes, we
read access the actual value via the hashed select value.

Since we only use a hashed value for ngValue, we will have extra checks for the hashed values:
- when options are read, for both single and multiple select
- when options are written, for multiple select

I don't expect this to have a performance impact, but it should be kept in mind.

Closes #9842
Closes #6297

BREAKING CHANGE:

`<option>` elements added to `<select ng-model>` via `ngValue` now add their values in hash form, i.e.
`<option ng-value="myString">` becomes `<option ng-value="myString" value="string:myString">`.

This is done to support binding options with values of any type to selects.

This should rarely affect applications, as the values of options are usually not relevant to the
application logic, but it's possible that option values are checked in tests.
fix(select): handle model updates when options are manipulated
These rules follow ngOptions behavior:

- when an option that is currently selected, is removed or its value changes, the model
is set to null.
- when an an option is added or its value changes to match the currently selected model,
this option is selected.
- when an option is disabled, the model is set to null.
- when the model value changes to a value that matches a disabled option,
this option is selected (analogue to ngOptions)

@Narretz Narretz merged commit 7b50f49 into angular:master Jul 1, 2016

0 of 3 checks passed

cla/google CLAs are signed, but unable to verify author consent
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
@gkalpak

This comment has been minimized.

Member

gkalpak commented Jul 1, 2016

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment