1175375 - Replace about:feedback png images with svg

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[:Margaret Leibovic]

Looking in /mobile/android/themes/core/images, we have multiple images in multiple sizes for about:feedback. We should look into replacing these with svg.

Comment 1

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: SVG_floatyheart.zip

Here's the heart and floaty in SVG format.

Comment 2

[:Margaret Leibovic]

This will involve replacing the images here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutFeedback.js#13

If we swap out all of these different png images for single svg images, we can stop doing this image setting logic in JS, and instead do it directly in HTML.

That means we could get rid of this block of logic:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutFeedback.js#47

Comment 3

[Giovanny Gongora [:gioyik]]

Attachment: 1175375.patch

Comment 4

[Giovanny Gongora [:gioyik]]

Hi Margaret,

I attached a patch for this bug. Let me know when you have time to review it, if is OK or needs corrections.

Thanks

Comment 5

[:Margaret Leibovic]

I'm traveling this week. I'll try to review this soon, but maybe ally or liuche can get to it first.

Comment 6

[Giovanny Gongora [:gioyik]]

Not a problem, enjoy your travel. :)

Comment 7

[Allison Naaktgeboren :ally]

Comment on attachment 8637639 [details] [diff] [review]
1175375.patch

Review of attachment 8637639 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there! The wonderful thing about svg is the space they save and the cleaner code we can have.

The difficult thing is that they are code themselves and have to be cleaned up and formatted correctly from what the auto-gen tools spit out. 

I commented on the ones that really popped at me, but here is the full list if you'd like to have a look over: 
https://etherpad.mozilla.org/svg-guidelines

::: mobile/android/chrome/content/aboutFeedback.js
@@ +34,5 @@
>    document.getElementById("sad-link").addEventListener("click", function(evt) {
>      switchSection("sad");
>    }, false);
>  
> +  let helpSectionIcon = FLOATY_ICON;

So much nicer. :)

::: mobile/android/themes/core/images/icon_floaty.svg
@@ +1,1 @@
> +<?xml version="1.0" encoding="UTF-8" standalone="no"?>

license needed

@@ +1,3 @@
> +<?xml version="1.0" encoding="UTF-8" standalone="no"?>
> +<svg width="121px" height="121px" viewBox="0 0 121 121" version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" xmlns:sketch="http://www.bohemiancoding.com/sketch/ns">
> +    <!-- Generator: Sketch 3.3.2 (12043) - http://www.bohemiancoding.com/sketch -->

Remove this generator comment

@@ +1,5 @@
> +<?xml version="1.0" encoding="UTF-8" standalone="no"?>
> +<svg width="121px" height="121px" viewBox="0 0 121 121" version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" xmlns:sketch="http://www.bohemiancoding.com/sketch/ns">
> +    <!-- Generator: Sketch 3.3.2 (12043) - http://www.bohemiancoding.com/sketch -->
> +    <title>XXHDPI</title>
> +    <desc>Created with Sketch.</desc>

<desc> not needed, title needs to be something descriptive, floaty icon maybe?

@@ +2,5 @@
> +<svg width="121px" height="121px" viewBox="0 0 121 121" version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" xmlns:sketch="http://www.bohemiancoding.com/sketch/ns">
> +    <!-- Generator: Sketch 3.3.2 (12043) - http://www.bohemiancoding.com/sketch -->
> +    <title>XXHDPI</title>
> +    <desc>Created with Sketch.</desc>
> +    <defs></defs>

remove the defs tag since there is nothing in it

@@ +4,5 @@
> +    <title>XXHDPI</title>
> +    <desc>Created with Sketch.</desc>
> +    <defs></defs>
> +    <g id="Page-1" stroke="none" stroke-width="1" fill="none" fill-rule="evenodd" sketch:type="MSPage">
> +        <g id="XXHDPI" sketch:type="MSArtboardGroup" transform="translate(-132.000000, -538.000000)" fill="#D23C3C">

2 space indent (applies to whole file)

::: mobile/android/themes/core/images/icon_heart.svg
@@ +1,1 @@
> +<?xml version="1.0" encoding="UTF-8" standalone="no"?>

license needed

