salesforce / lwc

⚡️ LWC - A Blazing Fast, Enterprise-Grade Web Components Foundation
https://lwc.dev
Other
1.59k stars 386 forks source link

LWC appears to be removing properties from DOM elements #3336

Open simonsarris opened 1 year ago

simonsarris commented 1 year ago

Hello, I am an author of the diagramming library GoJS. Recently, GoJS has been breaking in LWC for customers due to, I think, DOM modification on LWC's part. It is difficult to debug exactly what the problem is, but it seems that LWC, at least with Lightning Web Security turned on, is removing properties from DOM elements.

In GoJS we attach a _diagram property to an HTML Div and HTML Canvas:

image

This is used for example during events, like dragging and dropping from one Diagram to another, to identify drop targets. It seems like LWC with web security turned on is removing this property from the DOM elements that GoJS creates, causing the events to fail.

Is this intentional behavior on the part of LWC? Is there some way LWC can prevent this modification of the DOM elements our library creates? Or some option our clients can set so that it does not happen?

nolanlawson commented 1 year ago

@simonsarris Thanks for reaching out! Is GoJS modifying the prototype of Div and and Canvas elements, or only the instances of those elements?

jdalton commented 1 year ago

From the LWS side this is seen as a 1 sided mutation. So happening inside the sandbox and not reflected in the outer-world DOM. There are a couple of options.

1) instead of bolting on properties to DOM elements you can track metadata with them using a WeakMap with the DOM element as the key and the meta data as the value. 2) You can use the dataset API: https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/dataset

simonsarris commented 1 year ago

Only instances (and only instances that we create).

On Wed, Feb 8, 2023, 7:46 PM Nolan Lawson @.***> wrote:

@simonsarris https://github.com/simonsarris Thanks for reaching out! Is GoJS modifying the prototype of Div and and Canvas elements, or only the instances of those elements?

— Reply to this email directly, view it on GitHub https://github.com/salesforce/lwc/issues/3336#issuecomment-1423444811, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACTH23BQ2KBAVHDBTHOY73WWQ46ZANCNFSM6AAAAAAUV5BSKE . You are receiving this because you were mentioned.Message ID: @.***>

simonsarris commented 1 year ago

Thanks for the suggestions and confirmation @jdalton. We can't use (solely) dataset since we're storing references, and not just strings.

Might your sandboxing DOM manipulation break a WeakMap, too? If I create an HTMLDivElement and add it to the DOM and add an entry to my map, and the end user later fires an event over it from another DOM element, where that first HTMLDivElement is the event.target or first item of event.path, will that target === my reference? Or is that safe per your sandboxing model?

In other words, if I drag and drop between two Divs, will the PointerUp's target be the Div reference I originally created?

jdalton commented 1 year ago

event.target and event.path (as long as it's not inside a shadow root) should be fine. DOM/Event properties tend to work, custom expandos don't.

caridy commented 1 year ago

Ok, I'm confused here @simonsarris. If GoJS is evaluated inside sandbox A, and the library adds expands to DOM elements that are solely read by the library in sandbox A, there is no problem. That should work just fine IMO. The protection about expandos is between realms (meaning between sandboxes and between a sandbox and system mode). Before we look for solutions, let's try to identify the actual problem first.

You can try that in a sandbox very easily, just do:

const a = {};
document.body.firstChild._foo = a;
console.log(a === document.body.firstChild._foo); // should yield true
simonsarris commented 1 year ago

I am currently awaiting confirmation that a library build which uses a WeakMap instead of attaching properties to the Divs and Canvases really does solve the problem. If it does, that's good enough for me, and I will make a new release of GoJS.

@caridy Since I am simply the library developer, not a LWC user myself, and it is difficult to predict how my customers are using (or even importing) GoJS into their LWC apps. Some may be doing it more correctly than others.

One has set me up a sandbox with two Diagrams, and so two DIVs that hold elements:

<div id="d1"> <!-- the user creates this DIV and GoJS populates its contents -->
  <canvas></canvas>
</div>

<div id="d2">  <!-- the user creates this DIV and GoJS populates its contents -->
  <canvas></canvas>
</div>

The problem is (as far as I know) only in the dragging between Divs/Canvases, which I assume is where the sandbox is failing. What I am able to confirm by inspection is that when PointerDown on one begins on one, and PointerUp ends upon the second one, the <div> and <canvas> as the event target of the second one do not have _diagram attached as a property. Is there something the customer ought to be doing differently so that these two Divs are within the same sandbox, together?

caridy commented 1 year ago

well, we will need a lot more info here for sure, but I have few follow up questions:

  1. are you certain that the target of the event is effectively the same element? I suspect they have those two canvas on different components, and each component is using Shadow DOM, in which case, by dropping the element on another shadow will have implications in terms of what the target was, because you can't really see the actual target if it is inside another shadow. Just a hunch.
  2. did this ever worked? or it is just that they can't get it to work?
simonsarris commented 1 year ago
  1. Unfortunately I don't know a lot about their setup and from inspecting the code I don't know enough about LWS to say. I'll continue that line of inquiry with them, though depending on their setup it seems reasonable that two or more Diagrams might be in very different components on their actual app (think of a navbar with palettes to drag and drop things from, and a main Diagram as the drop target - I would assume in LWC's concept of components, these would be separate components, but maybe not.)

