Skip to content
This repository has been archived by the owner on Sep 5, 2024. It is now read-only.

Commit

Permalink
fix(select): fix bugs introduced by keeping md-select-menu in DOM. Fi…
Browse files Browse the repository at this point in the history
…x tests

Closes #6071
  • Loading branch information
rschmukler authored and jelbourn committed Dec 7, 2015
1 parent ef05ea3 commit 7edda11
Show file tree
Hide file tree
Showing 2 changed files with 75 additions and 96 deletions.
25 changes: 17 additions & 8 deletions src/components/select/select.js
Original file line number Diff line number Diff line change
Expand Up @@ -418,6 +418,7 @@ function SelectDirective($mdSelect, $mdUtil, $mdTheming, $mdAria, $compile, $par
selectContainer[0].setAttribute('class', value);
}
selectMenuCtrl = selectContainer.find('md-select-menu').controller('mdSelectMenu');
selectMenuCtrl.init(ngModelCtrl, attr.ngModel);
}

function handleKeypress(e) {
Expand Down Expand Up @@ -452,6 +453,7 @@ function SelectDirective($mdSelect, $mdUtil, $mdTheming, $mdAria, $compile, $par
element: selectContainer,
target: element[0],
preserveElement: true,
parent: element,
hasBackdrop: true,
loadingAsync: attr.mdOnOpen ? scope.$eval(attr.mdOnOpen) || true : false
}).finally(function() {
Expand All @@ -468,7 +470,7 @@ function SelectMenuDirective($parse, $mdUtil, $mdTheming) {

return {
restrict: 'E',
require: ['mdSelectMenu', '^ngModel'],
require: ['mdSelectMenu'],
scope: true,
controller: SelectMenuController,
link: {pre: preLink}
Expand All @@ -478,12 +480,10 @@ function SelectMenuDirective($parse, $mdUtil, $mdTheming) {
// its child options run postLink.
function preLink(scope, element, attr, ctrls) {
var selectCtrl = ctrls[0];
var ngModel = ctrls[1];

$mdTheming(element);
element.on('click', clickListener);
element.on('keypress', keyListener);
if (ngModel) selectCtrl.init(ngModel);

function keyListener(e) {
if (e.keyCode == 13 || e.keyCode == 32) {
Expand Down Expand Up @@ -551,7 +551,7 @@ function SelectMenuDirective($parse, $mdUtil, $mdTheming) {

// watchCollection on the model because by default ngModel only watches the model's
// reference. This allowed the developer to also push and pop from their array.
$scope.$watchCollection($attrs.ngModel, function(value) {
$scope.$watchCollection(self.modelBinding, function(value) {
if (validateArray(value)) renderMultiple(value);
self.ngModel.$setPristine();
});
Expand Down Expand Up @@ -598,8 +598,9 @@ function SelectMenuDirective($parse, $mdUtil, $mdTheming) {
}
};

self.init = function(ngModel) {
self.init = function(ngModel, binding) {
self.ngModel = ngModel;
self.modelBinding = binding;

// Allow users to provide `ng-model="foo" ng-model-options="{trackBy: 'foo.id'}"` so
// that we can properly compare objects set on the model to the available options
Expand Down Expand Up @@ -782,7 +783,15 @@ function OptionDirective($mdButtonInkRipple, $mdUtil) {
$mdButtonInkRipple.attach(scope, element);
configureAria();

function setOptionValue(newValue, oldValue) {
function setOptionValue(newValue, oldValue, prevAttempt) {
if (!selectCtrl.hashGetter) {
if (!prevAttempt) {
scope.$$postDigest(function() {
setOptionValue(newValue, oldValue, true);
});
}
return;
}
var oldHashKey = selectCtrl.hashGetter(oldValue, scope);
var newHashKey = selectCtrl.hashGetter(newValue, scope);

Expand Down Expand Up @@ -1238,7 +1247,7 @@ function SelectProvider($$interimElementProvider) {
* trigger the [optional] user-defined expression
*/
function announceClosed(opts) {
var mdSelect = opts.target.controller('mdSelect');
var mdSelect = opts.selectEl.controller('mdSelect');
if (mdSelect) {
var menuController = opts.selectEl.controller('mdSelectMenu');
mdSelect.setLabelText(menuController.selectedLabels());
Expand All @@ -1253,7 +1262,7 @@ function SelectProvider($$interimElementProvider) {
function calculateMenuPositions(scope, element, opts) {
var
containerNode = element[0],
targetNode = opts.target[0].children[0], // target the label
targetNode = opts.target[0].children[1], // target the label
parentNode = $document[0].body,
selectNode = opts.selectEl[0],
contentNode = opts.contentEl[0],
Expand Down
Loading

2 comments on commit 7edda11

@mckenzielong
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rschmukler this might have re-introduced #6023 and #6030.

@rschmukler
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mckenzielong indeed, thanks for keeping up with it! I actually didn't intend for this to get merged as it was. I'll be reverting some of the changes in an upcoming commit. Thanks!

Please sign in to comment.