feat(overlay): detach on outside click #16611

wnvko · 2019-07-26T07:54:04Z

Allow detaching of the overlay via detachOnOutsideClick property in OverlayConfig. If set to true the overlay will determine if user performed outside click. If so overlay will detach itself.

Additionally excludeFromOutsideClick property is added to OverlayConfig. This is an array of HTMLElements. Developer may add elements to this array. If user clicks on any of these elements the click will not be considered outside click.

crisbeto

On top of the code comments, I spent some time testing it against an iOS device and it looks like this approach has the same issue that I ran into when I tried doing this a couple of years ago and which was raised in the design doc: touch devices won't dispatch mouse events when the user is tapping on a non-clickable element. This is a problem because the user can't tap anywhere outside of the overlay, like they can now, to close the element. The recording is from the iOS Simulator from xcode, but I'm seeing the same issue on a device running iOS 12.3.1. Note how tapping anywhere on the body doesn't close it, but it works once I tap on a clickable element like one of the buttons.

crisbeto · 2019-08-07T08:28:52Z

src/cdk/overlay/overlay-config.ts

+  /**
+   * Array of HTML elements clicking on which should not be considered as outside click
+   */
+  excludeFromOutsideClick?: HTMLElement[] = [];


I'm not quite sure what the use case is for this.

This array will contain elements which will be excluded from outside clicks. Let say you have two buttons. Clicking on each one will open a mat-menu. Right now if you click on button one mat-menu will be shown. Then if you click on button two mat-menu will be closed, but second mat-menu will not be shown. As described in this issue.
If we set detachOnOutsideClick for mat-menu to true this will work (as well as set hasBackdrop to false). However in this case when you click on button one mat-menu will be shown. If at this point you click again at button one this will happen:

Document's body will recognize this as outside click and will force mat-menu to close.

Click event will reach the button one. At this point the mat-menu is already closed, so the button will try to open it.

To prevent this from happen we should say to overlay consider clicking on the button as not outside click. And we can achieve this by adding the button to excludeFromOutsideClick array.

Now that detachOnOutsideClick has been removed in favor of having a click stream, is this option still necessary? Consumers of the API can now make the determination of what to do on the click based on what was clicked, right?

I was thinking for the situations where overlayRef has no clue who opened it, and one could not determine in the handler of outsideClickEvents, clicking on which element on the page should or should not detach the overlay.

crisbeto · 2019-08-07T08:31:39Z

src/cdk/overlay/mouse/overlay-mouse-click-dispatcher.ts

