holoviz / panel

Panel: The powerful data exploration & web app framework for Python
https://panel.holoviz.org
BSD 3-Clause "New" or "Revised" License
4.84k stars 520 forks source link

ReactiveHTML js callback `event.target` issue in Panel v1 #5522

Open CmpCtrl opened 1 year ago

CmpCtrl commented 1 year ago

ALL software version info

Bokeh 3.2.1 Panel 1.2.1

Description of expected behavior and the observed behavior

In an older version of Panel (v0.13 as i recall) i had developed an input table with quite a few js callbacks defined in the _scripts dictionary. Many of those relied on the target field of the event object that would be passed to the callbacks and would refer to whichever element triggered the event. I recently tried updating to Panel v1.2.1 and those are now broken. The target now refers to the top most div containing a shadow DOM above whichever element triggered the event. I presume this is related to the shadow DOM which i know very little about.

Complete, minimal, self-contained example code that reproduces the issue

Here is a mildly modified JSSlidshow example from the doc's.

import panel as pn
from panel.reactive import ReactiveHTML
import param

class JSSlideshow(ReactiveHTML):
    index = param.Integer(default=0)

    _template = """<img id="slideshow" src="https://picsum.photos/800/300?image=${index}" onclick="${script('click')}"></img>"""

    _scripts = {
        "click": """
                data.index += 1
                console.log(event)
                """
    }

if __name__ == "__main__":
    pn.Row(JSSlideshow(width=800, height=300)).show()

Stack traceback and/or browser JavaScript console output

The click event.target refers to div.bk-Row where i would expect it to refer to img#slidwshow-p1005

image

mattpap commented 1 year ago

event.composedPath()[0] should be used to replace event.target, though often we would use more generic event.composedPath().includes(some_element).

CmpCtrl commented 1 year ago

After further poking around i found the related #5195 where @MarcSkovMadsen shared this info https://javascript.info/shadow-dom-events which lead me to the solution @mattpap just beat me too. Thanks for the help.

Should i close this, or is there anything that should be changed as a result? Maybe just some better documentation?

Thanks again.

CmpCtrl commented 1 year ago

A further question on this is if these callbacks can be caught inside the shadow DOM. I still don't know much about it, but that seems much more intuitive, since i think that the event.target and getElementById() would work as expected. Would that cause issues with the other context provided to the callback?

Thanks for the help

CmpCtrl commented 1 year ago

Holy cow i spent all day chasing workarounds before figuring out that this is much easier than i thought. This was my biggest complaint with Panel in the past, there is too much magic. I don't really know how to explain what i mean other than that its very difficult for me to use and diagnose because i don't have any idea how it accomplishes some seemingly very complex things without seeming to get enough input to accomplish them. For instance, i had read but never grasped that <node> is available to the js functions in that you can reference nodes in the DOM by id as if they were an input argument. In the documentation its presented as an argument to the function, and I knew that the element id's get an identifier appended, and i couldn't fathom how that would be accomplished so i never even bothered to try it, it just didnt click. It wasn't until i was chasing bugs and inspecting the generated scripts that i stumbled onto the magic which happens when panel appends code to the beginning of my script to define a variable using the document.getElementById() appending the appropriate identifier to the node id. Finally it clicked and that becomes quite a useful feature now that i have some idea of how that magic works. Furthermore this clued me into how to deal with the shadowDOM issues that cropped up from me relying heavily on the document.getElementById() in my scripts. Clearly this is on me for not understanding it, but i would encourage a little different tact in the documentation to not expect the user to take so much on faith. Sharing a 1 line explanation of how that feature works would make it so much easier for me to learn how to use it.

Anyway, just to document my findings for anyone else who comes across similar issues. First, to find the target of an event replace event.target with event.composedPath()[0].

Then, since the shadowDOM separates the scope of which nodes are visible to the function, you need to replace document.getElementById() with view.shadow_el.getElementById() to get elements in the same scope as where an event is triggered.

I think generally you can avoid using the getElementById() if the node you're trying to access is static (you can just magically treat it as a variable, who knew??), but i think for choosing a node programmatically, you still need it. In my case i name my table cells with a row and column, and then sometimes need to select a different cell programmatically.

Another somewhat unrelated gotcha in this update was that the identifier that gets appended to each element id changed from a 4 digit numerical to 5 digits with a letter. Again, that's not needed normally but seems to be for programmatically selecting a node as i am doing.

Thanks for the help,

philippjfr commented 1 year ago

Really appreciate your feedback @CmpCtrl, it's very valuable. @MarcSkovMadsen hope you can capture some that that in your updated docs.

ahuang11 commented 1 year ago

Awesome feedback!

I asked ChatGPT for examples of what you mentioned to better understand; maybe it'll help others too, if it's accurate.

  1. Too Much Magic:

    • Framework Behavior: A developer uses a framework that automatically connects to a database without any configuration.
    • Example: The developer writes db.connect(), and the framework automatically chooses the correct database, server, and credentials. If there's an issue, the developer might be confused since they didn't specify any of those details.
  2. and DOM Reference:

    • Panel's Behavior: Directly reference DOM nodes by their ID in JavaScript functions.
    • Example:
      <div id="myElement">Hello</div>

      In Panel, a function might look like:

      function changeText(myElement) {
      myElement.textContent = "Changed!";
      }
  3. ShadowDOM:

    • Web Component Behavior: Encapsulate styles and markup.
    • Example:
      <my-web-component>
      #shadow-root
       <div id="innerElement">Inside Shadow DOM</div>
      </my-web-component>

      To access innerElement, you'd use:

      let el = myWebComponent.shadowRoot.getElementById("innerElement");
  4. Event Targeting:

    • ShadowDOM Behavior: Events might not directly reference the inner element.
    • Example:
      <my-web-component>
      #shadow-root
       <button id="innerButton">Click me</button>
      </my-web-component>

      If you add an event listener to the button, you might use:

      function handleClick(event) {
      let actualTarget = event.composedPath()[0];
      console.log(actualTarget.id);  // Outputs: "innerButton"
      }
  5. Element ID Identifier:

    • Panel's Behavior: Appends an identifier to element IDs.
    • Example: In a previous version:
      <button id="btn1234">Old Button</button>

      In a new version:

      <button id="btn1234a">New Button</button>

These examples illustrate the behaviors and solutions the user described in their feedback about the Panel framework.

philippjfr commented 1 year ago

Should we simply override the target on the event before it is passed to the user, i.e. internally we would do:

  event.target = event.composedPath()[0];