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

svg <use> tag frequently breaks on redraw in Chrome #2014

Closed
soulofmischief opened this issue Nov 7, 2017 · 22 comments
Closed

svg <use> tag frequently breaks on redraw in Chrome #2014

soulofmischief opened this issue Nov 7, 2017 · 22 comments
Projects

Comments

@soulofmischief
Copy link
Contributor

soulofmischief commented Nov 7, 2017

Back already with another head-scratcher!

When using Chrome and dynamically rendering svgs with a <use> tag that references an ID in an external svg spritesheet document, some svgs elements that remain between redraws disappear.

Expected Behavior

Each svg should display as normal. The behavior is correct in Firefox but not in Chrome.

Current Behavior

Seemingly at random, svg elements that remain between redraws suddenly disappear. Upon page refresh (with state saved in history.state), the elements reappear, but subsequent redraws trigger the bug again.

Possible Solution

You might need to familiarize yourself with the provided code below in order to understand the following.

#1297 likely provides a workaround, however because the svgs are not held in a separate document, they are not able to be cached.

Furthermore, I'm not certain that referencing a javascript object, say, 50 times in a page (ex. icons), is comparable in performance to referencing the same element by ID with use 50 times.

If these two problems could be addressed, that would make encoding the svgs as mithril components viable, however it is still more complicated to set up a build pipeline. Currently I have a script that watches a folder for changes and automatically creates a minified document containing each svg icon as a symbol. Reworking this to output a valid mithril component that displays the correct svg based on a provided argument, or a page with several individual components, is very convoluted and could also hurt my build times.

I have narrowed down the source of the problem a good bit. Providing a static string, instead of dynamically inserting the string, in the href attribute of the use tag still produces this bug. Some of the svgs render and some don't, even though they reference the same ID.

Curiously, when I remove the map function and just render every item instead of filtering based on their value, each svg displays just fine even when clicking on a different result.

So it is not an issue with resolving IDs, and must be an issue with how dynamic rendering is handled when svg and/or use elements are involved. I'm not sure how this relates back to Chrome but I'm not surprised it's browser-specific because the Chrome dev team loves to break shit. For all I know, this is purely a Chrome bug and mithril's implementation is correct.

Steps to Reproduce (for bugs)

For the sake of clarity, I have greatly simplified the following components.

The relevant components include a container, a component containing the svg element, and the model Items that contains information about each item.

vnode.attrs.items is formatted as

{
  item1: true / false,
  item2: true / false,
  item3: true / false, 
  ...
}

and Items is formatted as

[
  item1: {
    name: <name>,
    icon: '/spritesheet.svg#sprite-<name>'
  },
  ...
]

The indexes of both vnode.attrs.items and Items are the same for each item, and thusly the container converts vnode.attrs.items to an array of keys and values with Object.entries and loops through each entry and if the value is true, retrieves the relevant information from Items.

Container:

function MyContainer() {
  return {
    view: ( vnode ) => (
      <div>
        {
          Object.entries( vnode.attrs.items )
          // Render the component if the value is true
            .map(( x, i ) => (
              x[1] ?
                <MyComponent
                  key={ i }
                  name={ Items.list[ i ].name }
                  icon={ Items.list[ i ].icon }
                />
                : undefined
            ))
        }
      </div>
    )
  }
}

Component:

function MyComponent() {
  return {
    view: ( vnode ) => (
      <div>
          <svg class={ 'icon' }>
            <use href={ vnode.attrs.icon }/>
          </svg>

          <span>
            { String( vnode.attrs.name )}
          </span>
      </div>
    )
  }
}

Context

I have a list of search results. I am pulling a list of attributes from a document that changes whenever a result is clicked. This document is pulled in a parent component and only the attributes are passed through (as items). These attributes have associated full names (item1 -> "Item 1") and icons. The icons are stored as strings referencing the appropriate path and id of the svg symbol, and the appropriate svg is loaded from the external spritesheet on render.

Your Environment

  • Version used: 1.1.5
  • Browser Name and version: Chrome 61.0.3163.100 (64-bit)
  • Operating System and version (desktop or mobile): Fedora 26
  • Webpack + Babel + JSX

I know this bug is a bit complicated so I am happy to provide further context upon request.

@dead-claudia
Copy link
Member

Check in the dev tools. Could you verify they exist in the DOM itself? As long as they appear properly in the DOM, there's likely little Mithril could do about it.

@soulofmischief
Copy link
Contributor Author

Well that is an interesting bit I should have mentioned. They do appear in the DOM, but if I modify the element in any way, i.e. adding a space to the class attribute, the svg icon reappears. So you're saying this is likely a Chrome bug?

@dead-claudia
Copy link
Member

