Error when removing comment in large SVG file with image

[NightFurySL2001]

When parsing this svg file with topicosvg, an error occured: uni7cb9

Traceback (most recent call last):
  File "d:\Desktop\coding\svg2font\svgbuilder.py", line 209, in build_font_thread
    logs = build_font(
  File "d:\Desktop\coding\svg2font\newfontbuild.py", line 300, in build_font
    glyph, glyphadv = getOutlineFromSVG(svg_fulltext, metrics.upm, metrics.ascender, picoNormalize)
  File "d:\Desktop\coding\svg2font\newfontbuild.py", line 101, in getOutlineFromSVG
    picosvg.topicosvg(inplace=True, allow_text=True)
  File "D:\Desktop\coding\svg2font\kivy_venv\lib\site-packages\picosvg\svg.py", line 1352, in topicosvg
    self.remove_comments(inplace=True)
  File "D:\Desktop\coding\svg2font\kivy_venv\lib\site-packages\picosvg\svg.py", line 1023, in remove_comments
    el.getparent().remove(el)
AttributeError: 'NoneType' object has no attribute 'remove'

This seems to be due to the comment on the top of SVG: <!-- Generator: Adobe Illustrator 23.1.0, SVG Export Plug-In . SVG Version: 6.00 Build 0) -->

However, interestingly, when the image xlink:href and subsequent data is removed in SVG file, topicosvg is able to work correctly.

Using this Python code to remove invalid element like the image that contains the xlink:href before parsing with topicosvg doesn't help:

    picoerrors = picosvg.checkpicosvg()
    if picoerrors:
        for i in picoerrors:
            # skip if not image and style, prefilter
            if not re.search(r"/style\[\d+\]$", i) and not re.search(r"/image\[\d+\]$", i):
                continue

            # remove style and image elements
            for context in picosvg.breadth_first():
                if re.search(r"/style\[\d+\]$", context.path) or re.search(r"/image\[\d+\]$", context.path):
                    context.element.getparent().remove(context.element)
            break # only need remove once
    picosvg.topicosvg(inplace=True, allow_text=True)

[rsheeter]

Ty for reporting. Probably rather than trying to remove comments ourselves we should leverage XMLParser(remove_comments=True), https://lxml.de/apidoc/lxml.etree.html#lxml.etree.XMLParser

[rsheeter]

^ might be wrong; when I add a comment above the root to a simple test file nothing breaks. If I run your file directly I get Unable to convert to picosvg: BadElement: /svg[0]/style[0],BadElement: /svg[0]/image[0]. If I manually delete those elements then picsvg succeeds.

[NightFurySL2001]

If I run your file directly I get Unable to convert to picosvg: BadElement: /svg[0]/style[0],BadElement: /svg[0]/image[0].

I actually forgot to test this.... 😅 I do know BadElement should pop up in this scenario, but the given AttributeError caught me off hand. That is why I am trying to remove the style and image element before hand.

After a bit more jiggling, I pinpoint the source of problem: inplace=True cause the AttributeError when removing comments. Trying to remove the image element with the code at https://github.com/googlefonts/picosvg/issues/297#issue-1739133674 before using topicosvg doesn't work.

Sample:

from picosvg.svg import SVG
picosvg = SVG.parse("uni7cb9.svg")
print(picosvg.topicosvg(inplace=True).tostring())

I don't think you need to do a pull request on #298, since for my use case I don't actually have control over the SVG input file, which is a minor use case. (if you are interested on what I am trying to do, it's now available at https://github.com/NightFurySL2001/SVG2FontBuilder , which essentially expose picosvg to raw real world SVG inputs....)

[rsheeter]

https://github.com/NightFurySL2001/SVG2FontBuilder , which essentially expose picosvg to raw real world SVG inputs....)

Just in case you haven't seen it, https://github.com/googlefonts/nanoemoji does a somewhat similar job.

[NightFurySL2001]

To be honest, I have not been successful at making nanoemoji works. It either crashes or error out.

[anthrotype]

oh that's a bummer, please file issues, happy to take a look

[anthrotype]

inplace=True cause the AttributeError when removing comments.

@rsheeter did we also fix that?

[NightFurySL2001]

Thanks @anthrotype ! Verified that the same file that caused the error now gives ValueError as expected instead of AttributeError.

[anthrotype]

now gives ValueError as expected

there's the new drop_unsupported option to work around that as well ;)

[NightFurySL2001]

That is definitely a neat option too! 👍 Now I don't have to worry about rogue SVGs from users 😂