fix(autocomlete): Gets rid of uncompiled content flicker.

[kseamon]

Works around issues cased by the scope discontinuity in autocompleteParentScopeDirective.
Fixes #5188

[kseamon]

@robertmesserle @jelbourn @ThomasBurleson

This will need unit tests, but wanted to show it off for now. It's a hack to fix a hack. Before I spend any more time on this, we should consider whether MdAutocompleteItemScopeDirective really needs to exist in the first place.

[topherfangio]

@kseamon What would you suggest in place of the MdAutocompleteItemScopeDirective?

[kseamon]

If I understand correctly, all it is doing is providing encapsulation by linking it's contains against the parent scope of the autocomplete. The fact that the demo works fine with this directive commented out is evidence that we don't really need it.

An alternative would be to mark autocomplete 'a scope vars as private with $$ and leave the autocomplete contents in normal child scopes.

On Nov 9, 2015, at 5:18 PM, Topher Fangio notifications@github.com wrote:

@kseamon What would you suggest in place of the MdAutocompleteItemScopeDirective?

—
Reply to this email directly or view it on GitHub.

[topherfangio]

@kseamon It does indeed work on the standard autocomplete, but it breaks with custom attributes on the item scope. Specifically, it breaks the Contact Chips component/demo and anything else that has custom scope properties.

See #4390 and #4495 for more info on the issues and commit 6681e82 which adds the item scope directive and fixes these.

[topherfangio]

All that said, this PR does indeed appear to fix the flicker issue and still allow the Contact Chips demo to work, so it looks like a nice change!

@kseamon You mentioned that you wanted to add some tests; I'm just curious: are you planning to test the flicker? If so, that would be a pretty slick test :-)

[kseamon]

I'll be testing the scope behavior, which is the cause of the flicker, not the flicker itself.

[kseamon]

Related question: Is the distinction between having md-autocomplete-replace and not important? With the change to using transclusion, it will in effect always be replacing with this change. If need be, I could revert that part, but because it removes repeated calls to compile, the new way is faster.

[kseamon]

Added the test.

[topherfangio]

@robertmesserle Can you respond to #5637 (comment) ? I'm not entirely sure how the md-autocomplete-replace is used.

[robertmesserle]

@topherfangio Didn't you add that? (blame claims that you did)

[topherfangio]

@robertmesserle I pulled it over from the old parentScope.js file when I renamed it, but it was originally added in one of your commits: 1f0a845#diff-78c9b5c0c4b866fa43743791d73c84ffR15

[robertmesserle]

@topherfangio Ah, okay. This is required for ngMessages to work properly. Is md-autocomplete-replace causing issues?

[topherfangio]

@ThomasBurleson I'd like to do a tiny bit more testing on whether or not this causes issues with ng-messages before merging.

@robertmesserle Do you recall how this fixed ng-messages? Would it fail if we wrote a simple demo/test case that used ng-messages?

[robertmesserle]

@topherfangio I believe it was related to our implementation of md-input-container where ng-messages had to be a direct child.

[kseamon]

Any updates on this?

[jelbourn]

Need LGTM from @robertmesserle

[robertmesserle]

@jelbourn @kseamon LGTM

[cairomassimo]

Hi. This definitely breaks ng-messages inside md-autocomplete.
Look at this line:
https://github.com/angular/material/blob/master/src/components/autocomplete/js/autocompleteDirective.js#L216
The outer element is not replaced anymore, and md-input-container does not behave correctly.

As a workaround, I added the md-input-message-animation class to all the ng-message elements, and it seems at least to show error messages properly (font-size, auto-hide).

[ThomasBurleson]

@cairomax - please submit a new issue.