GoJS does work in other component frameworks, including those that use shadow DOM.

  1. They claim that with Lightning Web Security turned off, it works and has worked (since at least 2019 as far as I know), but their own requirements require them to turn this security on, so they asked me for help.
simonsarris commented 1 year ago

It appears they are hosting GoJS Diagrams in the same component. It also appears that switching from attached properties to using a WeakMap alone does not work. The target found is now a Proxy, which of course won't be found in the map.

Unfortunately, however go.js is added to the LWC app, I cannot seem to step through the code, so it's difficult for me to gather more information.

jdalton commented 1 year ago

If your library running in system mode had an API like .getData(element) then if the code in the sandbox called some code called it, them the value would be unwrapped from a proxy to an element automagically.

caridy commented 1 year ago

@simonsarris I think at this point what we need is a way to debug this. cc @manuel-jasso

@jdalton I wonder how can go.js be running in system mode? that seems very unlikely. My hunch is that closed shadows will break go.js. @simonsarris have you tried to use two closed shadow roots and drag and drop items between them using vanilla web components maybe?

simonsarris commented 1 year ago

I am sorry that I do not know enough about LWC to be of much help yet in this debugging.

In terms of the GoJS code needed, here's the simplest example of a Palette (top div) that can drag its nodes onto the Diagram (bottom div). Each has an un-styled Node that is just as line of text. Live, or as plain HTML/JS:

<script src="https://gojs.net/latest/release/go.js"></script>

<body>
  <div id="myPaletteDiv" style="width: 200px; height: 80px; border: 1px solid red"></div>
  <div id="myDiagramDiv" style="width: 200px; height: 200px; border: 1px solid black"></div>
</body>
<script>
  document.addEventListener('DOMContentLoaded', () => {
    // Create a Palette for the specified DIV HTML element
    // Can be an ID string or a reference to the HTML element itself
    myPalette = new go.Palette('myPaletteDiv');
    myPalette.model = new go.GraphLinksModel([{ key: 'DragMe' }]);

    // create a Diagram to drop onto
    myDiagram = new go.Diagram('myDiagramDiv');
    myDiagram.model = new go.GraphLinksModel([{ key: 'Alpha' }]);
  });
</script>
caridy commented 1 year ago

@simonsarris I tried to modify the example a little bit, here is my new code:

<script src="https://gojs.net/latest/release/go.js"></script>

<body>

  <div id="myPaletteDiv"></div> 
  <div id="myDiagramDiv"></div> 

</body>
<script>
function init() {  // Create a Palette for the specified DIV HTML element
  // Can be an ID string or a reference to the HTML element itself
  const sr1 = document.querySelector("#myPaletteDiv").attachShadow({ mode: 'open' });
  sr1.innerHTML = '<div style="width:200px; height:80px; border: 1px solid red;"></div>';
  myPalette = new go.Palette(sr1.firstChild);
  myPalette.model = new go.GraphLinksModel(
    [
      { key: "DragMe" }
    ]);

  // create a Diagram to drop onto
  const sr2 = document.querySelector("#myDiagramDiv").attachShadow({ mode: 'open' });
  sr2.innerHTML = '<div style="width:200px; height:200px; border: 1px solid black;"></div>';
  myDiagram = new go.Diagram(sr2.firstChildElement);
  myDiagram.model = new go.GraphLinksModel(
    [
      { key: "Alpha" }
    ]);

} // end init

init();
</script>

This is pretty much the same... but this time rather than using the divs inside the body, I'm attaching an open shadow inside those divs, create new divs inside the shadows, styling them there, and creating the palette and diagram on each shadow... and it does not work. I can only drag it in the scope of the container shadow, but not beyond. I don't know if this is what they are facing... but certain raise the question about gojs working with ShadowDOM. Note: I'm using open shadow, but with LWS you get closed shadows... so that is even more complicated.

simonsarris commented 1 year ago

In your code, if you changed sr2.firstChildElement to sr2.firstChild, it would work. Live

caridy commented 1 year ago

In your code, if you changed sr2.firstChildElement to sr2.firstChild, it would work. Live

