Skip to content

refactor(cdk/overlay): remove logic which tracks excluded elements from outside clicks #19887

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 29, 2020
Merged

refactor(cdk/overlay): remove logic which tracks excluded elements from outside clicks #19887

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 20 additions & 12 deletions src/cdk/overlay/dispatchers/overlay-outside-click-dispatcher.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,26 +125,34 @@ describe('OverlayOutsideClickDispatcher', () => {
overlayTwo.dispose();
});

it(`should not dispatch click event when click on element
included in excludeFromOutsideClick array`, () => {
it('should dispatch the click event when click is on an element outside the overlay', () => {
const portal = new ComponentPortal(TestComponent);
const overlayRef = overlay.create();
const spy = jasmine.createSpy('overlay mouse click event spy');
overlayRef.attach(portal);
const button = document.createElement('button');
document.body.appendChild(button);

const spy = jasmine.createSpy('overlay mouse click spy');
overlayRef.outsidePointerEvents().subscribe(spy);

const overlayConfig = overlayRef.getConfig();
expect(overlayConfig.excludeFromOutsideClick).toBeDefined();
expect(overlayConfig.excludeFromOutsideClick!.length).toBe(0);
button.click();
expect(spy).toHaveBeenCalled();

overlayRef.attach(new ComponentPortal(TestComponent));
button.parentNode!.removeChild(button);
overlayRef.dispose();
Copy link

Choose a reason for hiding this comment

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

Should these disposals happen in an afterEach() block, so they happen even if the test fails?

Copy link
Member

Choose a reason for hiding this comment

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

Slightly tricky because different tests create a different number of overlays; it would probably be better to de-dupe the dispose calls, but I wouldn't hold up the PR on it since it was already like this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@teflonwaffles I agree that it should be cleaned up, but I opted to keep with the structure of the existing tests rather than polluting this PR with a test refactor.

});

it('should not dispatch the click event when click is on an element inside the overlay', () => {
const portal = new ComponentPortal(TestComponent);
const overlayRef = overlay.create();
overlayRef.attach(portal);

const buttonShouldNotDetach = document.createElement('button');
document.body.appendChild(buttonShouldNotDetach);
overlayConfig.excludeFromOutsideClick!.push(buttonShouldNotDetach);
buttonShouldNotDetach.click();
const spy = jasmine.createSpy('overlay mouse click event spy');
overlayRef.outsidePointerEvents().subscribe(spy);

overlayRef.overlayElement.click();
expect(spy).not.toHaveBeenCalled();

buttonShouldNotDetach.parentNode!.removeChild(buttonShouldNotDetach);
overlayRef.dispose();
});
});
Expand Down
10 changes: 3 additions & 7 deletions src/cdk/overlay/dispatchers/overlay-outside-click-dispatcher.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,13 +81,9 @@ export class OverlayOutsideClickDispatcher extends BaseOverlayDispatcher {
continue;
}

const config = overlayRef.getConfig();
const excludeElements = [...config.excludeFromOutsideClick!, overlayRef.overlayElement];
const isInsideClick: boolean = excludeElements.some(e => e.contains(target));

// If it is inside click just break - we should do nothing
// If it is outside click dispatch the mouse event, and proceed with the next overlay
if (isInsideClick) {
// If it's a click inside the overlay, just break - we should do nothing
// If it's an outside click dispatch the mouse event, and proceed with the next overlay
if (overlayRef.overlayElement.contains(target as Node)) {
break;
}

Expand Down
5 changes: 0 additions & 5 deletions src/cdk/overlay/overlay-config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,6 @@ export class OverlayConfig {
*/
disposeOnNavigation?: boolean = false;

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

Choose a reason for hiding this comment

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

I think we talked about having this be a predicate function instead of an array of elements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jelbourn we later agreed that we should just emit on the _outsidePointerEvents subject and have the user decide correct?

Copy link
Member

@jelbourn jelbourn Jul 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, when Andy and I followed-up on the discussion in the team meeting, I remembered that the outsidePointerEvents stream is, well, a stream that people can filter themselves however they want, so adding a predication would literally just be adding a call to filter, so calling the API is the same amount of work as adding a filter yourself.

Copy link
Contributor Author

@andy9775 andy9775 Jul 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@jelbourn thinking about it now, adding a predicate can stop overlays added earlier from being processed since the loop would just break based on the predicates return. I can implement the menu with either approach but I'm not sure if there's some edge cases where we may want an early break.

Edit: agreed to leave as is


constructor(config?: OverlayConfig) {
if (config) {
// Use `Iterable` instead of `Array` because TypeScript, as of 3.6.3,
Expand Down
1 change: 0 additions & 1 deletion tools/public_api_guard/cdk/overlay.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,6 @@ export declare class OverlayConfig {
backdropClass?: string | string[];
direction?: Direction | Directionality;
disposeOnNavigation?: boolean;
excludeFromOutsideClick?: HTMLElement[];
hasBackdrop?: boolean;
height?: number | string;
maxHeight?: number | string;
Expand Down