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.

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

Jump to bottom

refactor(overlay): make interaction controllers more lazy #4319

Merged
merged 1 commit into from Apr 29, 2024
Merged

refactor(overlay): make interaction controllers more lazy #4319

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Expand Up @@ -246,6 +246,7 @@
],
"output": [
"packages/**/*.js",
"packages/**/*.dev.js",
"projects/**/*.js",
"packages/**/*.js.map",
"projects/**/*.js.map",
Expand Down
4 changes: 4 additions & 0 deletions packages/overlay/package.json
View file
Open in desktop
Expand Up @@ -115,6 +115,10 @@
"development": "./src/slottable-request-event.dev.js",
"default": "./src/slottable-request-event.js"
},
"./src/strategies.js": {
"development": "./src/strategies.dev.js",
"default": "./src/strategies.js"
},
"./active-overlay.js": {
"development": "./active-overlay.dev.js",
"default": "./active-overlay.js"
Expand Down
21 changes: 13 additions & 8 deletions packages/overlay/src/AbstractOverlay.ts
View file
Open in desktop
Expand Up @@ -357,13 +357,15 @@ export class AbstractOverlay extends SpectrumElement {
const options = optionsV1;
AbstractOverlay.applyOptions(overlay, {
...options,
delayed: options.delayed || overlayContent.hasAttribute('delayed'),
delayed:
Rajdeepc marked this conversation as resolved.
Show resolved Hide resolved
options.delayed || overlayContent.hasAttribute('delayed'),
trigger: options.virtualTrigger || trigger,
type: interaction === 'modal'
? 'modal'
: interaction === 'hover'
? 'hint'
: 'auto'
type:
interaction === 'modal'
? 'modal'
: interaction === 'hover'
? 'hint'
: 'auto',
});
trigger.insertAdjacentElement('afterend', overlay);
await overlay.updateComplete;
Expand All @@ -375,7 +377,7 @@ export class AbstractOverlay extends SpectrumElement {
overlay.append(overlayContent);
AbstractOverlay.applyOptions(overlay, {
...options,
delayed: options.delayed || overlayContent.hasAttribute('delayed')
delayed: options.delayed || overlayContent.hasAttribute('delayed'),
});
overlay.updateComplete.then(() => {
// Do we want to "open" this path, or leave that to the consumer?
Expand All @@ -384,7 +386,10 @@ export class AbstractOverlay extends SpectrumElement {
return overlay;
}

static applyOptions(overlay: Overlay, options: OverlayOptions): void {
static applyOptions(
overlay: AbstractOverlay,
Copy link
Contributor

@Rajdeepc Rajdeepc Apr 29, 2024 •
edited

Choose a reason for hiding this comment

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

Just for curiosity which subclasses or properties we are trying to bring in by changing to AbstractOverlay from Overlay?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

An Overlay is an AbstractOverlay but AbstractOverlay comes with less transient dependencies, so there is less JS loaded when using this class in overlay-trigger-directive.ts

options: OverlayOptions
): void {
overlay.delayed = !!options.delayed;
overlay.receivesFocus = options.receivesFocus ?? 'auto';
overlay.triggerElement = options.trigger || null;
Expand Down
4 changes: 2 additions & 2 deletions packages/overlay/src/ClickController.ts
View file
Open in desktop
Expand Up @@ -28,13 +28,13 @@ export class ClickController extends InteractionController {

handleClick(): void {
if (!this.preventNextToggle) {
this.host.open = !this.host.open;
this.open = !this.open;
}
this.preventNextToggle = false;
}

handlePointerdown(): void {
this.preventNextToggle = this.host.open;
this.preventNextToggle = this.open;
}

override init(): void {
Expand Down
38 changes: 24 additions & 14 deletions packages/overlay/src/HoverController.ts
View file
Open in desktop
Expand Up @@ -37,23 +37,23 @@ export class HoverController extends InteractionController {
if (!document.activeElement?.matches(':focus-visible')) {
return;
}
this.host.open = true;
this.open = true;
this.focusedin = true;
}

handleTargetFocusout(): void {
this.focusedin = false;
if (this.pointerentered) return;
this.host.open = false;
this.open = false;
}

handleTargetPointerenter(): void {
if (this.hoverTimeout) {
clearTimeout(this.hoverTimeout);
this.hoverTimeout = undefined;
}
if (this.host.disabled) return;
this.host.open = true;
if (this.overlay?.disabled) return;
this.open = true;
this.pointerentered = true;
}

Expand All @@ -76,11 +76,11 @@ export class HoverController extends InteractionController {

override prepareDescription(): void {
// require "content" to apply relationship
if (!this.host.elements.length) return;
if (!this.overlay.elements.length) return;

const triggerRoot = this.target.getRootNode();
const contentRoot = this.host.elements[0].getRootNode();
const overlayRoot = this.host.getRootNode();
const contentRoot = this.overlay.elements[0].getRootNode();
const overlayRoot = this.overlay.getRootNode();
if (triggerRoot === overlayRoot) {
this.prepareOverlayRelativeDescription();
} else if (triggerRoot === contentRoot) {
Expand All @@ -92,7 +92,7 @@ export class HoverController extends InteractionController {
const releaseDescription = conditionAttributeWithId(
this.target,
'aria-describedby',
[this.host.id]
[this.overlay.id]
);
this.releaseDescription = () => {
releaseDescription();
Expand All @@ -102,10 +102,10 @@ export class HoverController extends InteractionController {

private prepareContentRelativeDescription(): void {
const elementIds: string[] = [];
const appliedIds = this.host.elements.map((el) => {
const appliedIds = this.overlay.elements.map((el) => {
elementIds.push(el.id);
if (!el.id) {
el.id = `${this.host.tagName.toLowerCase()}-helper-${randomID()}`;
el.id = `${this.overlay.tagName.toLowerCase()}-helper-${randomID()}`;
}
return el.id;
});
Expand All @@ -117,7 +117,7 @@ export class HoverController extends InteractionController {
);
this.releaseDescription = () => {
releaseDescription();
this.host.elements.map((el, index) => {
this.overlay.elements.map((el, index) => {
el.id = this.elementIds[index];
});
this.releaseDescription = noop;
Expand All @@ -130,7 +130,7 @@ export class HoverController extends InteractionController {
if (this.focusedin && triggerElement.matches(':focus-visible')) return;

this.hoverTimeout = setTimeout(() => {
this.host.open = false;
this.open = false;
}, HOVER_DELAY);
}

Expand Down Expand Up @@ -159,12 +159,22 @@ export class HoverController extends InteractionController {
() => this.handleTargetPointerleave(),
{ signal }
);
this.host.addEventListener(
if (this.overlay) {
this.initOverlay();
}
}

override initOverlay(): void {
if (!this.abortController) {
return;
}
const { signal } = this.abortController;
this.overlay.addEventListener(
'pointerenter',
() => this.handleHostPointerenter(),
{ signal }
);
this.host.addEventListener(
this.overlay.addEventListener(
'pointerleave',
() => this.handleHostPointerleave(),
{ signal }
Expand Down
74 changes: 71 additions & 3 deletions packages/overlay/src/InteractionController.ts
View file
Open in desktop
Expand Up @@ -19,21 +19,84 @@ export enum InteractionTypes {
'longpress',
}

export type ControllerOptions = {
overlay?: AbstractOverlay;
handleOverlayReady?: (overlay: AbstractOverlay) => void;
isPersistent?: boolean;
};

export class InteractionController implements ReactiveController {
abortController!: AbortController;

get activelyOpening(): boolean {
return false;
}

type!: InteractionTypes;
private handleOverlayReady?: (overlay: AbstractOverlay) => void;

public get open(): boolean {
return this.overlay?.open ?? false;
}

/**
* Set `open` against the associated Overlay lazily.
*/
public set open(open: boolean) {
if (this.overlay) {
// If there already is an Overlay, apply the value of `open` directly.
this.overlay.open = open;
Rajdeepc marked this conversation as resolved.
Show resolved Hide resolved
return;
}
if (!open) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we keep this above line 42?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If there is a value for this.overlay we want to apply it. If there isn't a value, and the state is closed, this is the same as having no Overlay at all, so we early return. I can add some comments on this.

// When `open` moves to `false` and there is not yet an Overlay,
// assume that no Overlay and a closed Overlay are the same and return early.
return;
}
// When there is no Overlay and `open` is moving to `true`, lazily import/create
// an Overlay and apply that state to it.
customElements
.whenDefined('sp-overlay')
.then(async (): Promise<void> => {
const { Overlay } = await import('./Overlay.js');
this.overlay = new Overlay();
this.overlay.open = true;
});
import('@spectrum-web-components/overlay/sp-overlay.js');
}

public get overlay(): AbstractOverlay {
return this._overlay;
}

constructor(public host: AbstractOverlay, public target: HTMLElement, private isPersistent = false) {
this.host.addController(this);
public set overlay(overlay: AbstractOverlay | undefined) {
if (!overlay) return;
if (this.overlay === overlay) return;
if (this.overlay) {
this.overlay.removeController(this);
}
this._overlay = overlay;
this.overlay.addController(this);
this.initOverlay();
this.prepareDescription(this.target);
this.handleOverlayReady?.(this.overlay);
}

private _overlay!: AbstractOverlay;

protected isPersistent = false;

type!: InteractionTypes;

constructor(
public target: HTMLElement,
{ overlay, isPersistent, handleOverlayReady }: ControllerOptions
) {
this.isPersistent = !!isPersistent;
this.handleOverlayReady = handleOverlayReady;
if (this.isPersistent) {
this.init();
}
this.overlay = overlay;
}

prepareDescription(_: HTMLElement): void {}
Expand All @@ -47,6 +110,11 @@ export class InteractionController implements ReactiveController {
// Abstract init() method.
}

/* c8 ignore next 3 */
initOverlay(): void {
// Abstract initOverlay() method.
}

abort(): void {
this.releaseDescription();
this.abortController?.abort();
Expand Down
16 changes: 10 additions & 6 deletions packages/overlay/src/LongpressController.ts
View file
Open in desktop
Expand Up @@ -18,7 +18,10 @@ import { conditionAttributeWithId } from '@spectrum-web-components/base/src/cond
import { randomID } from '@spectrum-web-components/shared/src/random-id.js';

import { noop } from './AbstractOverlay.js';
import { InteractionController, InteractionTypes } from './InteractionController.js';
import {
InteractionController,
InteractionTypes,
} from './InteractionController.js';

const LONGPRESS_DURATION = 300;
export const LONGPRESS_INSTRUCTIONS = {
Expand Down Expand Up @@ -48,7 +51,7 @@ export class LongpressController extends InteractionController {
private timeout!: ReturnType<typeof setTimeout>;

handleLongpress(): void {
this.host.open = true;
this.open = true;
this.longpressState =
this.longpressState === 'potential' ? 'opening' : 'pressed';
}
Expand Down Expand Up @@ -83,7 +86,8 @@ export class LongpressController extends InteractionController {
// or `pointerup` should move the `longpressState` to
// `null` so that the earlier event can void the "light
// dismiss" and keep the Overlay open.
this.longpressState = this.host.state === 'opening' ? 'pressed' : null;
this.longpressState =
this.overlay?.state === 'opening' ? 'pressed' : null;
document.removeEventListener('pointerup', this.handlePointerup);
document.removeEventListener('pointercancel', this.handlePointerup);
};
Expand Down Expand Up @@ -123,7 +127,7 @@ export class LongpressController extends InteractionController {
// do not reapply until target is recycled
this.releaseDescription !== noop ||
// require "longpress content" to apply relationship
!this.host.elements.length
!this.overlay.elements.length
) {
return;
}
Expand All @@ -134,13 +138,13 @@ export class LongpressController extends InteractionController {
longpressDescription.textContent = LONGPRESS_INSTRUCTIONS[messageType];
longpressDescription.slot = 'longpress-describedby-descriptor';
const triggerParent = trigger.getRootNode() as HTMLElement;
const overlayParent = this.host.getRootNode() as HTMLElement;
const overlayParent = this.overlay.getRootNode() as HTMLElement;
// Manage the placement of the helper element in an accessible place with
// the lowest chance of negatively affecting the layout of the page.
if (triggerParent === overlayParent) {
// Trigger and Overlay in same DOM tree...
// Append helper element to Overlay.
this.host.append(longpressDescription);
this.overlay.append(longpressDescription);
} else {
// If Trigger in <body>, hide helper
longpressDescription.hidden = !('host' in triggerParent);
Expand Down
19 changes: 8 additions & 11 deletions packages/overlay/src/Overlay.ts
View file
Open in desktop
Expand Up @@ -45,10 +45,11 @@ import { OverlayNoPopover } from './OverlayNoPopover.js';
import { overlayStack } from './OverlayStack.js';
import { VirtualTrigger } from './VirtualTrigger.js';
import { PlacementController } from './PlacementController.js';
import { ClickController } from './ClickController.js';
import { HoverController } from './HoverController.js';
import { LongpressController } from './LongpressController.js';
import type { ClickController } from './ClickController.js';
import type { HoverController } from './HoverController.js';
import type { LongpressController } from './LongpressController.js';
export { LONGPRESS_INSTRUCTIONS } from './LongpressController.js';
import { strategies } from './strategies.js';
import {
removeSlottableRequest,
SlottableRequestEvent,
Expand All @@ -66,12 +67,6 @@ if (supportsPopover) {
OverlayFeatures = OverlayNoPopover(OverlayFeatures);
}

export const strategies = {
click: ClickController,
longpress: LongpressController,
hover: HoverController,
};

/**
* @element sp-overlay
*
Expand Down Expand Up @@ -498,8 +493,10 @@ export class Overlay extends OverlayFeatures {
if (!this.hasNonVirtualTrigger) return;
if (!this.triggerInteraction) return;
this.strategy = new strategies[this.triggerInteraction](
this,
this.triggerElement as HTMLElement
this.triggerElement as HTMLElement,
{
overlay: this,
}
);
}

Expand Down