Closed Bug 1351986 Opened 7 years ago Closed 7 years ago

Remove DOCTYPE declarations from SVG files

Categories

(Firefox :: Theme, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: dao, Assigned: sauravsachidanand, Mentored)

References

Details

(Keywords: good-first-bug)

Attachments

(1 file)

From bug 759252 comment 101:

> The SVG file probably shouldn't link to a DTD.  I saw this in the output of
> a debug build:
> 
> [Parent 23032] WARNING: Failed to open external DTD: publicId "-//W3C//DTD
> SVG 1.1//EN" systemId "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd"
> base
> "file:///home/dbaron/builds/clean-mozilla-central/mozilla/browser/themes/
> shared/tabbrowser/connecting.svg" URL "resource://gre/res/dtd/svg11.dtd":
> file
> /home/dbaron/builds/clean-mozilla-central/mozilla/parser/htmlparser/
> nsExpatDriver.cpp, line 702
> 
> (It really doesn't need a DOCTYPE declaration at all.)

Affected files:

browser/extensions/flyweb/skin/flyweb-icon.svg
browser/themes/shared/tabbrowser/connecting.svg
browser/themes/shared/tabbrowser/loading.svg
toolkit/themes/shared/reader/RM-Content-Width-Minus-42x16.svg
toolkit/themes/shared/reader/RM-Content-Width-Plus-44x16.svg
toolkit/themes/shared/reader/RM-Line-Height-Minus-38x14.svg
toolkit/themes/shared/reader/RM-Line-Height-Plus-38x24.svg
Blocks: 759252
version="1.1" could be removed too. And there's no need for the xmlns:xlink attribute in the RM*.svg files
Could probably write a test like browser_misused_characters_in_strings.js that scans SVG files for these types of things.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #2)
> Could probably write a test like browser_misused_characters_in_strings.js
> that scans SVG files for these types of things.

I tagged this as a good first bug, so not going to worry about that here. Also, cruft in SVGs doesn't do much harm. (I assume the parser doesn't actually try to connect to w3.org. If it does, it should probably stop doing that.) Letting image swaps bounce for this kind of thing doesn't seem like a net win to me.
I did file a bug about a command that cleans SVGs: bug 1220483

There a few drawbacks to the tool used:
- It automatically changes IDs to a, b, c, d, ... (but I think we can configure the tool to avoid it)
- There's no way to specify a license header (but I can guess that's possible without SVGO)
- The default configuration does not remove the SVG <title>, (but that can be configured too)
Note that we've started using svgo in some devtools files (notably the firebug theme images).
Blocks: 1352085
(In reply to Dão Gottwald [::dao] from comment #3)
> (I assume the parser doesn't actually try to connect to w3.org. If
> it does, it should probably stop doing that.)

We never connect to a remote server, but we do have local files that we parse for XML entities for certain doctypes:

https://dxr.mozilla.org/mozilla-central/rev/8df9fabf2587b7020889755acb9e75b664fe13cf/parser/htmlparser/nsExpatDriver.cpp#258

However, we seem to have removed the "-//W3C//DTD SVG 1.1//EN" entity doc at some point, so I _think_ the overhead from the doctype declarations in our SVG files should be minimal.
I'd like to work on this.
(In reply to Saurav Sachidanand from comment #7)
> I'd like to work on this.

Please do. Have you built Firefox already? Do you know how to create and submit a patch?
(In reply to Dão Gottwald [::dao] from comment #8)
> Have you built Firefox already?

Yes

> Do you know how to create and submit a patch?

I'm currently familiar with git. I'll read up on hg, mozreview and submit a patch. I've made the actual change though.
Comment on attachment 8853731 [details]
Bug 1351986 - Remove DOCTYPE, version, xmlns:xlink from SVG files;

https://reviewboard.mozilla.org/r/125804/#review128328

::: browser/extensions/flyweb/skin/flyweb-icon.svg:1
(Diff revision 1)
>  <?xml version="1.0" encoding="utf-8"?>
>  <!-- Generator: Adobe Illustrator 16.0.0, SVG Export Plug-In . SVG Version: 6.00 Build 0)  -->
> -<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
>  <svg version="1.1" id="Layer_1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" x="0px" y="0px"
>    width="64px" height="64px" viewBox="0 0 64 64" enable-background="new 0 0 64 64" xml:space="preserve">

There are a lot of things that can be removed here (xmlns:xlink, enable-background, xml:space, id, the adobe illustrator comment, x & y atributes, version), SVGOMG can do that for you: https://jakearchibald.github.io/svgomg/
Assignee: nobody → sauravsachidanand
Status: NEW → ASSIGNED
Comment on attachment 8853731 [details]
Bug 1351986 - Remove DOCTYPE, version, xmlns:xlink from SVG files;

https://reviewboard.mozilla.org/r/125804/#review128328

> There are a lot of things that can be removed here (xmlns:xlink, enable-background, xml:space, id, the adobe illustrator comment, x & y atributes, version), SVGOMG can do that for you: https://jakearchibald.github.io/svgomg/

We don't need to handle this here.
Comment on attachment 8853731 [details]
Bug 1351986 - Remove DOCTYPE, version, xmlns:xlink from SVG files;

https://reviewboard.mozilla.org/r/125804/#review128828
Attachment #8853731 - Flags: review?(dao+bmo) → review+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7cafcea59af9
Remove DOCTYPE, version, xmlns:xlink from SVG files; r=dao
https://hg.mozilla.org/mozilla-central/rev/7cafcea59af9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.