lit / lit-element

LEGACY REPO. This repository is for maintenance of the legacy LitElement library. The LitElement base class is now part of the Lit library, which is developed in the lit monorepo.
https://lit-element.polymer-project.org
BSD 3-Clause "New" or "Revised" License
4.49k stars 319 forks source link

[rc5] addEventListener not "hearing" events in sub-elements #486

Closed moderndeveloperllc closed 5 years ago

moderndeveloperllc commented 5 years ago

Description

Attempting to add event listener to nested elements never catches the event - unless you attach to document.body. This seems to be contrary to the examples shown in the documentation that are attaching to this in firstUpdated.

Live Demo

Line 58 in https://stackblitz.com/edit/lit-element-example-erqqcd?file=index.js (Stackblitz is being a little weird and not wanting to load index.js as a script sometimes.)

Steps to Reproduce

  1. Create my-element
  2. Add additional custom element in the rendered template of my-element.
  3. Create div in the sub-element with an ID.
  4. Attempt to add an addEventListener to sub-element's tag -->

Expected Results

Listener catches the event

Actual Results

Event listener either not attached, or event not caught.

Versions

moderndeveloperllc commented 5 years ago

I've found that on stackblitz, when you open the demo, use the Open in New Window feature and it will load properly to show the issue.

Edit: FWIW, trying to use @click on the nested element doesn't work either.

daKmoR commented 5 years ago

hey,

this seems more like a timing issue on when you actually trigger your element.

Also, your stackblitz seemed to have some sort of problem as even after forking I couldn't get it to run. Anyways I copied some stuff around and changed the trigger of the toast code from

          window.addEventListener('WebComponentsReady', async () => {
            const toast = document.querySelector('wpm-notify');
            toast.message = new Error('Test error');
            toast.additionalInfo = 'More info on your error';
            await toast.updateComplete;
            toast.show();
          });

to this

      function triggerToast() {
        const toast = document.querySelector('my-element');
        toast.message = new Error('Test error');
        toast.additionalInfo = 'More info on your error';
        toast.updateComplete.then(() => {
          toast.show();
        });
      }

      // workaround for stackblitz
      if (customElements.get('my-element')) {
        triggerToast();
      } else {
        customElements.whenDefined('my-element', () => {
          triggerToast();
        });
      }

as far as I can tell this works now... however I'm not sure what your really expected to work her... if you click on Element loaded - click me to close it now closes?

If you want the "close" button in the notification to actually do anything then it becomes a little tougher as this is a polymer template

        <template>
          <div class="default-slot">
              <h3>${this.message}</h3>
              ${this.additionalInfo.length ?
          html`<p>${this.additionalInfo}</p>` :
          html``}
          </div>
          <div class="actions-slot">
              <vaadin-button theme="primary" slot="actions" id="actionButton">${this.actionText}</vaadin-button>
          </div>
        </template>

I am surprised that this.message even works there... I assume lit will not be able to change it? also you can not simple do @click there...

overall I think using lit for this polymer template of the message will not work out nicely - if you want to use it you better create the message via js as in this example https://vaadin.com/components/vaadin-notification/html-examples/notification-basic-demos#simple-notification

PS: https://stackblitz.com/edit/tcmb9m?file=index.html

moderndeveloperllc commented 5 years ago

@daKmoR Actually the this.message part works fine. I’ll check the workaround so the example works on Stckblitz better. It works fine in a regular environment, but I do need to show the issue for others.

The issue is with the listener that fires the close() function (line 58). If I attach to the button or notification element, it doesn’t work. If I attach to document.body, it does. That doesn’t seem correct, but maybe it is due to how Shadow DOM works.

daKmoR commented 5 years ago

If you want the "close" button in the notification to actually do anything then it becomes a little tougher as this is a polymer template

in other words you can't really do anything there with lit-html... this template gets restamped to the body by vaadin-notification... and in polymer2 templates I don't think there was a way to add events via the template - however I don't really know it's a long time ago...

tlouisse commented 5 years ago

Hi,

I think the problem is that the rendered notifcation and the host <wpm-notify> are siblings in the DOM:

screen shot 2019-01-27 at 19 54 27

. The click handler on <wpm-notify> is thus never reached, as it doesn't bubble up from a child. Since document.body encapsulates all dom nodes on the page, the listener is reached here.

Looking at the vaadin docs (https://vaadin.com/components/vaadin-notification/html-examples/notification-basic-demos), the <template> probably only works with either static content or content rendered as a Polymer template.

This would be an example that solves the problem and defines all templates via lit-html: https://stackblitz.com/edit/lit-element-example-a4vmb4?file=index.js

web-padawan commented 5 years ago

This is actually not a supported use case, at least for Vaadin components using <template> tag. These Polymer templates should not be used with lit-html, we have a different concept instead.

We called if renderer functions and all the components using <template> tag already support them, so let me link several glitches with the code examples:

Note: this comment will be updated, as I will go through other components tomorrow, and verify if the glitches work with lit-elemrnt RC, then will add corresponding links to the list above.

moderndeveloperllc commented 5 years ago

@web-padawan Thanks for the clarification. No bugs, just my misuse of the components. Feel free to edit your comment, but I'll close this out for now.

Alos commented 3 years ago

It looks like if you render a mwc-button button inside a vaadin-grid using something like:

return render(
        html`<mwc-button
          outlined
          data-record="${rowData.id}"
          label="Click me"
          @click="${this.doTheThing}"
        ></mwc-button> `,
        root
      );

will not work, as the render function is in a different scope. Inside the @click section this does not point to the component that has the doTheThing() function.

This forces you to have to bind the function like this:

this.boundMyRenderFunctionWithTheButton = this.assignmentRenderer.bind(this);

I didn't see this in the example initially and caused be a few issues. I hope this helps others.

justinfagnani commented 3 years ago

@Alos if you're doing your own render() call, you'll need to pass in the event context with options to render. See https://lit-html.polymer-project.org/api/modules/_lit_html_.html#render and the RenderOptions type: https://lit-html.polymer-project.org/api/interfaces/_lit_html_.renderoptions.html

return render(
        html`<mwc-button
          outlined
          data-record="${rowData.id}"
          label="Click me"
          @click="${this.doTheThing}"
        ></mwc-button> `,
        root,
        {eventContext: this}
      );
Alos commented 3 years ago

Thanks @justinfagnani! I sort of got it to work. The issue is that inside the render function, this refers to the vaadin-grid-column. So it didn't actually work as I intended.