1181541 - Branding identity icons for internal pages should be SVG

Status: RESOLVED FIXED

(Firefox :: General, defect, P1)

Description

[Tim Taubert [:ttaubert] (inactive)]

With bug 1175678 fixed all the URL bar icons will be SVG. We should do the same for the branding icons, there currently are three different ones: Aurora, Nightly/Unofficial, and Official.

Steven, how difficult would it be to get those in 16px and 24px versions?

Comment 1

[bbell]

Attachment: firefox.svg

Here's an SVG. Not knowing what you need it for I have a few concerns. SVGs are not always an appropriate format, and you might actually need a highly tailored SVG file to get good results. 

Can someone send me the mock up of what's being proposed. I might be able to create a better option for you.

Comment 2

[Matthew N. [:MattN]]

This is for http://grab.by/IR7W (e.g. from about:home) which we want to scale for various dppx (e.g. on Windows with 1.25x, 1.5x, etc.)

We're looking for replacements for the files listed here:
https://mxr.mozilla.org/mozilla-central/find?string=identity-icons-brand&tree=mozilla-central&hint=

Comment 3

[bbell]

Attachment: identity-icons-firefox.zip

Here are the SVGs you asked for.

Comment 4

[Matthew N. [:MattN]]

Thanks!

Comment 5

[Matthew N. [:MattN]]

I couldn't fine anywhere the 2nd or 3rd state in the sprite were used in the tree but Classic Theme Restorer is using it and this bug isn't making the problem worse.

Marking addon-compat and CC'ing bsmedberg and Aris since their extensions reference the PNG files.

Comment 6

[Matthew N. [:MattN]]

Attachment: MozReview Request: Bug 1181541 - Convert branding identity icons for internal pages to SVG. r=ttaubert

Bug 1181541 - Convert branding identity icons for internal pages to SVG. r=ttaubert

Comment 7

[bbell]

(In reply to Matthew N. [:MattN] from comment #5)
> I couldn't fine anywhere the 2nd or 3rd state in the sprite were used in the
> tree but Classic Theme Restorer is using it and this bug isn't making the
> problem worse.
> 
> Marking addon-compat and CC'ing bsmedberg and Aris since their extensions
> reference the PNG files.

Would you prefer the icons on their own?

Comment 8

[bbell]

Attachment: Identity-singles.zip

All four icons minus the hover and active states.

Comment 9

[Matthew N. [:MattN]]

Tested on Win8.1 and OS X 10.10 btw.

Comment 10

[Matthew N. [:MattN]]

(In reply to bbell from comment #7)
> (In reply to Matthew N. [:MattN] from comment #5)
> > I couldn't fine anywhere the 2nd or 3rd state in the sprite were used in the
> > tree but Classic Theme Restorer is using it and this bug isn't making the
> > problem worse.
> 
> Would you prefer the icons on their own?

We'll see what Tim says. This bug won't make anything worse either way.

(In reply to bbell from comment #8)
> All four icons minus the hover and active states.

Thanks

Comment 11

[Aris]

Thanks for letting me know.

I will update CTRs brand icons references once SVG icons hit Nightly channel. Just did some local tests with provided SVG files and everything works fine.

Is this change planned for Fx42+ in general (F42: Nightly->Aurora->Beta->Release) or for Nightly only?

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8636957 [details]
MozReview Request: Bug 1181541 - Convert branding identity icons for internal pages to SVG. r=ttaubert

https://reviewboard.mozilla.org/r/13771/#review12373

The SVG files itself are quite heavy already, I think we shouldn't include the hover/active states if we don't use them anymore. All other location bar SVGs have a single state.

Should we push to try to see whether that makes a difference for Talos?

All SVG files should have the following header:

<?xml version="1.0" encoding="utf-8"?>
<!-- This Source Code Form is subject to the terms of the Mozilla Public
   - License, v. 2.0. If a copy of the MPL was not distributed with this
   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->

We should add the following to the top of every SVG file:

<?xml version="1.0" encoding="utf-8"?>
<!-- This Source Code Form is subject to the terms of the Mozilla Public
   - License, v. 2.0. If a copy of the MPL was not distributed with this
   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->

Comment 13

[Matthew N. [:MattN]]

Comment on attachment 8636957 [details]
MozReview Request: Bug 1181541 - Convert branding identity icons for internal pages to SVG. r=ttaubert

Bug 1181541 - Convert branding identity icons for internal pages to SVG. r=ttaubert

Comment 14

[Matthew N. [:MattN]]

Try push in progress (waiting for try autoland) for talos.

Aris, note that we're removing 2nd and third state now.

Comment 15

[Matthew N. [:MattN]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=103193c1cd13

Comment 16

[Matthew N. [:MattN]]

(In reply to Aris from comment #11)
> Thanks for letting me know.

No problem. It's easy when there are only a few affected AMO add-ons and I can figure out the author's bugmail address.

> Is this change planned for Fx42+ in general (F42:
> Nightly->Aurora->Beta->Release) or for Nightly only?

It will land in 42 and ride the trains.

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8636957 [details]
MozReview Request: Bug 1181541 - Convert branding identity icons for internal pages to SVG. r=ttaubert

https://reviewboard.mozilla.org/r/13771/#review12423

r=me with the tiny CSS fix and the <defs> and <g> elements removed from all four SVGs. Thanks!

::: browser/themes/shared/identity-block/identity-block.inc.css:98
(Diff revision 2)
> -  -moz-image-region: rect(0, 16px, 16px, 0);
> +  -moz-image-region: rect(0, 32px, 32px, 0);

We don't need this any longer.

::: browser/branding/aurora/content/identity-icons-brand.svg:14
(Diff revision 2)
> +  <g id="artboard-1">

<g> is a grouping element, we don't need it.

::: browser/branding/aurora/content/identity-icons-brand.svg:6
(Diff revision 2)
> +  <defs>

No need to wrap <style> in <defs>. Just remove the <defs> element.

Comment 18

[Matthew N. [:MattN]]

Sorry, I didn't attempt to clean up the SVG. I've addressed the comments locally. Just waiting on the Talos results now:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=8e5c888d0d89&newProject=try&newRevision=103193c1cd13

So far there's nothing really bad on Linux.

Comment 19

[Matthew N. [:MattN]]

Due to bugs in Talos and PerfHerder, I'm still waiting on accurate results:

https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]&fromchange=996530200d41&tochange=6f97042313dc
http://compare-talos.mattn.ca/?oldRevs=996530200d41&newRev=2673a2942c84&submit=true
http://compare-talos.mattn.ca/?oldRevs=c9b3e5fa453b&newRev=6f97042313dc&submit=true

Comment 20

[Matthew N. [:MattN]]

Windows 7 results have been running for 5 hours and I don't want to wait anymore after all of the talos/perfherder issues plus Win7 was fine on the push from comment 18 so I'm pushing this. No relevant red/green shows on compare-talos.

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/d6e98b1b3a63

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d6e98b1b3a63