fix(Control): Support <select multiple> with Control class

[erodozer]

• Please check if the PR fulfills these requirements
• The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit-message-format
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)
• What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
  Bug fix for select controls (Support <select multiple> with Control class #4427)
• What is the current behavior? (You can also link to an open issue here)
  Select currently only supports single option selection.
• What is the new behavior (if this is a feature change)?
  Modify change listening to making use of selectOptions on both multiple selects (degrading to iterating over all options for browsers that do not have selectOptions defined, such as IE). Using this read-only property, we're able to always get an array of any and all selected options (on single selects it just returns an array with the single option selected).
• Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
  All options now have their own ID for mapping that's stored, and the Text values output to DOM on options become "id:value" in all cases, not just on [ngValue] for better selected option resolving. The value assigned in to an ngModel, however, is strictly the stored value, so users should not have to change anything with their data. DOM output just might look a little different when inspecting.
• Other information:
  Checks are in place to detect if the select is of type multiple to prevent taking the slow execution option on single selects. Single selects should maintain their current behavior, making use of the value property of HTMLSelectElements. Multiple selects require iterating over the whole list to select and unselect the appropriate options when the model changes and invokes a writeValue on the accessor. It might be worth looking into optimizing this for larger dataset support, possibly keeping track of what the value was before and after the change.

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please let us know the company's name.

[erodozer]

I signed it!

[googlebot]

CLAs look good, thanks!

[googlebot]

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

[googlebot]

CLAs look good, thanks!

[mhevery]

Thanks for your contribution.

• Please fix broken tests
• Please add additional tests for the new use cases.

@kara Can you have a look to see if the approach is consistent with your thiniking?

[erodozer]

If it's decided that it's more ideal to have a single accessor not have two functionally different ways to resolve its value based on an attribute that acts as a flag, I can easily separate out the multiple handling functionality into a separate accessor and make the selectors more specific to prevent collisions.

[kara]

@nhydock Yeah, I think you're right that separating the functionality out into a separate accessor would help simplify the code and ensure there aren't collisions.

It also looks like binding to normal value property is broken, so you may want to address in your re-design: https://travis-ci.org/angular/angular/jobs/123406593#L3007.

[kara]

Can you go through the renderer rather than calling directly? This won't work outside of the browser context.

[erodozer]

Alright, I split it into a separate accessor and altered the selectors according to #6830 to prevent collision.

Also merged _optionElements and _optionMap into a single map on the new accessor. The single select test should be passing again, as all changes to select have been reverted. But now I need to add tests for specifically select multiple.

Also, I was enforcing IDs in value strings regardless of [value] or [ngValue] because I was having an issue where if my options are as follows

<select multiple [(ngModel)]="my_model">
    <option value="3">0</option>
    <option value="4">1</option>
    <option value="5">2</option>
    <option value="6">3</option>
</select>

I would have a problem where when performing _extractId(valueString) it'd find a number which it would think is an ID and it would select the wrong options. By always prepending the ID it prevents this.

[abijr]

Any updates on this?

[mhevery]

if constructor is empty there is no need for it.

[mhevery]

I am sorry about the delay. Could you please rebase on master and get the tests green. Otherwise this looks good to me. @kara may have more comments.

[yusijs]

Anything new here? Seems like the tests are passing :3 (I really want this functionality in rc.2 when it drops.. :3 )

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}