+ * on event target and order of overlay opens.
+ */
+@Injectable({providedIn: 'root'})
+export class OverlayMouseClickDispatcher implements OnDestroy {


It looks like this class shares most of its code with the keyboard dispatcher. I think that we should only have one dispatcher that deals with both so we don't have to duplicate all of the code. If we rolled this into the keyboard dispatcher the class name won't make sense anymore, but we can rename it in a backwards-compatible way.

Agreed that, if they share a lot of code, we should combine the two classes. This name also says "Mouse"- does this apply for touch event as well?

@crisbeto and @jelbourn you are right. I was looking at OverlayKeyboardDispatcher when I was implementing OverlayClickDispatcher. However looking at the code if we combine both these in one single class it will be a little awkward. On add OverlayKeyboardDispatcher adds event listener for keydown while OverlayClickDispatcher adds one for click. Then in the event listener function both dispatchers implements different logic. If I put both in one dispatcher it should have a lot of ifs inside. Additionally if in future a need for another dispatcher arise we should add more ifs. This is why I’ve extracted the common logic in base OverlayBaseDispatcher class which both keyboard and click dispatchers extend. What you think about this?

crisbeto · 2019-08-07T08:32:42Z

src/cdk/overlay/mouse/overlay-mouse-click-dispatcher.ts

+        return;
+      } else {
+        if (config.detachOnOutsideClick) {
+          overlayRef.detach();


Having this call happen here feel a little weird, because all of the other similar functionality happens inside the OverlayRef. I think that we should follow the same pattern where the dispatcher just emits some kind of event and then it's up to the overlay to decide what it wants to do based on its options.

Agreed, it should be outside the responsibility of the event manager to actually dismiss the overlay; the overlay should instead respond to a signal from the keyboard manager.

I've fixed this. Now new OverlayClickDispatcher fires _clickEvents. Anyone interested should subscribe to the public clickEvents observable in the overlay ref.

wnvko · 2019-09-25T11:30:47Z

On top of the code comments, I spent some time testing it against an iOS device and it looks like this approach has the same issue that I ran into when I tried doing this a couple of years ago and which was raised in the design doc: touch devices won't dispatch mouse events when the user is tapping on a non-clickable element. This is a problem because the user can't tap anywhere outside of the overlay, like they can now, to close the element. The recording is from the iOS Simulator from xcode, but I'm seeing the same issue on a device running iOS 12.3.1. Note how tapping anywhere on the body doesn't close it, but it works once I tap on a clickable element like one of the buttons.

I can solve this by forcing the documentElement to be "clickable" under iOS. This could be achieved by adding onclick attribute or by setting the cursor style to pointer. I can check if the OS is iOS and set the cursor. Of course, when all the overlays get closed I will restore the original cursor style. What do you think about such a fix?

crisbeto · 2019-09-28T11:09:51Z

I don't know whether this is only an issue on iOS or whether we'd need to do this for all touch devices. If we end up having to do it for all, it could look weird on hybrid devices that both have a touch screen and a mouse. What do you think @jelbourn?

jelbourn · 2019-12-03T16:10:31Z

Yes, we would definitely have to confirm that this works for all touch devices. I have a pixelbook I can test if you make the change. I would go with adding a no-op event listener to the document outside the zone if that makes the browser see all of the content as "clickable". The listener should only be applied while the overlay is open.

wnvko · 2019-12-06T14:42:56Z

@crisbeto and @jelbourn regarding the iOS issue I am setting now the body's cursor style to pointer when overlayRef is added. As soon as last overlayRef is removed I am restoring original style.
I've tested this on iPad and iPhone both with iOS 12.3.1 and it works.
I also have tested this on Android 9 device, on my PC and on PC with touch screen - it was ok on all these devices.

jelbourn · 2020-01-17T22:17:09Z

I tried this out locally just now- I set detachOnOutsideClick: true in the connected-overlay-demo, but clicking outside the panel doesn't dismiss the overlay. Am I missing something?

wnvko · 2020-01-20T14:28:38Z

I tried this out locally just now- I set detachOnOutsideClick: true in the connected-overlay-demo, but clicking outside the panel doesn't dismiss the overlay. Am I missing something?

Now if detachOnOutsideClick is set to true OverlayRef's clickEvents will dispatch event on outside click. You should add, in connected-overlay-demo in openWithConfig something like this:

    this.overlayRef.clickEvents().subscribe(() => {
      this.close();
    });

In this way this is now working similarly to OverlayKeyboardDispatcher

jelbourn · 2020-01-26T22:08:25Z

I see; I think the naming is currently misleading, then. detachOnOutsideClick would make me think the overlay will automatically do this. It's also not totally clear that clickEvents would include clicks outside the overlay, since the description says "observable of keydown events targeted to this overlay".

Would it make more sense to just have a stream of outsideClicks (similar to backdropClick) that the user can handle in the same manner?

wnvko · 2020-01-27T14:46:05Z

The initial idea was setting the detachOnOutsideClick to automatically do that. Then we switched it to observable, which is better I think. As a result detachOnOutsideClick became obsolete. I will remove it now. I should update and the design doc, should not I?

jelbourn · 2020-01-27T18:24:29Z

That sounds good

jelbourn

Looked though against based on the latest API; mainly just some comments about clarification about exactly what the APIs are doing

jelbourn · 2020-01-29T00:42:48Z