WIP: refactore dialog DOM creation

[ekzobrain]

Pull Request

📖 Description

The goal of this change is to refactore DialogService and DialogController to be more DOM-agnostic and allow flexible DefaultDialogDomRenderer overriding (for example create BootstrapDialogDomRenderer which renders dialog using Bootstrap's markup and native JS).
Changes include:

• DialogService does not listen and react to dialog's wrapper keydown event, moved to DefaultDialogDomRenderer
• DialogController does not listen and react to dialog's overlay click event, moved to DefaultDialogDomRenderer
• DialogDom is removed, it's functionality is moved to DefaultDialogDomRenderer
• keydown event is now listened on dialog's wrapper elements instead of Window (tabindex="-1" added to wrapper element to make it focusable and thus dispatch this event type). This allows not to warry about dialog hierarhy progmatically - the last open dialog (top) will always receive this event.

🎫 Issues

👩‍💻 Reviewer Notes

📑 Test Plan

⏭ Next Steps

[Vheissu]

This is looking good so far @netcitylife can't wait to see this evolve. I think an important thing to note is for such a change we will require:

• Tests
• Doc changes

But, you're probably already planning on those. Just thought I would mention.

[bigopon]

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.

[ekzobrain]

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

[bigopon]

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?

[ekzobrain]

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.

[ekzobrain]

This is looking good so far @netcitylife can't wait to see this evolve. I think an important thing to note is for such a change we will require:

• Tests

• Doc changes

But, you're probably already planning on those. Just thought I would mention.

Sure, when finished

[bigopon]

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.

[ekzobrain]

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.

[bigopon]

@netcitylife can you elaborate what would be the challenge extending the existing way of doing things for something like a bootstrap modal? IDialogRenderer accepts controller in its stateless render method, so there's no reason a custom renderer cannot create a custom IDialogDom that does more than the default one.

class MyRenderer {
  render(_, controller) {
    const dom = new BootstrapDialogDom(controller);
  }
}

class BootstrapDialogDom {
  constructor(private controller) {
    // attach listener here and close controller when necessary.
  } 
}

[ekzobrain]

@netcitylife can you elaborate what would be the challenge extending the existing way of doing things for something like a bootstrap modal? IDialogRenderer accepts controller in its stateless render method, so there's no reason a custom renderer cannot create a custom IDialogDom that does more than the default one.

class MyRenderer {
  render(_, controller) {
    const dom = new BootstrapDialogDom(controller);
  }
}

class BootstrapDialogDom {
  constructor(private controller) {
    // attach listener here and close controller when necessary.
  } 
}

In this approach I see no need in a renderer at all, it does nothing but instantiating BootstrapDialogDom, which will create DOM in constructor() and then destroy it in dispose(). So it may be just called directly from controller. I see your confusion about current renderer interface, see my comment above.

[bigopon]

...no need in a renderer at all, it does nothing but instantiating...

We are doing registration for plug & play so that custom renderer can be used. It's certainly a useful layer to have for customization.

Can you also help tackle the other parts in my comment, about (a) the challenges of extending the existing one, and (b) why wouldn't you be able to do it with just different IDialogDom above?

[ekzobrain]

...no need in a renderer at all, it does nothing but instantiating...

We are doing registration for plug & play so that custom renderer can be used. It's certainly a useful layer to have for customization.

Can you also help tackle the other parts in my comment, about (a) the challenges of extending the existing one, and (b) why wouldn't you be able to do it with just different IDialogDom above?

See my comment in Discord, please

[bigopon]

if it's desirable, we can add custom lifecycle hooks the way we have for router. Just need to figure out an acceptable API. We shouldn't be changing the renderer just to accommodate this. Rendering process should be simple, idempotent and synchronous.