fix(autocomplete): unable to click to select items in IE #3188

crisbeto · 2017-02-19T15:05:31Z

Fixes being unable to select autocomplete items by clicking in IE. This was due to IE not setting the event.relatedTarget for blur events.
Fixes potential issue if the user uses mat-option, instead of md-option.

kara · 2017-02-28T02:57:02Z

src/lib/autocomplete/autocomplete.spec.ts

  let event  = document.createEvent('Event');
+
+  if (extras) {
+    Object.assign(event, extras);


Can you use extendObject from core utils instead?

kara · 2017-02-28T03:21:37Z

src/lib/autocomplete/autocomplete-trigger.ts

-    // Only emit blur event if the new focus is *not* on an option.
-    if (newlyFocusedTag !== 'MD-OPTION') {
+    // Only emit blur event if the new focus is *not* on an element inside the panel.
+    if (relatedTarget && !this._overlayRef.overlayElement.contains(relatedTarget)) {


This appears to break clicking outside the panel to close it in Chrome and Firefox. Can you fix?

crisbeto · 2017-02-28T19:08:19Z

Addressed the feedback @kara.

kara · 2017-02-28T19:47:10Z

@crisbeto Closing on blur still appears to be broken on Firefox. Have you tried just expanding the old check to include MAT-OPTION?

crisbeto · 2017-02-28T19:48:31Z

The issue isn't with the node name, I'm testing it against our demo app that uses only md-option. It breaks because IE doesn't assign the relatedTarget.

kara · 2017-02-28T19:49:42Z

Ok. Can you fix Firefox?

crisbeto · 2017-02-28T19:51:09Z

I will, although it may be worth rethinking the approach since the relatedTarget seems to be pretty brittle.

crisbeto · 2017-03-01T21:24:08Z

@kara could you give this another look?

Only1MrAnderson · 2017-03-16T17:45:45Z

Any update on merging this?

donduni · 2017-04-10T14:46:38Z

Is there any plan to merge this one?

kara

Sorry for the late review on this. Seems to be working great on both Firefox and Chrome now, but I think it needs a rebase.

kara · 2017-04-11T18:13:45Z

src/lib/autocomplete/autocomplete-trigger.ts

+            this._outsideClickStream.next(null);
+          }
+        });
+    }


openPanel is getting a bit heavy. Can you move this new block into its own method?

kara · 2017-04-11T18:15:37Z

src/lib/autocomplete/autocomplete-trigger.ts