I thought I fixed on both places, thanks for fixing that. Now, this is where things get very tricky. If I change the shadows to be closed (e.g.: .attachShadow({ mode: 'closed' })), it doesn't seem to work anymore. And this is probably because you're not longer able to identify the right target, for the drop. And this is effectively what is going on if you make this LWC and you run them with or without LWS, that is the observable change, all shadows are effectively "closed".

simonsarris commented 1 year ago

I see. So in such a case the PointerEvent.target.shadowRoot is no longer accessible (null, rather than a document fragment the library can inspect to find the Diagram DIV), and the Diagram Div is not present in the composedPath either.

I assume the point of this mode is the sandboxing - that one Div cannot be found from another (or another's events). But this is precisely what we want in a case like drag and drop. What is the canonical way to solve that?

In our simplistic example I can of course still find the outer div, but in practice I would simply want the Diagram Div to be that findable outer div, so I assume that's not very helpful.

caridy commented 1 year ago

This is the encapsulation model of the Shadow DOM, you can only traverse up, and to your siblings, you can't get inside the shadow of another element because you're effectively accessing to implementation details of it. A good example is the <input> element that has a shadow, but it is, by no means, accessible to the outside.

The canonical way to solve this for libraries is pretty much this:

Disclaimer: this is my personal opinion, this is not documented anywhere in the specs.

  1. a library that has access to 2 elements, in two distinct fragments (shadows), must track down events (using weak maps) in case the targets, and composedPath needs to be reconciled for the same event.
  2. the library can store information about the different targets, and paths observed along the way to facilitate reconciliation.
  3. the library can, at any given time, extra cached information about an event, by identity, or by association (something that can identify similar events, e.g.: keyup/keydown happening in sequence).
  4. the library can use cached information to reconciliate and understand the actual structure of the DOM across different shadows.

In other words, events will occur, and you can listen to them at the shadowRoot level (when an element is selected to be a Pallete or Diagram, you can always find out who the rootNode is (e.g.: elm.getRootNode()), and add listener at that level. You can also access the host of the root, (e.g.: elm.getRootNode().host) so you can now know where the cut out is, and how a shadow interacts with its host. Then track all kind of events that can emerge from within the shadows, to track down internally who are the actual targets on the different shadows.

simonsarris commented 1 year ago

Supposing the problem are these shadow roots, then it shouldn't be too much work for my customer to make a workaround. Below is the code you demonstrated last week, with the closed mode shadows attached, plus a workaround so that the Diagrams are able to detect each-other. This works in the basic JS version:

    function init() {
      // Create a Palette for the specified DIV HTML element
      // Can be an ID string or a reference to the HTML element itself
      const sr1 = document.querySelector('#myPaletteDiv').attachShadow({ mode: 'closed' });
      sr1.innerHTML = '<div style="width:200px; height:80px; border: 1px solid red;"></div>';
      myPalette = new go.Palette(sr1.firstChild);
      myPalette.model = new go.GraphLinksModel([{ key: 'DragMe' }]);

      // create a Diagram to drop onto
      const sr2 = document.querySelector('#myDiagramDiv').attachShadow({ mode: 'closed' });
      sr2.innerHTML = '<div style="width:200px; height:200px; border: 1px solid black;"></div>';
      myDiagram = new go.Diagram(sr2.firstChild);
      myDiagram.model = new go.GraphLinksModel([{ key: 'Alpha' }]);

      // A workaround is implemented below:
      // A list of all relevant Diagrams (at least the drop targets)
      const allDiagrams = [myDiagram, myPalette];

      // For each Palette or diagram you drag OUT of, you must extend
      // doMouseUp and doMouseMove to set potential drop targets.
      myPalette.doMouseUp = function () {
        setRealTarget(this);
        go.Diagram.prototype.doMouseUp.call(this);
      };
      myPalette.doMouseMove = function () {
        setRealTarget(this);
        go.Diagram.prototype.doMouseMove.call(this);
      };

      function setRealTarget(diagram) {
        const e = diagram.lastInput;
        for (d of allDiagrams) {
          if (d.div === null) continue;
          const root = d.div.getRootNode();
          if (root && root.host === e.event.target) {
            e.targetDiagram = d;
            return;
          }
        }
      }
    } // end init

And attached here is a full .html demo: minimalDevShadowDOM.txt

But giving this code to my LWC-using customer, it still seems to fail for them. I have little visibility into debugging, but I assume that root.host === e.event.target is still false in LWC. What else would be different from the LWC setup and a .attachShadow({ mode: 'closed' }) setup?

nolanlawson commented 1 year ago

This might not be helpful, but you may want to try testing Locker / LWS in the console:

These provide additional runtime changes that are not 100% the same as attachShadow({ mode: 'closed' }).

Off the top of my head, I believe root.host === e.event.target might be false because the two objects are distorted differently by Locker/LWS?