MithrilJS / mithril.js

A JavaScript Framework for Building Brilliant Applications
https://mithril.js.org
MIT License
13.99k stars 926 forks source link

Is there a bug with dynamically changing svgs? #2886

Open EverettMcKay opened 12 months ago

EverettMcKay commented 12 months ago

I have an array of numbers (assume 1 - 5) that is mapped to svgs for display. When the user clicks on a number, the svg changes . However, the wrong svg is often updated (so, clicking on 3 will update the svg for 5 instead of 3).

I suspect this is a mithril svg update problem. Here's why: 1) Using the debugger and console.log show the code is behaving as expected. 2) I have an option to display some html markup instead of the svgs, and that works as expected. 3) The bug only appears after interaction/update--the initial display of the svg array is always correct.

I've tried hacking the key for the containing div, but that appears to make no difference.

Is this a known problem? Is there a work around?

I am using the current versions of Chrome and Edge.

boazblake commented 12 months ago

@EverettMcKay Can you provide a sample code in flems that shows this?

EverettMcKay commented 11 months ago

I tried to come up with a simplified version of what I am doing:

const options = [0, 1, 2, 3, 4, 5, 6];
const state = [0, 0, 0, 0, 0, 0, 0];

const getIcon = (i: number): object => {
  const n = i % 3;
  switch (n) {
    case 1:
      return (
        <svg width="100" height="100" viewBox="0 0 100 100">
          <path
            d="M50,10 A40,40 0 1,1 50,90 A40,40 0 1,1 50,10"
            fill="#0000ff"
          />
        </svg>
      );
    case 2:
      return (
        <svg width="100" height="100" viewBox="0 0 100 100">
          <path d="M50 10 L90 90 L10 90 Z" fill="#ff0000" />
        </svg>
      );
    case 0:
    default:
      return (
        <svg width="100" height="100" viewBox="0 0 100 100">
          <path d="M10 10 L90 10 L90 90 L10 90 Z" fill="#00ff00" />
        </svg>
      );
  }
};

const HomePage = (): any => ({
  tag: "home",
  attrs: () => {},
  state: () => {},
  view: () => (
    <div>
      {options.map((i: number) => (
        <div
          style="display:inline-block;"
          onclick={() => {
            state[i] = (state[i] + 1) % 3;
          }}
        >
          {getIcon(state[i])}
        </div>
      ))}
    </div>
  ),
});

m.route(document.body, "/home", {
  "/home": HomePage,
});

The only problem: this sample code works exactly as expected. I will need to spend some quality time to figure out how my code is different. (I fully expected this to repro the problem.)

In the meantime, any tips on how to figure this out would be greatly appreciated.

CreaturesInUnitards commented 11 months ago

This is almost certainly a keying issue — try passing a unique key to each div in your map, right next to style & onclick.

https://mithril.js.org/keys.html

EverettMcKay commented 11 months ago

Thanks Scott. I am definitely using keys and it's the first thing I tried. My keys look like this:

key =key${i}-${icon}-${state)};

Where i is the index of the icon in the array, icon determines the svg to display, and state is the current user input that this is based on. You would think that would cover all bases.

The console trace shows that these keys are exactly as expected. Unfortunately, the browser Elements tab doesn't show keys in the markup.

Here is the overall structure of how I am using the keys (in the actual code):

   <div style={getIconStyle(xxx)} key={key} onclick=(handler is here) >
      <div style={backgroundStyle}>{getIcon(xxx)}</div>
     </div>

So, the key is where you would expect it. (Right?)

CreaturesInUnitards commented 11 months ago

mmm. Try creating a unique id field for the actual source objects and just use that. Relying on array index is a footgun. Sent from my iPhoneOn Oct 8, 2023, at 11:45 AM, EverettMcKay @.***> wrote: Thanks Scott. I am definitely using keys and it's the first thing I tried. My keys look like this: key = key${i}-${icon}-${state)}; Where i is the index of the icon in the array, icon determines the svg to display, and state is the current user input that this is based on. You would think that would cover all bases. The console trace shows that these keys are exactly as expected. Unfortunately, the browser Elements tab doesn't show keys in the markup. Here is the overall structure of how I am using the keys (in the actual code): <div style={getIconStyle(xxx)} key={key} onclick=(handler is here) >

{getIcon(xxx)}
 </div>

So, the key is where you would expect it. (Right?)

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you commented.Message ID: @.***>

EverettMcKay commented 11 months ago

