1175377 - Replace about:reader png images with svg

Status: RESOLVED WONTFIX

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

Description

[:Margaret Leibovic]

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

Comment 1

[:Margaret Leibovic]

antlam, if you can give me svg images, I can make this change.

The icons I need are:

reader-minus
reader-plus
reader-share-icon
reader-style-icon
reader-toggle-off-icon
reader-toggle-on-icon

Here's where you can see the current versions in the tree:
http://mxr.mozilla.org/mozilla-central/find?text=&string=mobile%2Fandroid%2Fthemes%2Fcore%2Fimages

Comment 2

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

Attachment: icons_readerview_SVG.zip

Here you go!

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Just a comment about the SVG files themselves. I see a little "sketch" cruft in there. Should we clean that up manually? The files aren't too big, but I am a little OCD.

Comment 4

[:Margaret Leibovic]

shorlander, do you have any advice about formatting SVG files? Maybe we could make a little "best practices" guide somewhere.

Comment 5

[Stephen Horlander [:shorlander]]

Attachment: replace-reader-images-with-svg.patch

(In reply to :Margaret Leibovic from comment #4)
> shorlander, do you have any advice about formatting SVG files? Maybe we
> could make a little "best practices" guide somewhere.

ntim made this etherpad with some SVG best practices: https://etherpad.mozilla.org/svg-guidelines

Mostly the stuff Sketch, Illustrator and Inkscape spits out is pretty bad, but usable if you remove all the unnecessary bits. 

I am attaching the patch I made in Whistler. I made all of the icons the same size (24 x 24) and pixel snapped them. I don't have a device to test all of the resolutions on but I think it should just work. 

I noticed the icons are strangely named but didn't change them, would suggest:
- reader-plus.svg                 -->    reader-zoomIn.svg
- reader-minux.svg                -->    reader-zoomOut.svg
- reader-share-icon.svg           -->    reader-share.svg
- reader-style-icon.svg           -->    reader-style.svg
- reader-style-icon-active.svg    -->    reader-style-active.svg
- reader-toggle-off-icon.svg      -->    reader-list-add.svg
- reader-toggle-on-icon.svg       -->    reader-list-remove.svg

or I noticed Anthony's had better names too:
- reader-plus.svg                 -->    icon_typebigger_SVG.svg
- reader-minux.svg                -->    icon_typesmaller_SVG.svg
- reader-share-icon.svg           -->    icon_share_SVG.svg
- reader-style-icon.svg           -->    icon_controls_SVG.svg
- reader-toggle-off-icon.svg      -->    icon_addtoRL_SVG.svg
- reader-toggle-on-icon.svg       -->    icon_removefromRL_SVG.svg

Comment 6

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

Awesome, thanks Shorlander!

Comment 7

[:Margaret Leibovic]

Attachment: (v2) Replace Reader View's multiple PNGs with SVG

Thanks so much for the patch! I took it and updated the jar.mn and the CSS, although it looks like some of the icons have changed a bit... I'll attach screenshots to show what I mean.

Comment 8

[:Margaret Leibovic]

Attachment: before (no patch applied)

Comment 9

[:Margaret Leibovic]

Attachment: after (patch applied)

Comment 10

[Stephen Horlander [:shorlander]]

I had to tweak them a bit to make them pixel snapped. I can alter them more if needed. Although the Zoom-Out Icon just looks broken, I don't think it should look like that, maybe the size is set wrong in the CSS?

Comment 11

[Stephen Horlander [:shorlander]]

Will probably need to change the background-size to be 24px x 24px in a few places:

https://dxr.mozilla.org/mozilla-central/source/mobile/android/themes/core/aboutReaderControls.css#80
https://dxr.mozilla.org/mozilla-central/source/mobile/android/themes/core/aboutReaderControls.css#189
https://dxr.mozilla.org/mozilla-central/source/mobile/android/themes/core/aboutReaderControls.css#199

Comment 12

[Tim Nguyen :ntim]

Comment on attachment 8628012 [details] [diff] [review]
(v2) Replace Reader View's multiple PNGs with SVG

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

The comments on the svg apply to all of the svgs.

::: mobile/android/themes/core/images/reader-minus.svg
@@ +1,2 @@
> +<?xml version="1.0" encoding="utf-8"?>
> +<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">

nit : the doctype isn't needed

@@ +1,4 @@
> +<?xml version="1.0" encoding="utf-8"?>
> +<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
> +
> +<svg version="1.1"

The svg version isn't either

@@ +2,5 @@
> +<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
> +
> +<svg version="1.1"
> +     xmlns="http://www.w3.org/2000/svg"
> +     xmlns:xlink="http://www.w3.org/1999/xlink"

There's no xlink:href attribute used in this file, so this namespace is also useless.

@@ +15,5 @@
> +  </style>
> +
> +  <g id="icon-reader-zoomOut" class="style-reader-UI-lighter">
> +    <rect x="0" y="10" width="24" height="4" rx="1" ry="1" />
> +  </g>

Instead of having classnames that apply to only one element, can we directly apply the fill to that one element ?

<rect x="0" y="10" width="24" height="4" rx="1" ry="1" fill="#afb1b3"/>

Like this, we can remove the <g> tag, and the <style> tag. 
In files where there are 2 or more tags as children of the <g> tag, the <g> tag can be kept (without it's class and it's id though), and the fill can be applied on it.

Comment 13

[Stephen Horlander [:shorlander]]

(In reply to Tim Nguyen [:ntim] from comment #12)
> > +  </style>
> > +
> > +  <g id="icon-reader-zoomOut" class="style-reader-UI-lighter">
> > +    <rect x="0" y="10" width="24" height="4" rx="1" ry="1" />
> > +  </g>
> 
> Instead of having classnames that apply to only one element, can we directly
> apply the fill to that one element ?
> 
> <rect x="0" y="10" width="24" height="4" rx="1" ry="1" fill="#afb1b3"/>
> 
> Like this, we can remove the <g> tag, and the <style> tag. 
> In files where there are 2 or more tags as children of the <g> tag, the <g>
> tag can be kept (without it's class and it's id though), and the fill can be
> applied on it.

I mostly left that for my own sake to keep some consistency and maintainability between files.

Would probably a little more efficient if we stuck them all in one file so they could share styles. Like this http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/incontentprefs/icons.svg?raw=1

Comment 14

[apoorvasingh2811]

Hi, can I take this up?

Comment 15

[Firefox Bug Husbandry Bot]

Re-triaging per https://bugzilla.mozilla.org/show_bug.cgi?id=1473195

Needinfo :susheel if you think this bug should be re-triaged.

Comment 16

[Paarmita Bhargava]

Hi,
If this issue is open, I would like to work on it.

Comment 17

[Snikitha Siddavatam]

Hi! Where can i find the source code of this project. I'm unable to find it

Comment 18

[James Willcox (:snorp) ([e-mail]) (he/him)]

Firefox for Android is being sunset soon, and about:reader is not part of GeckoView anymore, so we can close this bug.