+        (event: MouseEvent) => {
+          if (!this._inputContainer._elementRef.nativeElement.contains(event.target) &&
+              !this._overlayRef.overlayElement.contains(event.target as HTMLElement)) {
+            this._outsideClickStream.next(null);


Have you thought about using Observable.fromEvent here? I think it would be cleaner, because then we don't have the maintain the subscription or manually emit (we can just pass it to panelClosingActions).

get outsideClickStream() { return Observable.fromEvent(this._document, 'click') .filter(event => // check if contains target); } get panelClosingActions() { ... this.outsideClickStream }

IIRC, I didn't go with fromEvent, because it may break server-side rendering. Thinking about it again, it should be fine since the menu shows up as a result from a user action.

I see. Yeah, had the same train of thought. I think it will be fine given that it's not on initial render.

crisbeto · 2017-04-12T18:23:14Z

Fixed the test failures, addressed the feedback and rebased @kara.

kara

A few more nits, then should be good to go.

kara · 2017-04-12T18:32:42Z

src/lib/autocomplete/autocomplete-trigger.ts

@@ -100,9 +102,11 @@ export class MdAutocompleteTrigger implements ControlValueAccessor, OnDestroy {
  }

  constructor(private _element: ElementRef, private _overlay: Overlay,
-              private _viewContainerRef: ViewContainerRef,
+              private _renderer: Renderer, private _viewContainerRef: ViewContainerRef,


Looks like you're not using the renderer anymore. Remove?

kara · 2017-04-12T18:33:07Z

src/lib/autocomplete/autocomplete-trigger.ts

@@ -144,6 +148,7 @@ export class MdAutocompleteTrigger implements ControlValueAccessor, OnDestroy {

    this._panelOpen = false;
    this._resetPlaceholder();
+    this._changeDetectorRef.detectChanges();


Curious why this is necessary. Add comment?

I'll add a comment. It seems like using fromEvent doesn't trigger change detection, or at least it doesn't trigger it at the proper time. This means that the placeholder doesn't get reset when you click on the backdrop.

Makes sense. Just for posterity, would be good to have that documented.

kara · 2017-04-12T18:36:56Z

src/lib/autocomplete/autocomplete-trigger.ts

@@ -170,6 +175,17 @@ export class MdAutocompleteTrigger implements ControlValueAccessor, OnDestroy {
    }
  }

+  get outsideClickStream(): Observable<any> {


Hmm let's keep this private? I think I may have omitted the _ in my comment above by mistake, but I liked that you originally had it private.

crisbeto · 2017-04-12T18:51:25Z

Addressed the last set of comments @kara.

kara

LGTM. Found a typo, so apply merge label whenever you're ready.

kara · 2017-04-12T19:47:32Z

src/lib/autocomplete/autocomplete-trigger.ts

@@ -170,6 +179,18 @@ export class MdAutocompleteTrigger implements ControlValueAccessor, OnDestroy {
    }
  }

+  /** Stream of click outside of the autocomplete panel. */


Typo: clicks

* Fixes being unable to select autocomplete items by clicking in IE. This was due to IE not setting the event.relatedTarget for blur events. * Fixes potential issue if the user uses mat-option, instead of md-option. Fixes angular#3351.

angular-automatic-lock-bot · 2019-09-06T11:02:59Z

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.}

crisbeto added the pr: needs review label Feb 19, 2017

crisbeto assigned kara Feb 19, 2017

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Feb 19, 2017

This was referenced Feb 19, 2017

fix(autocomplete): blur not being handled in compatibility mode #3181

Closed

Autocomplete IE 11 Compatibility #3255

Closed

crisbeto force-pushed the ie-autocomplete-click branch from 3193bcd to b028856 Compare February 23, 2017 18:31

kara requested changes Feb 28, 2017

View reviewed changes

kara added pr: needs cleanup / tests and removed pr: needs review labels Feb 28, 2017

kara assigned crisbeto and unassigned kara Feb 28, 2017

crisbeto force-pushed the ie-autocomplete-click branch from b028856 to ba31165 Compare February 28, 2017 18:59

crisbeto added pr: needs review and removed pr: needs cleanup / tests labels Feb 28, 2017

crisbeto assigned kara and unassigned crisbeto Feb 28, 2017

kara assigned crisbeto and unassigned kara Feb 28, 2017

kara added the pr: needs cleanup / tests label Feb 28, 2017

crisbeto mentioned this pull request Mar 1, 2017

Bug - Can't select an option in autocomplete IE 11 #3351

Closed

crisbeto removed the pr: needs cleanup / tests label Mar 1, 2017

crisbeto assigned kara and unassigned crisbeto Mar 1, 2017

kara requested changes Apr 11, 2017

View reviewed changes

kara added the pr: needs rebase label Apr 11, 2017

kara assigned crisbeto and unassigned kara Apr 11, 2017

crisbeto force-pushed the ie-autocomplete-click branch from 83a5673 to aa8cfa6 Compare April 12, 2017 18:14

crisbeto removed the pr: needs rebase label Apr 12, 2017

crisbeto assigned crisbeto and kara and unassigned crisbeto Apr 12, 2017

kara reviewed Apr 12, 2017

View reviewed changes

crisbeto force-pushed the ie-autocomplete-click branch 2 times, most recently from 56ac940 to b6ac2a9 Compare April 12, 2017 18:51

kara approved these changes Apr 12, 2017

View reviewed changes

kara added pr: lgtm and removed pr: needs review labels Apr 12, 2017

kara assigned crisbeto and unassigned kara Apr 12, 2017

crisbeto force-pushed the ie-autocomplete-click branch from b6ac2a9 to eeca900 Compare April 12, 2017 20:12

crisbeto added the action: merge The PR is ready for merge by the caretaker label Apr 12, 2017

kara merged commit 78985b7 into angular:master Apr 18, 2017

crisbeto mentioned this pull request Apr 25, 2017

[MdAutocomplete] cant select items in IE11 #4258

Closed

This was referenced May 12, 2017

Autocomplete option not clickable #4465

Closed

Autocomplete throws error in 2.0.0-beta.5 #4555

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(autocomplete): unable to click to select items in IE #3188

fix(autocomplete): unable to click to select items in IE #3188

crisbeto commented Feb 19, 2017 •

edited

Loading

kara Feb 28, 2017

kara Feb 28, 2017

crisbeto commented Feb 28, 2017

kara commented Feb 28, 2017

crisbeto commented Feb 28, 2017

kara commented Feb 28, 2017

crisbeto commented Feb 28, 2017

crisbeto commented Mar 1, 2017

Only1MrAnderson commented Mar 16, 2017

donduni commented Apr 10, 2017

kara left a comment

kara Apr 11, 2017

kara Apr 11, 2017

crisbeto Apr 11, 2017

kara Apr 11, 2017

crisbeto commented Apr 12, 2017

kara left a comment

kara Apr 12, 2017

kara Apr 12, 2017

crisbeto Apr 12, 2017

kara Apr 12, 2017

kara Apr 12, 2017

crisbeto commented Apr 12, 2017

kara left a comment

kara Apr 12, 2017 •

edited

Loading

angular-automatic-lock-bot bot commented Sep 6, 2019

fix(autocomplete): unable to click to select items in IE #3188

fix(autocomplete): unable to click to select items in IE #3188

Conversation

crisbeto commented Feb 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisbeto commented Feb 28, 2017

kara commented Feb 28, 2017

crisbeto commented Feb 28, 2017

kara commented Feb 28, 2017

crisbeto commented Feb 28, 2017

crisbeto commented Mar 1, 2017

Only1MrAnderson commented Mar 16, 2017

donduni commented Apr 10, 2017

kara left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisbeto commented Apr 12, 2017

kara left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisbeto commented Apr 12, 2017

kara left a comment

Choose a reason for hiding this comment

kara Apr 12, 2017 • edited Loading

Choose a reason for hiding this comment

angular-automatic-lock-bot bot commented Sep 6, 2019

crisbeto commented Feb 19, 2017 •

edited

Loading

kara Apr 12, 2017 •

edited

Loading