@@ +1,2 @@
> +<?xml version="1.0" encoding="UTF-8" standalone="no"?>
> +<svg width="180px" height="153px" viewBox="0 0 180 153" version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" xmlns:sketch="http://www.bohemiancoding.com/sketch/ns">

Does it need to this wide/high?

@@ +2,5 @@
> +<svg width="180px" height="153px" viewBox="0 0 180 153" version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" xmlns:sketch="http://www.bohemiancoding.com/sketch/ns">
> +    <!-- Generator: Sketch 3.3.2 (12043) - http://www.bohemiancoding.com/sketch -->
> +    <title>XXHDPI</title>
> +    <desc>Created with Sketch.</desc>
> +    <defs></defs>

remove defs and desc. make title something like heart icon

@@ +3,5 @@
> +    <!-- Generator: Sketch 3.3.2 (12043) - http://www.bohemiancoding.com/sketch -->
> +    <title>XXHDPI</title>
> +    <desc>Created with Sketch.</desc>
> +    <defs></defs>
> +    <g id="Page-1" stroke="none" stroke-width="1" fill="none" fill-rule="evenodd" sketch:type="MSPage">

id actually used? if not used, remove

@@ +4,5 @@
> +    <title>XXHDPI</title>
> +    <desc>Created with Sketch.</desc>
> +    <defs></defs>
> +    <g id="Page-1" stroke="none" stroke-width="1" fill="none" fill-rule="evenodd" sketch:type="MSPage">
> +        <g id="XXHDPI" sketch:type="MSArtboardGroup" transform="translate(-103.000000, -225.000000)" fill="#EB6482">

Do we actually need this id? Do we use it anywhere?

@@ +7,5 @@
> +    <g id="Page-1" stroke="none" stroke-width="1" fill="none" fill-rule="evenodd" sketch:type="MSPage">
> +        <g id="XXHDPI" sketch:type="MSArtboardGroup" transform="translate(-103.000000, -225.000000)" fill="#EB6482">
> +            <path d="M274.529412,305.125 C256.541169,334.175806 193,378 193,378 C193,378 129.764482,334.175806 111.470588,305.125 C92.3140347,276.46267 106.115218,225 154.882353,225 C178.515208,225 193,252.625 193,252.625 C193,252.625 207.790443,225 232.176471,225 C279.974267,225 293.559287,276.46267 274.529412,305.125 Z M193,264.3125 C193.000074,264.373762 193.000111,264.343135 193,264.3125 L193,264.3125 Z" sketch:type="MSShapeGroup"></path>
> +        </g>
> +    </g>

2 space indent (whole file)

Comment 8

[Giovanny Gongora [:gioyik]]

> I commented on the ones that really popped at me, but here is the full list if you'd like to have a look over: 
> https://etherpad.mozilla.org/svg-guidelines

There's a tool or script for this? I think will be more useful something that lint svg images and less painful instead of doing it manually. Would you be interested on this? I can create a script (Python or JavaScript) that does this. If yes, let me know and I will work on it too and happy to know it will be useful (or someone will find useful) for a Mozilla team.

Thanks for the feedback, I will create another patch with the changes.

Comment 9

[:Margaret Leibovic]

(In reply to Giovanny Gongora [:gioyik] from comment #8)
> > I commented on the ones that really popped at me, but here is the full list if you'd like to have a look over: 
> > https://etherpad.mozilla.org/svg-guidelines
> 
> There's a tool or script for this? I think will be more useful something
> that lint svg images and less painful instead of doing it manually. Would
> you be interested on this? I can create a script (Python or JavaScript) that
> does this. If yes, let me know and I will work on it too and happy to know
> it will be useful (or someone will find useful) for a Mozilla team.
> 
> Thanks for the feedback, I will create another patch with the changes.

shorlander might know if a tool like this already exists. And if not, he might be happy to have a tool like this :)

Comment 10

[Stephen Horlander [:shorlander]]

Hi! There are a few tools I have seen that are designed to clean SVG. I don't know of any that do exactly what's in that etherpad though. If there was a way to automate that I would use it :)

Comment 11

[Tim Nguyen :ntim]

Comment on attachment 8637639 [details] [diff] [review]
1175375.patch

Review of attachment 8637639 [details] [diff] [review]:
-----------------------------------------------------------------

