Remove DOCTYPE declarations from SVG files

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Theme
RESOLVED FIXED
a month ago
25 days ago

People

(Reporter: dao, Assigned: Saurav Sachidanand, Mentored)

Tracking

({good-first-bug})

Trunk
Firefox 55
good-first-bug
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

a month ago
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
(Reporter)

Updated

a month ago
Blocks: 759252

Comment 1

29 days ago
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.
(Reporter)

Comment 3

29 days ago
(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.

Comment 4

29 days ago
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)

Comment 5

29 days ago
Note that we've started using svgo in some devtools files (notably the firebug theme images).

Updated

29 days ago
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.
(Assignee)

Comment 7

27 days ago
I'd like to work on this.
(Reporter)

Comment 8

27 days ago
(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?
(Assignee)

Comment 9

27 days ago
(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 hidden (mozreview-request)

Comment 11

26 days ago
mozreview-review
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
(Reporter)

Comment 12

25 days ago
mozreview-review-reply
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.
(Reporter)

Comment 13

25 days ago
mozreview-review
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+

Comment 14

25 days ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7cafcea59af9
Remove DOCTYPE, version, xmlns:xlink from SVG files; r=dao

Comment 15

25 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7cafcea59af9
Status: ASSIGNED → RESOLVED
Last Resolved: 25 days ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.