@soulofmischief Most likely. As long as it's present in the DOM and working as intended there, there's little Mithril can do. So go ahead and file a bug there.

@StephanHoyer
Copy link
Member

I experience all sorts of strange bug with SVGs, Chrome and mithril. Sometimes setting proper namespaces fixed the issues. Also clean up SVG with tools like svgo helps a little. We also avoid using svg-maps with use-attribute but simply inlined the svg with m.trust.

My impression still is, that it's mostly a chrome issue.

@pygy
Copy link
Member

pygy commented Nov 8, 2017

@StephanHoyer many namespace-related bugs have been fixed in Mithril since v1.0 was released, so this may no longer be needed...

@soulofmischief if you want to keep on using a sprite, you could perhaps try to interact with the DOM element from the oncreate and onupdate hooks, like you've done manually.

@soulofmischief
Copy link
Contributor Author

soulofmischief commented Nov 10, 2017

@pygy I have spent some more time troubleshooting this but remain stumped. I tried all sorts of convoluted workarounds from within mithril. Interestingly, manipulating the real DOM from lifecycle hooks isn't working either. Only when I make changes from Chrome Dev Tools.

Tomorrow I will file a bug report with Chrome. As I don't expect a quick fix from them, one possible interim solution would be to force the persisting icons to be destroyed and recreated on each state change, as it seems Chrome's diffing algorithm in combination with mithril's diffing algorithm causes any icons that are shared between states to be recycled, even if they change position in the DOM.

Any insight into how I could do this? I've tried a few things to no avail. Even if I successfully destroy and redraw the icons from within mithril by rendering a variable that clears and then sets itself after a short delay inside of lifecycle hooks, this seems to ultimately remain opaque to Chrome, who still thinks they were always there.

@StephanHoyer thank you for that link.

I've implemented your sprite system but I have a few issues with it. Namely, I cannot find a way to modify the svg file after wrapping it in a comment. The only option I can see is to assign a unique identifier, track it, and use it to pluck the element from the DOM. Only then I am back to editing the DOM directly and am not working through mithril. That feels really dirty and doesn't do a good job of separating concerns, nor is it idiomatic.

I need to inject classes into the svg. I could manually add the classes to each svg, or create a build system that automatically inserts the classes after running svgo, but that doesn't solve dynamic styling. One of the major reasons I use this sprite system is that it allows me to dynamically style the size, fill and stroke. This is crucial for the kind of UIs I develop.

There are also cases where I would like to animate or otherwise interact with the elements contained in the svg, for things like click events. This interactivity is another reason I use my system. It can only be done with inlining your svg or using use with an href set to an external svg file.

Having an external file also allows for efficient caching that is independent of the rest of the app, the importance of which scales with the amount of icons in use and the frequency in which you release updates.

I guess I can live without caching, but I can't live without dynamic styling and interactivity. Your solution, while elegant, is only really viable for static icons and not icons that are part of a rich and interactive UI. It also requires updating icons.js any time you add or remove an svg, instead of the system I currently have where everything is handled for you after you save the image in your vector program.

I will continue to play around with this.

@pygy
Copy link
Member

pygy commented Nov 10, 2017

@soulofmischief I'm not used to work with SVG so it took me a while to set things up... starting from this what do you need to change to make it fail?

You can fork this gist to get cracking? bl.ocks.org caches the gists so you must put the full hash as above to see your latest version you don't get things right immediately (click "revisions", then copy the URL f the "view" button next to "index.html" to get the proper link).

@soulofmischief
Copy link
Contributor Author

soulofmischief commented Dec 8, 2017

This is how I ended up working around the bug. I have a component and a list of .js files each containing an svg. I made use of a small lodash module get which allows you to get a value with a query like 'path.to.value'. I currently create my svg .js files by hand, but automating it should be trivial.

This method should be close enough in performance to working with use but I haven't tested it under high load. It allows me to style, animate, rig events, and pass through attributes to svgs, and returns the svg directly instead of an svg wrapped in a div. With a build engine like webpack, the svg files could be bundled separately as a one bundle or as a series of smaller sets which will allow the client to cache them even when the application updates. I think that checks off everything for me.

The component looks like this in action

import { Icon, classList } from 'Components/Icon'

<Icon name='user.logout' class={ `${classList.stroke.red} ${classList.fill.none}` }>

It works like this

index.js

import get from 'lodash/get'
import { classList } from './classList'
import { icons } from './icons'
import style from './icon.scss'