About the automated script, I started writing one a while ago, but it's very buggy, and doesn't handle all cases mentioned in the etherpad. 
Anyway, I haven't got time to finish it, but if you'd like to do it, here it is : https://gist.github.com/nt1m/21217e3e2ea5b0c0586b

I've also added some other tools/frameworks that can help as well at the end of the etherpad (https://etherpad.mozilla.org/svg-guidelines ).

Also, here are some comments about the SVGs, they apply to both files.

::: mobile/android/themes/core/images/icon_floaty.svg
@@ +1,2 @@
> +<?xml version="1.0" encoding="UTF-8" standalone="no"?>
> +<svg width="121px" height="121px" viewBox="0 0 121 121" version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" xmlns:sketch="http://www.bohemiancoding.com/sketch/ns">

Attributes that can be removed :
- xmlns:xlink (no xlink:href attributes in the file)
- xmlns:sketch (leftover from the editor)
- version (useless)

@@ +1,4 @@
> +<?xml version="1.0" encoding="UTF-8" standalone="no"?>
> +<svg width="121px" height="121px" viewBox="0 0 121 121" version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" xmlns:sketch="http://www.bohemiancoding.com/sketch/ns">
> +    <!-- Generator: Sketch 3.3.2 (12043) - http://www.bohemiancoding.com/sketch -->
> +    <title>XXHDPI</title>

This title can be removed. It's invisible to the user unless he opens the SVG in a tab. Either way, the file name is descriptive enough.

@@ +5,5 @@
> +    <desc>Created with Sketch.</desc>
> +    <defs></defs>
> +    <g id="Page-1" stroke="none" stroke-width="1" fill="none" fill-rule="evenodd" sketch:type="MSPage">
> +        <g id="XXHDPI" sketch:type="MSArtboardGroup" transform="translate(-132.000000, -538.000000)" fill="#D23C3C">
> +            <path d="M189.25,546.508573 C190.450926,546.426452 191.662948,546.384718 192.884718,546.384718 C221.879667,546.384718 245.384718,569.889768 245.384718,598.884718 C245.384718,600.145593 245.340269,601.396087 245.252852,602.634718 L222.652616,602.634718 C222.805805,601.406239 222.884718,600.154694 222.884718,598.884718 C222.884718,582.316175 209.45326,568.884718 192.884718,568.884718 C191.654444,568.884718 190.441466,568.958773 189.25,569.10267 L189.25,569.10267 L189.25,546.508573 L189.25,546.508573 Z M140.508573,595.25 C140.426452,596.450926 140.384718,597.662948 140.384718,598.884718 C140.384718,627.879667 163.889768,651.384718 192.884718,651.384718 C194.145593,651.384718 195.396087,651.340269 196.634718,651.252852 L196.634718,628.652616 L196.634718,628.652616 C195.406239,628.805805 194.154694,628.884718 192.884718,628.884718 C176.316175,628.884718 162.884718,615.45326 162.884718,598.884718 C162.884718,597.654444 162.958773,596.441466 163.10267,595.25 L140.508573,595.25 L140.508573,595.25 Z M192.884718,658.884718 C226.021803,658.884718 252.884718,632.021803 252.884718,598.884718 C252.884718,565.747633 226.021803,538.884718 192.884718,538.884718 C159.747633,538.884718 132.884718,565.747633 132.884718,598.884718 C132.884718,632.021803 159.747633,658.884718 192.884718,658.884718 Z M192.884718,621.384718 C205.311124,621.384718 215.384718,611.311124 215.384718,598.884718 C215.384718,586.458311 205.311124,576.384718 192.884718,576.384718 C180.458311,576.384718 170.384718,586.458311 170.384718,598.884718 C170.384718,611.311124 180.458311,621.384718 192.884718,621.384718 Z" sketch:type="MSShapeGroup"></path>

Instead of :
<g attributes-making-the-shape-invisible>
  <g fill="color" (makes the shape visible again) transform="translate(...)">
    <path d="..." leftover-sketch-attribute>
  </g>
</g>
You can simply do :
<path d="..." fill="color" transform="translate(...)">

Also, the transform values can be rounded.

Comment 12

[Richard Newman [:rnewman]]

We're not shipping about:feedback in the product now. It's web-hosted.