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

WIP: refactore dialog DOM creation #1671

Closed
wants to merge 5 commits into from
Closed

WIP: refactore dialog DOM creation #1671

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
237ce55
feat(dialog): move DOM event handling to DialogDomRenderer, remove Di…
ekzobrain Feb 10, 2023
6aadfc4
fix(dialog): add missing tabindex
ekzobrain Feb 10, 2023
d34ee33
fix(dialog): use setting from controller, fix build
ekzobrain Feb 10, 2023
4a24c7a
feat(dialog): make renderer async and allow communication with compon…
ekzobrain Feb 18, 2023
8784751
feat(dialog): adjust renderer container
ekzobrain Feb 18, 2023
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
2 changes: 0 additions & 2 deletions packages/dialog/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ export {
IDialogService,
IDialogController,
IDialogDomRenderer,
IDialogDom,

// dialog results
DialogCloseResult,
Expand Down Expand Up @@ -46,7 +45,6 @@ export {
} from './plugins/dialog/dialog-configuration';

export {
DefaultDialogDom,
DefaultDialogDomRenderer,
DefaultDialogGlobalSettings,
} from './plugins/dialog/dialog-default-impl';
38 changes: 13 additions & 25 deletions packages/dialog/src/plugins/dialog/dialog-controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import {
DialogDeactivationStatuses,
IDialogController,
IDialogDomRenderer,
IDialogDom,
DialogOpenResult,
DialogCloseResult,
DialogCancelError,
Expand Down Expand Up @@ -47,9 +46,9 @@ export class DialogController implements IDialogController {
public readonly closed: Promise<DialogCloseResult>;

/**
* The dom structure created to support the dialog associated with this controller
* The renderer used to create dialog DOM
*/
private dom!: IDialogDom;
private renderer!: IDialogDomRenderer;

/**
* The component controller associated with this dialog controller
Expand All @@ -74,17 +73,18 @@ export class DialogController implements IDialogController {

/** @internal */
public activate(settings: IDialogLoadedSettings): Promise<DialogOpenResult> {
this.settings = settings;

const container = this.ctn.createChild();
const { model, template, rejectOnCancel } = settings;
const hostRenderer: IDialogDomRenderer = container.get(IDialogDomRenderer);
const dialogTargetHost = settings.host ?? this.p.document.body;
const dom = this.dom = hostRenderer.render(dialogTargetHost, settings);
const renderer = container.get(IDialogDomRenderer);
const contentHost = renderer.render(dialogTargetHost, this);
const rootEventTarget = container.has(IEventTarget, true)
? container.get(IEventTarget) as Element
: null;
const contentHost = dom.contentHost;

this.settings = settings;
this.renderer = renderer;
// application root host may be a different element with the dialog root host
// example:
// <body>
Expand All @@ -97,7 +97,7 @@ export class DialogController implements IDialogController {

container.register(
instanceRegistration(INode, contentHost),
instanceRegistration(IDialogDom, dom),
instanceRegistration(IDialogDomRenderer, renderer),
);

return new Promise(r => {
Expand All @@ -106,7 +106,7 @@ export class DialogController implements IDialogController {
})
.then(canActivate => {
if (canActivate !== true) {
dom.dispose();
renderer.dispose();
if (rejectOnCancel) {
throw createDialogCancelError(null, 'Dialog activation rejected');
}
Expand All @@ -126,12 +126,11 @@ export class DialogController implements IDialogController {
)
) as ICustomElementController;
return onResolve(ctrlr.activate(ctrlr, null, LifecycleFlags.fromBind), () => {
dom.overlay.addEventListener(settings.mouseEvent ?? 'click', this);
return DialogOpenResult.create(false, this);
});
});
}, e => {
dom.dispose();
renderer.dispose();
throw e;
});
}
Expand All @@ -143,7 +142,7 @@ export class DialogController implements IDialogController {
}

let deactivating = true;
const { controller, dom, cmp, settings: { mouseEvent, rejectOnCancel }} = this;
const { controller, renderer, cmp, settings: { rejectOnCancel }} = this;
const dialogResult = DialogCloseResult.create(status, value);

const promise: Promise<DialogCloseResult<T>> = new Promise<DialogCloseResult<T>>(r => {
Expand All @@ -162,8 +161,7 @@ export class DialogController implements IDialogController {
return onResolve(cmp.deactivate?.(dialogResult),
() => onResolve(controller.deactivate(controller, null, LifecycleFlags.fromUnbind),
() => {
dom.dispose();
dom.overlay.removeEventListener(mouseEvent ?? 'click', this);
renderer.dispose();
if (!rejectOnCancel && status !== DialogDeactivationStatuses.Error) {
this._resolve(dialogResult);
} else {
Expand Down Expand Up @@ -217,23 +215,13 @@ export class DialogController implements IDialogController {
() => onResolve(
this.controller.deactivate(this.controller, null, LifecycleFlags.fromUnbind),
() => {
this.dom.dispose();
this.renderer.dispose();
this._reject(closeError);
}
)
)));
}

/** @internal */
public handleEvent(event: MouseEvent): void {
if (/* user allows dismiss on overlay click */this.settings.overlayDismiss
&& /* did not click inside the host element */!this.dom.contentHost.contains(event.target as Element)
) {
// eslint-disable-next-line @typescript-eslint/no-floating-promises
this.cancel();
}
}

private getOrCreateVm(container: IContainer, settings: IDialogLoadedSettings, host: HTMLElement): IDialogComponent<object> {
const Component = settings.component;
if (Component == null) {
Expand Down
96 changes: 70 additions & 26 deletions packages/dialog/src/plugins/dialog/dialog-default-impl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,8 @@
import { IPlatform } from '@aurelia/runtime-html';
import {
IDialogDomRenderer,
IDialogDom,
IDialogGlobalSettings,
} from './dialog-interfaces';
import { IDialogDomRenderer, IDialogGlobalSettings, DialogActionKey, IDialogController } from './dialog-interfaces';

import { IContainer } from '@aurelia/kernel';
import { singletonRegistration } from '../../utilities-di';
import { singletonRegistration, transientRegistration } from '../../utilities-di';

export class DefaultDialogGlobalSettings implements IDialogGlobalSettings {

Expand All @@ -20,45 +16,93 @@ export class DefaultDialogGlobalSettings implements IDialogGlobalSettings {
}

const baseWrapperCss = 'position:absolute;width:100%;height:100%;top:0;left:0;';
const wrapperCss = `${baseWrapperCss}display:flex;`;
const hostCss = 'position:relative;margin:auto;';

export class DefaultDialogDomRenderer implements IDialogDomRenderer {
export class DefaultDialogDomRenderer implements IDialogDomRenderer, EventListenerObject {

/** @internal */
protected static inject = [IPlatform];

public wrapper!: HTMLElement;

public overlay!: HTMLElement;

public contentHost!: HTMLElement;

/** @internal */
protected controller!: IDialogController;

public constructor(private readonly p: IPlatform) {}

public static register(container: IContainer) {
singletonRegistration(IDialogDomRenderer, this).register(container);
transientRegistration(IDialogDomRenderer, this).register(container);
}

private readonly wrapperCss: string = `${baseWrapperCss} display:flex;`;
private readonly overlayCss: string = baseWrapperCss;
private readonly hostCss: string = 'position:relative;margin:auto;';

public render(dialogHost: HTMLElement): IDialogDom {
public render(dialogHost: HTMLElement, controller: IDialogController): HTMLElement {
Copy link
Member

Choose a reason for hiding this comment

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

Associating a parameter to some state in a private property this way is not appropriate. It's brittle in a way that this render call is now supposed to be called only once, but the interface doesn't communicate that. Overall, this is where my concerns are: it's unclear at what point what states are available if we make something named "renderer" a "lazily-stateful" service.

Copy link
Contributor Author

@ekzobrain ekzobrain Feb 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I agree that renderer interface should be improved. This change was just the first step, showing the main goal - incapsulation of DOM manipulation inside renderer or some DialogDom instance, cleanup of DialogService and dialogController. Now I plan to create several POC renderers to see what states and renderer capabilities would they need to integrate seemlessly into current dialog lifecycle, so next step is to design and approve final renderer interface.

const doc = this.p.document;
const h = (name: string, css: string) => {
const h = (name: string, css: string): HTMLElement => {
const el = doc.createElement(name);
el.style.cssText = css;
return el;
};
const wrapper = dialogHost.appendChild(h('au-dialog-container', this.wrapperCss));
const overlay = wrapper.appendChild(h('au-dialog-overlay', this.overlayCss));
const host = wrapper.appendChild(h('div', this.hostCss));
return new DefaultDialogDom(wrapper, overlay, host);
}
}
const wrapper = dialogHost.appendChild(h('au-dialog-container', wrapperCss));
wrapper.setAttribute('tabindex', '-1');
const overlay = wrapper.appendChild(h('au-dialog-overlay', baseWrapperCss));
const contentHost = wrapper.appendChild(h('div', hostCss));

overlay.addEventListener(controller.settings.mouseEvent ?? 'click', this);
wrapper.addEventListener('keydown', this);

this.wrapper = wrapper;
this.overlay = overlay;
this.contentHost = contentHost;
this.controller = controller;

export class DefaultDialogDom implements IDialogDom {
public constructor(
public readonly wrapper: HTMLElement,
public readonly overlay: HTMLElement,
public readonly contentHost: HTMLElement,
) {
return contentHost;
}

public dispose(): void {
this.wrapper.removeEventListener('keydown', this);
this.overlay.removeEventListener(this.controller.settings.mouseEvent ?? 'click', this);
this.wrapper.remove();
}

/** @internal */
public handleEvent(event: KeyboardEvent | MouseEvent): void {
const { controller } = this;

// handle wrapper keydown
if (event.type === 'keydown') {
const key = getActionKey(event as KeyboardEvent);
if (key == null) {
return;
}

const keyboard = controller.settings.keyboard;
if (key === 'Escape' && keyboard.includes(key)) {
void controller.cancel();
} else if (key === 'Enter' && keyboard.includes(key)) {
void controller.ok();
}
return;
}

// handle overlay click
if (/* user allows to dismiss on overlay click */controller.settings.overlayDismiss
&& /* did not click inside the host element */!this.contentHost.contains(event.target as Element)
) {
void controller.cancel();
}
}
}

function getActionKey(e: KeyboardEvent): DialogActionKey | undefined {
if ((e.code || e.key) === 'Escape' || e.keyCode === 27) {
return 'Escape';
}
if ((e.code || e.key) === 'Enter' || e.keyCode === 13) {
return 'Enter';
}
return undefined;
}
13 changes: 2 additions & 11 deletions packages/dialog/src/plugins/dialog/dialog-interfaces.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,17 +45,8 @@ export interface IDialogController {
* An interface describing the object responsible for creating the dom structure of a dialog
*/
export const IDialogDomRenderer = createInterface<IDialogDomRenderer>('IDialogDomRenderer');
export interface IDialogDomRenderer {
render(dialogHost: Element, settings: IDialogLoadedSettings): IDialogDom;
}

/**
* An interface describing the DOM structure of a dialog
*/
export const IDialogDom = createInterface<IDialogDom>('IDialogDom');
export interface IDialogDom extends IDisposable {
readonly overlay: HTMLElement;
readonly contentHost: HTMLElement;
export interface IDialogDomRenderer extends IDisposable {
render(dialogHost: Element, controller: IDialogController): 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'm a bit concerned by this interface changes. This change implies states are being brought into the renderer. Doing it this way make it questionable what's the relationship between rendered elements and the renderer. Are they tied to each other until disposed?
Previously renderer is just renderer, after rendering, nothing is needed from the renderer.

Copy link
Contributor Author

@ekzobrain ekzobrain Feb 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, renderer is now transient, not singleton. It is merged together with DialogDom, which is now gone, because it reflected a concrete DOM structure, which could not be overriden via configuration

Copy link
Member

Choose a reason for hiding this comment

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

it reflected a concrete DOM structure

This DOM structure is quite standard for dialog though. Overlay + content host are commonly expected. Was this structure the issue leading to this PR?

Copy link
Contributor Author

@ekzobrain ekzobrain Feb 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Sure it is quite common, but most of the times we will not need access to this structure. Previously, as I understand, DialogDom was created for keeping a statefull reference to DOM elements for these purposes:

  1. Allow DialogController to access DOM nodes to handle their events. This is not the case any more because it is now renderer responsibility.
  2. Allow styling of dialog container/overlay/host (https://docs.aurelia.io/aurelia-packages/dialog#centering-uncentering-dialog-position) in default renderer implementation. This is a very specific approach to set container element styles directly from a dialog component, but it is still available for default implementation, you just need to inject IDialogDomRenderer instead of IDialogDom in dialog content component.

Now that renderer subscribes and unsubscribes from DOM events - it needs to be statefull, so I made it transient and merged with DialogDom to avoid state duplication.

Now consider that we need to implement a new renderer for Bootstrap modal (https://getbootstrap.com/docs/5.3/components/modal/#via-javascript). We need to create Bootstrap DOM structure and instantiate it's JS modal component (or use a wrapper custom element around it). So we need to keep some state between DOM render and disposal (reference to bootstrap.Modal instance in this case), and that state would be much different than DialogDom was. And if we implement a renderer for Material Web Components Dialog - it would be the third different state. So state depends heavily on underlying renderer implementation (library API) and keeping separate DialogDom with this concrete structure (state) does not make much sense, it is a specific case for current default implementation.

}

// export type IDialogCancellableOpenResult = IDialogOpenResult | IDialogCancelResult;
Expand Down
38 changes: 1 addition & 37 deletions packages/dialog/src/plugins/dialog/dialog-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import { IContainer, onResolve, resolveAll } from '@aurelia/kernel';
import { AppTask, IPlatform } from '@aurelia/runtime-html';

import {
DialogActionKey,
DialogCloseResult,
DialogDeactivationStatuses,
DialogOpenResult,
Expand Down Expand Up @@ -104,10 +103,7 @@ export class DialogService implements IDialogService {
dialogController.activate(loadedSettings),
openResult => {
if (!openResult.wasCancelled) {
if (this.dlgs.push(dialogController) === 1) {
this.p.window.addEventListener('keydown', this);
}

this.dlgs.push(dialogController);
const $removeController = () => this.remove(dialogController);
dialogController.closed.then($removeController, $removeController);
}
Expand Down Expand Up @@ -152,28 +148,6 @@ export class DialogService implements IDialogService {
if (idx > -1) {
this.dlgs.splice(idx, 1);
}
if (dlgs.length === 0) {
this.p.window.removeEventListener('keydown', this);
}
}

/** @internal */
public handleEvent(e: Event): void {
const keyEvent = e as KeyboardEvent;
const key = getActionKey(keyEvent);
if (key == null) {
return;
}
const top = this.top;
if (top === null || top.settings.keyboard.length === 0) {
return;
}
const keyboard = top.settings.keyboard;
if (key === 'Escape' && keyboard.includes(key)) {
void top.cancel();
} else if (key === 'Enter' && keyboard.includes(key)) {
void top.ok();
}
}
}

Expand Down Expand Up @@ -238,13 +212,3 @@ function asDialogOpenPromise(promise: Promise<unknown>): DialogOpenPromise {
(promise as DialogOpenPromise).whenClosed = whenClosed;
return promise as DialogOpenPromise;
}

function getActionKey(e: KeyboardEvent): DialogActionKey | undefined {
if ((e.code || e.key) === 'Escape' || e.keyCode === 27) {
return 'Escape';
}
if ((e.code || e.key) === 'Enter' || e.keyCode === 13) {
return 'Enter';
}
return undefined;
}
3 changes: 3 additions & 0 deletions packages/dialog/src/utilities-di.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,6 @@ export const instanceRegistration = Registration.instance;

/** @internal */
export const callbackRegistration = Registration.callback;

/** @internal */
export const transientRegistration = Registration.transient;