Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Is there a bug with dynamically changing svgs? #2886

Open
EverettMcKay opened this issue Oct 3, 2023 · 15 comments
Open

Is there a bug with dynamically changing svgs? #2886

EverettMcKay opened this issue Oct 3, 2023 · 15 comments
Assignees
Labels
Type: Bug For bugs and any other unexpected breakage

Comments

@EverettMcKay
Copy link

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.

@EverettMcKay EverettMcKay added the Type: Bug For bugs and any other unexpected breakage label Oct 3, 2023
@boazblake
Copy link
Contributor

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

@EverettMcKay
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

CreaturesInUnitards commented Oct 8, 2023 via email

@EverettMcKay
Copy link
Author

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
Copy link
Member

dead-claudia commented Oct 8, 2023

@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
Copy link
Author

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
Copy link
Contributor

boazblake commented Oct 9, 2023

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
Copy link
Member

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
Copy link
Author

EverettMcKay commented Oct 9, 2023

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
Copy link
Member

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 whatwg/html#5484 (title says iframes, but it also impacts animations). Try seeing if using a shadow DOM works per whatwg/html#5484 (comment) - if so, then that's most certainly your issue.

@EverettMcKay
Copy link
Author

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
Copy link
Author

EverettMcKay commented Dec 6, 2023

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
Copy link
Member

pygy commented Dec 6, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug For bugs and any other unexpected breakage
Projects
None yet
Development

No branches or pull requests

5 participants