export function Icon() {
  return {
    view: ( vnode ) =>
      // Return an svg icon from provided string and prepend the icon class
      get( icons, vnode.attrs.name )(
        // Pipe vnode.attrs to the SVG element and add default class
        Object.assign(
          vnode.attrs,
          {
            class: `${ style.icon } ` + (
              // Check for both 'class' and 'className'
              vnode.attrs.class
              || vnode.attrs.className
              || ''
            ),
            // We don't want to keep both
            className: undefined
          }
        )
      )
  }
}

export { classList }

icons.js

const user = {
  logout: require( './user/logout' )
}

export const icons = {
user,
}

user/logout.js

import m from 'mithril'

module.exports = ( attrs ) => (
  <svg viewBox="0 0 100 100" { ...attrs }>
    {/* svg data */}
  </svg>
)

classList.js

import style from './sprite.scss'

export const classList = {
  icon: style.icon,
  fill: {
    none: style.fill__none,
    red: style.fill__red,
  },
  stroke: {
    none: style.stroke__none,
    red: style.stroke__red,
  },
}

icon.scss

.icon {
  height: 100%;
  width: 100%;
}

.fill {
  &__none { fill: none !important }
  &__red { fill: #f00 }
}

.stroke {
  &__none { stroke: none !important }
  &__red { stroke: #f00 }
}

Do you think it's worth making a note in the documentation recommending an approach like this for dealing with svgs in mithril to save others future headaches?

@lukaszkups
Copy link

It may relate to chrome - I have similar issue with dynamically updating href param for <use> svg tag while working with Vue.js - everything works fine on firefox, but not chrome :/

@dead-claudia
Copy link
Member

@mrmnmly It's most likely a Chrome bug. If I find any time to actually return to the project, I might see to investigate whether 1. this issue persists in the current tree since @pygy ripped out pooling, and 2. if so, whether deferring any particular change(s) until the tree is live will help.

I don't see that happening very soon, however. (Other @MithrilJS/collaborators might be able to remedy this faster.)

@dead-claudia dead-claudia added this to Needs triage in Triage/bugs Oct 28, 2018
@dead-claudia
Copy link
Member

Closing. Please file a bug against the Chrome browser if this remains a problem.

Triage/bugs automation moved this from Needs triage to Closed Nov 7, 2018
@soulofmischief
Copy link
Contributor Author

I have discovered a number of svg-related bugs with Chrome since I filed this issue. I've figured out workarounds for the most part but it's definitely a Chrome bug.

@josephwegner
Copy link

@soulofmischief did you open a Chrome issue for this? I'm running into it pretty hard, and am interested in tracking the resolution from the chromium team. I couldn't find an open issue that seemed to match the descrition here.

@dead-claudia
Copy link
Member

@josephwegner You could yourself at https://crbug.com/new. 👍

@soulofmischief
Copy link
Contributor Author

@josephwegner I hadn't had the time to sit down and figure out enough about the issue in order to make a useful bug report.

@josephwegner
Copy link

josephwegner commented Nov 21, 2018

OK, cool - I'll create an issue today. I made a vanilla JS fiddle that seems to reproduce the issue reliably for me: https://jsfiddle.net/st8h1v6d/3/

@rubenlg
Copy link

rubenlg commented Nov 3, 2019

@josephwegner Did you ever file that bug against Chrome?

@rubenlg
Copy link

rubenlg commented Nov 3, 2019

FYI: I just filed a more generic SVG bug, for this: http://jsfiddle.net/oncjgmLb
It's not just about , any dynamically generated SVG seems broken, and the fix (rewriting innerHTML can sometimes be really nasty, since you kill all event listeners.

Here is the bug: http://crbug.com/1020893

Not sure why everything is green and without details. I guess it's some anti-abuse protection until someone verifies that it's not spam?

@rubenlg
Copy link

rubenlg commented Nov 3, 2019

Update: Firefox has the same issue, reported here: https://bugzilla.mozilla.org/show_bug.cgi?id=1593525

@rubenlg
Copy link

rubenlg commented Nov 4, 2019

The folks at Mozilla clarified that it's not a bug. SVG stuff can't be created with document.createElement, you have to use document.createElementNS. Here is a working demo:

http://jsfiddle.net/akrztmus/

@dead-claudia
Copy link
Member

@rubenlg Mithril handles namespaces of such elements correctly, BTW. It's also a separate issue from this.

As for the original bug with <use> specifically, I'm only instating a workaround once it's clear 1. there is a workaround (I'm not completely sure there is), 2. there's a self-contained repro (there isn't one currently), and 3. it affects enough users I need to urgently work around it (it's clearly pretty small).

@rubenlg
Copy link

rubenlg commented Nov 4, 2019

Thanks @isiahmeadows. I actually don't use Mithril, just ended up in this thread accidentally, because I had the same problem with <use>.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

No branches or pull requests

7 participants