Skip to content
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.

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

feat: make popover modeless by default, add modality properties #7412

Merged
merged 5 commits into from
May 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions packages/popover/src/vaadin-popover-overlay.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@ class PopoverOverlay extends PopoverOverlayMixin(DirMixin(ThemableMixin(PolylitM
return [
overlayStyles,
css`
:host([modeless][with-backdrop]) [part='backdrop'] {
pointer-events: none;
vursen marked this conversation as resolved.
Show resolved Hide resolved
}

:host([position^='top'][top-aligned]) [part='overlay'],
:host([position^='bottom'][top-aligned]) [part='overlay'] {
margin-top: var(--vaadin-popover-offset-top, 0);
Expand Down
16 changes: 15 additions & 1 deletion packages/popover/src/vaadin-popover.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,15 @@ declare class Popover extends PopoverPositionMixin(
*/
renderer: PopoverRenderer | null | undefined;

/**
* When true, the popover prevents interacting with background elements
* by setting `pointer-events` style on the document body to `none`.
* This also enables trapping focus inside the overlay.
*/
modal: boolean;

/**
* Set to true to disable closing popover overlay on outside click.
* Closing on outside click only works when the popover is modal.
*
* @attr {boolean} no-close-on-outside-click
*/
Expand All @@ -50,6 +56,14 @@ declare class Popover extends PopoverPositionMixin(
*/
noCloseOnEsc: boolean;

/**
* When true, the overlay has a backdrop (modality curtain) on top of the
* underlying page content, covering the whole viewport.
*
* @attr {boolean} with-backdrop
*/
withBackdrop: boolean;

/**
* Requests an update for the content of the popover.
* While performing the update, it invokes the renderer passed in the `renderer` property.
Expand Down
51 changes: 50 additions & 1 deletion packages/popover/src/vaadin-popover.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,18 @@ class Popover extends PopoverPositionMixin(
type: Object,
},

/**
* When true, the popover prevents interacting with background elements
* by setting `pointer-events` style on the document body to `none`.
* This also enables trapping focus inside the overlay.
*/
modal: {
type: Boolean,
value: false,
},

/**
* Set to true to disable closing popover overlay on outside click.
* Closing on outside click only works when the popover is modal.
*
* @attr {boolean} no-close-on-outside-click
*/
Expand All @@ -73,6 +82,17 @@ class Popover extends PopoverPositionMixin(
value: false,
},

/**
* When true, the overlay has a backdrop (modality curtain) on top of the
* underlying page content, covering the whole viewport.
*
* @attr {boolean} with-backdrop
*/
withBackdrop: {
type: Boolean,
value: false,
},

/** @private */
_opened: {
type: Boolean,
Expand All @@ -83,6 +103,7 @@ class Popover extends PopoverPositionMixin(

constructor() {
super();
this.__onGlobalClick = this.__onGlobalClick.bind(this);
this.__onTargetClick = this.__onTargetClick.bind(this);
this.__onTargetKeydown = this.__onTargetKeydown.bind(this);
}
Expand All @@ -99,6 +120,9 @@ class Popover extends PopoverPositionMixin(
.positionTarget="${this.target}"
.position="${effectivePosition}"
.opened="${this._opened}"
.modeless="${!this.modal}"
.focusTrap="${this.modal}"
.withBackdrop="${this.withBackdrop}"
?no-horizontal-overlap="${this.__computeNoHorizontalOverlap(effectivePosition)}"
?no-vertical-overlap="${this.__computeNoVerticalOverlap(effectivePosition)}"
.horizontalAlign="${this.__computeHorizontalAlign(effectivePosition)}"
Expand Down Expand Up @@ -131,10 +155,19 @@ class Popover extends PopoverPositionMixin(
this._overlayElement = this.shadowRoot.querySelector('vaadin-popover-overlay');
}

/** @protected */
connectedCallback() {
super.connectedCallback();

document.addEventListener('click', this.__onGlobalClick, true);
}

/** @protected */
disconnectedCallback() {
super.disconnectedCallback();

document.removeEventListener('click', this.__onGlobalClick, true);

this._opened = false;
}

Expand All @@ -158,6 +191,22 @@ class Popover extends PopoverPositionMixin(
target.removeEventListener('keydown', this.__onTargetKeydown);
}

/**
* Overlay's global outside click listener doesn't work when
* the overlay is modeless, so we use a separate listener.
* @private
*/
__onGlobalClick(event) {
if (
this._opened &&
!this.modal &&
!event.composedPath().some((el) => el === this._overlayElement || el === this.target) &&
!this.noCloseOnOutsideClick
) {
this._opened = false;
}
}

/** @private */
__onTargetClick() {
this._opened = !this._opened;
Expand Down
84 changes: 83 additions & 1 deletion packages/popover/test/basic.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,38 @@ describe('popover', () => {
});
});

describe('overlay properties', () => {
it('should set modeless on the overlay by default', () => {
expect(overlay.modeless).to.be.true;
});

it('should set modeless on the overlay to false when modal is true', async () => {
popover.modal = true;
await nextUpdate(popover);
expect(overlay.modeless).to.be.false;
});

it('should not set focusTrap on the overlay by default', () => {
expect(overlay.modeless).to.be.true;
});

it('should set focusTrap on the overlay to true when modal is true', async () => {
popover.modal = true;
await nextUpdate(popover);
expect(overlay.focusTrap).to.be.true;
});

it('should propagate withBackdrop property to the overlay', async () => {
popover.withBackdrop = true;
await nextUpdate(popover);
expect(overlay.withBackdrop).to.be.true;

popover.withBackdrop = false;
await nextUpdate(popover);
expect(overlay.withBackdrop).to.be.false;
});
});

describe('interactions', () => {
let target;

Expand Down Expand Up @@ -149,6 +181,18 @@ describe('popover', () => {
expect(overlay.opened).to.be.false;
});

it('should close overlay on outside click when modal is true', async () => {
popover.modal = true;
await nextUpdate(popover);

target.click();
await nextRender();

outsideClick();
await nextRender();
expect(overlay.opened).to.be.false;
});

it('should not close on outside click if noCloseOnOutsideClick is true', async () => {
popover.noCloseOnOutsideClick = true;
await nextUpdate(popover);
Expand All @@ -170,13 +214,30 @@ describe('popover', () => {
expect(overlay.opened).to.be.false;
});

it('should remove document click listener when popover is detached', async () => {
const spy = sinon.spy(document, 'removeEventListener');
popover.remove();
await nextRender();
expect(spy).to.be.called;
expect(spy.firstCall.args[0]).to.equal('click');
});

describe('Escape press', () => {
beforeEach(async () => {
target.click();
await nextRender();
});

it('should close overlay on global Escape press by default', async () => {
it('should not close overlay on global Escape press by default', async () => {
esc(document.body);
await nextRender();
expect(overlay.opened).to.be.true;
});

it('should close overlay on global Escape press when modal is true', async () => {
popover.modal = true;
await nextUpdate(popover);

esc(document.body);
await nextRender();
expect(overlay.opened).to.be.false;
Expand All @@ -195,6 +256,7 @@ describe('popover', () => {
});

it('should not close on global Escape press if noCloseOnEsc is true', async () => {
popover.modal = true;
popover.noCloseOnEsc = true;
await nextUpdate(popover);

Expand All @@ -221,5 +283,25 @@ describe('popover', () => {
expect(overlay.opened).to.be.true;
});
});

describe('backdrop', () => {
beforeEach(async () => {
popover.withBackdrop = true;
await nextUpdate(popover);

target.click();
await nextRender();
});

it('should set pointer-events on backdrop to none when non modal', () => {
expect(getComputedStyle(overlay.$.backdrop).pointerEvents).to.equal('none');
});

it('should set pointer-events on backdrop to auto when modal', async () => {
popover.modal = true;
await nextUpdate(popover);
expect(getComputedStyle(overlay.$.backdrop).pointerEvents).to.equal('auto');
});
});
});
});
2 changes: 2 additions & 0 deletions packages/popover/test/typings/popover.types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,5 +23,7 @@ assertType<HTMLElement | undefined>(popover.target);
assertType<PopoverPosition>(popover.position);
assertType<PopoverRenderer | null | undefined>(popover.renderer);
assertType<string>(popover.overlayClass);
assertType<boolean>(popover.modal);
assertType<boolean>(popover.withBackdrop);
assertType<boolean>(popover.noCloseOnEsc);
assertType<boolean>(popover.noCloseOnOutsideClick);
Loading