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

Commit

Permalink
perf(autocomplete): Use virtual repeat...
Browse files Browse the repository at this point in the history 
...and some other optimizations

Closes #3733.
  • Loading branch information
@kseamon @ThomasBurleson
kseamon authored and ThomasBurleson committed Aug 24, 2015
1 parent 391cff5 commit f817193
Show file tree
Hide file tree
Showing 12 changed files with 141 additions and 141 deletions.
2 changes: 1 addition & 1 deletion src/components/autocomplete/autocomplete-theme.scss
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ md-autocomplete.md-THEME_NAME-theme {
}
}
}
.md-autocomplete-suggestions.md-THEME_NAME-theme {
.md-autocomplete-suggestions-container.md-THEME_NAME-theme, {
background: '{{background-50}}';
li {
color: '{{background-900}}';
Expand Down
3 changes: 2 additions & 1 deletion src/components/autocomplete/autocomplete.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,6 @@
*/
angular.module('material.components.autocomplete', [
'material.core',
'material.components.icon'
'material.components.icon',
'material.components.virtualRepeat'
]);
28 changes: 11 additions & 17 deletions src/components/autocomplete/autocomplete.scss
View file
Original file line number Diff line number Diff line change
Expand Up @@ -170,14 +170,18 @@ md-autocomplete {
}
}
}
.md-autocomplete-suggestions {

.md-virtual-repeat-container.md-autocomplete-suggestions-container {
position: absolute;
box-shadow: 0 2px 5px rgba(black, 0.25);
height: 41px * 5.5;
max-height: 41px * 5.5;
z-index: $z-index-tooltip;
}
.md-autocomplete-suggestions {
margin: 0;
list-style: none;
padding: 0;
overflow: auto;
max-height: 41px * 5.5;
z-index: $z-index-tooltip;
li {
cursor: pointer;
font-size: 14px;
Expand All @@ -189,23 +193,13 @@ md-autocomplete {
margin: 0;
white-space: nowrap;
text-overflow: ellipsis;
&.ng-enter,
&.ng-hide-remove {
transition: none;
animation: md-autocomplete-list-in 0.2s;
}
&.ng-leave,
&.ng-hide-add {
transition: none;
animation: md-autocomplete-list-out 0.2s;
}

&:focus {
outline: none;
}

outline: none;
}
}
}

@media screen and (-ms-high-contrast: active) {
md-autocomplete,
.md-autocomplete-suggestions {
Expand Down
28 changes: 24 additions & 4 deletions src/components/autocomplete/autocomplete.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,21 @@ describe('<md-autocomplete>', function () {
};
}

function waitForVirtualRepeat(element) {
// Because the autocomplete does not make the suggestions menu visible
// off the bat, the virtual repeat needs a couple more iterations to
// figure out how tall it is and then how tall the repeated items are.

// Using md-item-size would reduce this to a single flush, but given that
// autocomplete allows for custom row templates, it's better to measure
// rather than assuming a given size.
inject(function ($$rAF) {
$$rAF.flush();
element.scope().$apply();
$$rAF.flush();
});
}

describe('basic functionality', function () {
it('should update selected item and search text', inject(function ($timeout, $mdConstant) {
var scope = createScope();
Expand All @@ -57,9 +72,11 @@ describe('<md-autocomplete>', function () {

element.scope().searchText = 'fo';
$timeout.flush();
waitForVirtualRepeat(element);

expect(scope.searchText).toBe('fo');
expect(scope.match(scope.searchText).length).toBe(1);

expect(ul.find('li').length).toBe(1);

ctrl.keydown(keydownEvent($mdConstant.KEY_CODE.DOWN_ARROW));
Expand Down Expand Up @@ -165,6 +182,7 @@ describe('<md-autocomplete>', function () {

element.scope().searchText = 'fo';
$timeout.flush();
waitForVirtualRepeat(element);

expect(scope.searchText).toBe('fo');
expect(scope.match(scope.searchText).length).toBe(1);
Expand All @@ -182,9 +200,9 @@ describe('<md-autocomplete>', function () {
});

describe('xss prevention', function () {
it('should not allow html to slip through', function () {
var html = 'foo <img src="img" onerror="alert(1)" />';
var scope = createScope([ { display: html } ]);
it('should not allow html to slip through', inject(function($timeout) {
var html = 'foo <img src="img" onerror="alert(1)" />';
var scope = createScope([ { display: html } ]);
var template = '\
<md-autocomplete\
md-selected-item="selectedItem"\
Expand All @@ -202,14 +220,16 @@ describe('<md-autocomplete>', function () {
expect(scope.selectedItem).toBe(null);

scope.$apply('searchText = "fo"');
$timeout.flush();
waitForVirtualRepeat(element);

expect(scope.searchText).toBe('fo');
expect(scope.match(scope.searchText).length).toBe(1);
expect(ul.find('li').length).toBe(1);
expect(ul.find('li').find('img').length).toBe(0);

element.remove();
});
}));
});

describe('API access', function () {
Expand Down
2 changes: 1 addition & 1 deletion src/components/autocomplete/demoBasicUsage/index.html
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<div ng-controller="DemoCtrl as ctrl" layout="column">
<md-content class="md-padding">
<form ng-submit="$event.preventDefault()">
<p>Use <code>md-autocomplete</code> to search for matches from local or remote data sources.</p>
<p>Use <code>md-autocomplete</code> to search for matches from local or remote data sources.</p>
<md-autocomplete
ng-disabled="ctrl.isDisabled"
md-no-cache="ctrl.noCache"
Expand Down
57 changes: 35 additions & 22 deletions src/components/autocomplete/js/autocompleteController.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ function MdAutocompleteCtrl ($scope, $element, $mdUtil, $mdConstant, $mdTheming,
ctrl.getCurrentDisplayValue = getCurrentDisplayValue;
ctrl.registerSelectedItemWatcher = registerSelectedItemWatcher;
ctrl.unregisterSelectedItemWatcher = unregisterSelectedItemWatcher;
ctrl.notFoundVisible = notFoundVisible;

return init();

Expand Down Expand Up @@ -94,19 +95,20 @@ function MdAutocompleteCtrl ($scope, $element, $mdUtil, $mdConstant, $mdTheming,
styles.bottom = 'auto';
styles.maxHeight = Math.min(MAX_HEIGHT, root.bottom - hrect.bottom - MENU_PADDING) + 'px';
}
elements.$.ul.css(styles);

elements.$.scrollContainer.css(styles);
$mdUtil.nextTick(correctHorizontalAlignment, false);

/**
* Makes sure that the menu doesn't go off of the screen on either side.
*/
function correctHorizontalAlignment () {
var dropdown = elements.ul.getBoundingClientRect(),
var dropdown = elements.scrollContainer.getBoundingClientRect(),
styles = {};
if (dropdown.right > root.right - MENU_PADDING) {
styles.left = (hrect.right - dropdown.width) + 'px';
}
elements.$.ul.css(styles);
elements.$.scrollContainer.css(styles);
}
}

Expand All @@ -115,10 +117,10 @@ function MdAutocompleteCtrl ($scope, $element, $mdUtil, $mdConstant, $mdTheming,
*/
function moveDropdown () {
if (!elements.$.root.length) return;
$mdTheming(elements.$.ul);
elements.$.ul.detach();
elements.$.root.append(elements.$.ul);
if ($animate.pin) $animate.pin(elements.$.ul, $rootElement);
$mdTheming(elements.$.scrollContainer);
elements.$.scrollContainer.detach();
elements.$.root.append(elements.$.scrollContainer);
if ($animate.pin) $animate.pin(elements.$.scrollContainer, $rootElement);
}

/**
Expand Down Expand Up @@ -146,18 +148,20 @@ function MdAutocompleteCtrl ($scope, $element, $mdUtil, $mdConstant, $mdTheming,
*/
function cleanup () {
angular.element($window).off('resize', positionDropdown);
elements.$.ul.remove();
elements.$.scrollContainer.remove();
}

/**
* Gathers all of the elements needed for this controller
*/
function gatherElements () {
elements = {
main: $element[ 0 ],
ul: $element.find('ul')[ 0 ],
input: $element.find('input')[ 0 ],
wrap: $element.find('md-autocomplete-wrap')[ 0 ],
elements = {
main: $element[0],
scrollContainer: $element[0].getElementsByClassName('md-virtual-repeat-container')[0],
scroller: $element[0].getElementsByClassName('md-virtual-repeat-scroller')[0],
ul: $element.find('ul')[0],
input: $element.find('input')[0],
wrap: $element.find('md-autocomplete-wrap')[0],
root: document.body
};
elements.li = elements.ul.getElementsByTagName('li');
Expand Down Expand Up @@ -567,18 +571,27 @@ function MdAutocompleteCtrl ($scope, $element, $mdUtil, $mdConstant, $mdTheming,
* Makes sure that the focused element is within view.
*/
function updateScroll () {
if (!elements.li[ ctrl.index ]) return;
var li = elements.li[ ctrl.index ],
top = li.offsetTop,
bot = top + li.offsetHeight,
hgt = elements.ul.clientHeight;
if (top < elements.ul.scrollTop) {
elements.ul.scrollTop = top;
} else if (bot > elements.ul.scrollTop + hgt) {
elements.ul.scrollTop = bot - hgt;
if (!elements.li[0]) return;
var height = elements.li[0].offsetHeight,
top = height * ctrl.index,
bot = top + height,
hgt = elements.scroller.clientHeight,
scrollTop = elements.scroller.scrollTop;
if (top < scrollTop) {
scrollTo(top);
} else if (bot > scrollTop + hgt) {
scrollTo(bot - hgt);
}
}

function scrollTo (offset) {
elements.$.scrollContainer.controller('mdVirtualRepeatContainer').scrollTo(offset);
}

function notFoundVisible () {
return !ctrl.matches.length && !ctrl.loading && ctrl.scope.searchText;
}

/**
* Starts the query to gather the results for the current searchText. Attempts to return cached
* results first, then forwards the process to `fetchResults` if necessary.
Expand Down
53 changes: 28 additions & 25 deletions src/components/autocomplete/js/autocompleteDirective.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -155,21 +155,26 @@ function MdAutocomplete () {
<md-progress-linear\
ng-if="$mdAutocompleteCtrl.loading && !$mdAutocompleteCtrl.hidden"\
md-mode="indeterminate"></md-progress-linear>\
<ul role="presentation"\
class="md-autocomplete-suggestions md-whiteframe-z1 {{menuClass || \'\'}}"\
id="ul-{{$mdAutocompleteCtrl.id}}"\
ng-hide="$mdAutocompleteCtrl.hidden"\
ng-mouseenter="$mdAutocompleteCtrl.listEnter()"\
ng-mouseleave="$mdAutocompleteCtrl.listLeave()"\
ng-mouseup="$mdAutocompleteCtrl.mouseUp()">\
<li ng-repeat="(index, item) in $mdAutocompleteCtrl.matches"\
ng-class="{ selected: index === $mdAutocompleteCtrl.index }"\
ng-click="$mdAutocompleteCtrl.select(index)"\
md-autocomplete-list-item="$mdAutocompleteCtrl.itemName">\
' + itemTemplate + '\
</li>\
' + noItemsTemplate + '\
</ul>\
<md-virtual-repeat-container\
md-auto-shrink\
md-auto-shrink-min="1"\
ng-hide="$mdAutocompleteCtrl.hidden && !$mdAutocompleteCtrl.notFoundVisible()"\
class="md-autocomplete-suggestions-container md-whiteframe-z1"\
role="presentation">\
<ul class="md-autocomplete-suggestions"\
ng-class="::menuClass"\
id="ul-{{$mdAutocompleteCtrl.id}}"\
ng-mouseenter="$mdAutocompleteCtrl.listEnter()"\
ng-mouseleave="$mdAutocompleteCtrl.listLeave()"\
ng-mouseup="$mdAutocompleteCtrl.mouseUp()">\
<li md-virtual-repeat="item in $mdAutocompleteCtrl.matches"\
ng-class="{ selected: $index === $mdAutocompleteCtrl.index }"\
ng-click="$mdAutocompleteCtrl.select($index)"\
md-extra-name="$mdAutocompleteCtrl.itemName">\
' + itemTemplate + '\
</li>' + noItemsTemplate + '\
</ul>\
</md-virtual-repeat-container>\
</md-autocomplete-wrap>\
<aria-status\
class="md-visually-hidden"\
Expand All @@ -178,21 +183,19 @@ function MdAutocomplete () {
<p ng-repeat="message in $mdAutocompleteCtrl.messages track by $index" ng-if="message">{{message}}</p>\
</aria-status>';

function getItemTemplate () {
var templateTag = element.find('md-item-template').remove(),
html = templateTag.length ? templateTag.html() : element.html();
function getItemTemplate() {
var templateTag = element.find('md-item-template').detach(),
html = templateTag.length ? templateTag.html() : element.html();
if (!templateTag.length) element.empty();
return html;
}

function getNoItemsTemplate () {
var templateTag = element.find('md-not-found').remove(),
template = templateTag.length ? templateTag.html() : '';
function getNoItemsTemplate() {
var templateTag = element.find('md-not-found').detach(),
template = templateTag.length ? templateTag.html() : '';
return template
? '<li ng-if="!$mdAutocompleteCtrl.matches.length && !$mdAutocompleteCtrl.loading\
&& !$mdAutocompleteCtrl.hidden"\
ng-hide="$mdAutocompleteCtrl.hidden"\
md-autocomplete-parent-scope>' + template + '</li>'
? '<li ng-if="$mdAutocompleteCtrl.notFoundVisible()"\
md-autocomplete-parent-scope>' + template + '</li>'
: '';

}
Expand Down
Loading

17 comments on commit f817193

@kuhnroyal
Copy link

Choose a reason for hiding this comment

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

The md-virtual-repeat-container is not being removed after an item has been selected.

@topherfangio
Copy link
Contributor

Choose a reason for hiding this comment

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

This also appears to have caused some other issues with the chips autocomplete (which is used for the Contact Chips example).

Basically, the content doesn't look like it's being bound properly because the virtual repeat appears and you can scroll through and select one, but the email/picture is not appearing (in the dom or visually).

I will open an issue and reference this commit.

@lmadeira
Copy link

Choose a reason for hiding this comment

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

md-highlight-text does not work anymore on md-item-template

@houmark
Copy link

Choose a reason for hiding this comment

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

@topherfangio We have a custom chip directive that works much like the contact chips and they also render empty in the list, so I can confirm that behaviour. The demo of the contact chips also confirms this issue. Hope a fix is in the making soon.

Did you open an issue that I missed?

@topherfangio
Copy link
Contributor

Choose a reason for hiding this comment

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

@houmark I didn't make an issue yet, however, I have pushed a branch which has a "fix" for it. It seems that the virtual repeat contents aren't compiled against the proper scope: https://github.com/angular/material/compare/fix-chips-autocomplete-virtual

However, this uses a scope.$parent.$parent.$parent reference which is a pretty bad practice, so I'm trying to find a better workaround/fix before submitting a PR.

@houmark
Copy link

@houmark houmark commented on f817193 Sep 2, 2015

Choose a reason for hiding this comment

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

I have been testing this out but was unable to make it work with our directive that uses a mix of chips and autocomplete. It might be in our end though, as I also noticed that this also happens in the a normal input field with autocomplete but also in the demos right now so this should probably have some attention before 0.11 is out.

Any news in your end @topherfangio ?

@houmark
Copy link

@houmark houmark commented on f817193 Sep 2, 2015

Choose a reason for hiding this comment

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

By testing I mean I used your branch to see how that changed things.

@topherfangio
Copy link
Contributor

Choose a reason for hiding this comment

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

@houmark We've been closing out some other issues, so I have not had a chance to get back to this. Were you able to get it working on your end?

For reference, my branch only fixes it for the contact chips demo, so if you are using the autocomplete inside the chips, you may run into the exact same issue and have to do something similar for the time being.

@gpopovic
Copy link

Choose a reason for hiding this comment

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

I hope this gets fixed in v.0.11 because currently 0.11 is unusable because of this :(

@houmark
Copy link

@houmark houmark commented on f817193 Sep 4, 2015

Choose a reason for hiding this comment

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

Yeah me too, and @topherfangio we were unable to make it work with our directives so I'm eager to see any generic fixes so we can test if this is compatible with our code (which works without issues with 0.10.1).

@houmark
Copy link

Choose a reason for hiding this comment

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

This seems to be one of the major bugs in 0.11.0 and I'm quite surprised to see this one slip through and 0.11.0 being released with a pretty serious bug in one of the major components. Oh well, beta we say :)

@gpopovic
Copy link

@gpopovic gpopovic commented on f817193 Sep 10, 2015 via email

Choose a reason for hiding this comment

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

@topherfangio
Copy link
Contributor

Choose a reason for hiding this comment

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

@houmark @gpopovic I have a PR which fixes this on the demo site. Any chance you could test it to see if it resolves your issues as well?

Edit: PR is here: #4391

@houmark
Copy link

Choose a reason for hiding this comment

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

I sure will, hopefully later today! I had missed that PR as well.

@topherfangio
Copy link
Contributor

Choose a reason for hiding this comment

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

@houmark Awesome, thanks! Please make sure to ping me when you respond. I sometimes miss e-mails, but I really want to hear your feedback on this one :-)

@houmark
Copy link

Choose a reason for hiding this comment

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

@topherfangio Yeah, sorry for that. I forgot to mention you on the last one. I really hope I get to this one today, but since been a few weeks since I tested the last time with HEAD, I might hit other blockers overall before I can get to this one so...

@houmark
Copy link

Choose a reason for hiding this comment

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

@topherfangio I can confirm this is working with a normal use case for us (with a more or less standard auto complete dropdown that is broken on 0.11), but our custom chips with autocomplete combo is still not working and I'm not completely sure why right now, but I will keep debugging on that one and figure out a workaround. Your PR should definitely go in as it's a major improvement and fixes normal use cases.

I'll update you more if/when I figure it out but please push to get this one in :)

Nice work!

Please sign in to comment.