I'm not sure what to do, then. I have an array of 5 icons, 1 - 5. So, the index is the best way to distinguish them. Icon 1 is always icon 1, etc.

Upon further testing, I've noticed something that might help:

The bug boils down to that fact that no matter which icon I click on, icon 5 is the one that gets updated. So, if I click on icon 1, the key is right, the icon is right, but for some reason icon 5 is the one that changes. So, at least icon 5 works...

I've tried different keys (and have confirmed are different and change when the state changes) and even removing the keys, but the result is always the same.

So, for whatever reason, the DOM change is happening to the last icon in the array.

dead-claudia commented 11 months ago

@EverettMcKay

Thanks Scott. I am definitely using keys and it's the first thing I tried. My keys look like this:

key = key${i}-${icon}-${state)};

Use the option name as your key. In your simplified code, that would be i, but in your real code might look like option, option.id, or option.name. That's the "source object" that @CreaturesInUnitards is referring to.

If the key is the object identity, add such a property (can just be an ID counter or symbol) that can be used as a property key (Mithril tracks them as property keys internally).

Edit: elaborate

EverettMcKay commented 11 months ago

Thanks, Claudia. I understand the general case, but in this specific case, the option names are literally 1 - 5. (The options are used to give a 1 - 5 score.)

boazblake commented 11 months ago

The bug boils down to that fact that no matter which icon I click on, icon 5 is the one that gets updated. So, if I click on icon 1, the key is right, the icon is right, but for some reason icon 5 is the one that changes. So, at least icon 5 works...

So, for whatever reason, the DOM change is happening to the last icon in the array.

If it's always returning the last item in the array then check to make sure your view is not iterating over the icons and always returning the last event handler. Or the event handler code uses the correct icon identifier.

Personally I found it helps to simplify the logic in the view and move that complexity into the closure of the component / into a shared file if needed. Then you can map over you list of icons and apply a transformation on them into a data structure that you can use in the view handler.

dead-claudia commented 11 months ago

Thanks, Claudia. I understand the general case, but in this specific case, the option names are literally 1 - 5. (The options are used to give a 1 - 5 score.)

@EverettMcKay Then those are the names to plug into your key:. Nothing wrong with the names being numbers.

If that's not working, you'll need to share code that reproduces the problem.

EverettMcKay commented 11 months ago

Since the easy attempt to repro the problem (above) didn't work, I will need to add more to it until the problem appears.

But I would like to explore one last idea: If the problem were key related, I'm pretty sure I would have solved it long ago. I am not able to repro the problem with my existing code if I'm not using svgs. So, if replace < svg >[some icon]< /svg > with < p >{${i}, ${iconID}}< /p >, poof, problem disappears and it works exactly as expected.

Seems like an important clue that I don't know what to do with (and my reason for the title question.)

dead-claudia commented 11 months ago

Since the easy attempt to repro the problem (above) didn't work, I will need to add more to it until the problem appears.

But I would like to explore one last idea: If the problem were key related, I'm pretty sure I would have solved it long ago. I am not able to repro the problem with my existing code if I'm not using svgs. So, if replace < svg >[some icon]< /svg > with < p >{${i}, ${iconID}}< /p >, poof, problem disappears and it works exactly as expected.

That's just a sign you probably need keys.

I still want to see self-contained code that repros your problem, though. (And this search also helps tremendously in figuring out if it's a bug in your code or a bug in the framework.)

Also, I have a sneaking suspicion you may be getting bit by https://github.com/whatwg/html/issues/5484 (title says iframes, but it also impacts animations). Try seeing if using a shadow DOM works per https://github.com/whatwg/html/issues/5484#issuecomment-620481794 - if so, then that's most certainly your issue.

EverettMcKay commented 11 months ago

A quick update: I need to support email HTML (which is a bit dodgy and doesn't always support svg tags), so I changed the code to this:

{target === email ? <img src="http://whatever" /> : <svg ... />}

In this case, everything works as expected when the target is email.

I have many reasons to prefer using the svgs, but this test again confirms that it's an svg refresh problem.

EverettMcKay commented 9 months ago

I spent a few more hours on this today to try different approaches. All failed. Again, the problem is that for svgs, the shadow dom isn't properly refreshing propertly when you change an SVG dynamically.

pygy commented 9 months ago

Could you post an example that doesn't work so that we may toy with it too?

Here's a flems with your previous example converted to plain jsx since flems either supports ts or jsx but not tsx.