vaadin / web-components

A set of high-quality standards based web components for enterprise web applications. Part of Vaadin 20+
https://vaadin.com/docs/latest/components
473 stars 83 forks source link

[overlay] Consider avoiding teleportation #749

Open platosha opened 7 years ago

platosha commented 7 years ago

Let us say, <consumer-element> uses <vaadin-overlay>. Teleporting <vaadin-overlay>’s host element to document body introduces cumbersome usage issues in consumer elements like <consumer-element>.

In particular, if <vaadin-overlay> (or its subclass) is placed in <consumer-element>’s template, it is hard for <consumer-element> to find a stable reference to the overlay. The opened observer can teleport it early, so that it’s no longer possible for <consumer-element> find the overlay element by querying the local DOM.

Right now we use workarounds like adding id to the overlay and using Polymer’s builtin memoized $.ids references. This works, but relies on Polymer’s init timing, therefore not elegant.

I think, we should consider not teleporting the overlay host, therefore allowing the consumer component to always have it in their light DOM and query at any given time. This would remove need for the $.ids workarounds.

The above does not mean we drop core features/requirements, of course we have to keep them, including:

jouni commented 7 years ago

Am I right to assume, that you are proposing we would teleport some internal element rather than the host?

platosha commented 7 years ago

@jouni the implementation details are to be figured out.

Personally, at this point, I think it’s best to avoid teleporting altogether, so that it does not occur at all. If we teleport some other element, I’m afraid, same issues might strike back on the overlay element level this time.

I’ve got two rough ideas to start prototyping around:

  1. Make <vaadin-overlay> host not used in parent template. Instead, make a helper class (or mixin?) to instantiate the overlay and return the instance reference:

    consumerElement._overlay = consumerElement._ensureOverlayInstance(
      consumerElement._overlay,
      'vaadin-overlay'
    );
    consumerElement._overlay.opened = true;
  2. Another idea: make it so that the during opening, instead of teleporting itself, would create a helper element instance in the body, stamp the template in the helper’s shadow root and store its reference:

    _openedChanged(opened) {
    // ensure this._templateInstance is stamped, then...
    if (!this._helperInstance) {
      this._helperInstance = document.createElement('vaadin-overlay-helper');
      this._helperInstance.attachShadowRoot({mode: 'open'});
      this._helperInstance.appendChild(this._templateInstance.root);
    }
    document.body.appendChild(this._helperInstance);
    }
platosha commented 7 years ago

My gut feeling is that the first idea makes more sense for these reasons:

manolo commented 7 years ago

In vaadin-notification, the overlay is imperatively and lazy created, and appended directly to the body, so as it's not necessary to find it in the consumer dom. I'm wondering if we could use one instance of the overlay for all consumers, I think is doable. Then the overlay is responsible to add/remove teleported fragments to it's content region as they are used.

jouni commented 7 years ago

I'm wondering if we could use one instance of the overlay for all consumers, I think is doable

That’s the way it’s done in FW7/8. I won’t say anything about if this would be a good idea for our new elements as well, I don’t know.