Convert columnpicker.gif to SVG

RESOLVED FIXED in Firefox 66

Status

()

RESOLVED FIXED
5 years ago
2 months ago

People

(Reporter: Dolske, Assigned: 3DIndian)

Tracking

(Blocks: 1 bug, {good-first-bug})

unspecified
mozilla66
good-first-bug
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
I couple of XUL widgets contain icons that should have HiDPI version, so that they don't look blurry (upscaled) on Retina displays.

  * <splitter> uses toolkit/themes/osx/global/splitter/dimple.png

  * <tree> uses toolkit/themes/osx/global/tree/columnpicker.gif
Flags: firefox-backlog+

Comment 1

3 months ago
The dimple was removed in bug 1451710.

As for columnpicker.gif, it can be replaced the SVG comm-central uses: https://searchfox.org/comm-central/rev/7f660f1bc41fbb2456448fcb5173f6ca4672b85d/mail/themes/shared/mail/icons/columnpicker.svg
Blocks: 1450569
Summary: Use HiDPI icons in <splitter> and <tree> → Convert columnpicker.gif to SVG

Comment 2

3 months ago
Steps to fix this bug:

1) Create a new file at toolkit/themes/shared/icons/columnpicker.svg containing:

<!-- 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/. -->
<svg xmlns="http://www.w3.org/2000/svg" width="12" height="12" viewBox="0 0 12 12" fill="context-fill">
  <path d="M2.03 2.06l-.03 8h4v-1H3l.03-4H6v1.97h1V5.06h2.97v2h1v-5H2.03zm1 1H6v1H3.03v-1zm3.97 0h2.97v1H7v-1zm5.53 4.97l-5.94.03 2.91 3.35 3.03-3.38z"/>
</svg>

2) Run `hg add toolkit/themes/shared/icons/columnpicker.svg` to make sure the file gets added to the revision

3) In `toolkit/themes/shared/jar.inc.mn`, add the following line:

  skin/classic/global/icons/columnpicker.svg               (../../shared/icons/columnpicker.svg)

4) At https://searchfox.org/mozilla-central/rev/7a922172a94cfe24c7a48e0a581577895e1da8c4/toolkit/themes/shared/tree.inc.css#145 , replace the url with "chrome://global/skin/icons/columnpicker.svg"

5) Run the following commands, this will delete the old GIF files:

hg rm toolkit/themes/osx/global/tree/columnpicker.gif
hg rm toolkit/themes/windows/global/tree/columnpicker.gif

6) Delete the following lines:

https://searchfox.org/mozilla-central/rev/7a922172a94cfe24c7a48e0a581577895e1da8c4/toolkit/themes/osx/global/jar.mn#65
https://searchfox.org/mozilla-central/rev/7a922172a94cfe24c7a48e0a581577895e1da8c4/toolkit/themes/shared/non-mac.jar.inc.mn#28

Please use the needinfo field if you have any questions.
Keywords: good-first-bug
(Assignee)

Comment 3

3 months ago
Posted patch bug1022039.patch (obsolete) — Splinter Review
Hi, I don't have an OSX machine but I followed the given steps. Attached is the patch for the bug.

Comment 4

3 months ago
Thanks for the patch!
Assignee: nobody → 3DIndian
OS: Mac OS X → Unspecified
Hardware: x86 → Unspecified

Comment 5

3 months ago
Comment on attachment 9033625 [details] [diff] [review]
bug1022039.patch

Looks mostly good, I only have a few minor comments:

>diff --git a/toolkit/themes/osx/global/jar.mn b/toolkit/themes/osx/global/jar.mn
>--- a/toolkit/themes/osx/global/jar.mn
>+++ b/toolkit/themes/osx/global/jar.mn
>@@ -61,5 +61,4 @@ toolkit.jar:
>   skin/classic/global/icons/sslWarning.png                           (icons/sslWarning.png)
> * skin/classic/global/in-content/common.css                          (in-content/common.css)
> * skin/classic/global/in-content/info-pages.css                      (in-content/info-pages.css)
>-  skin/classic/global/tree/columnpicker.gif                          (tree/columnpicker.gif)
>-  skin/classic/global/plugins/pluginHelp-16.png                      (plugins/pluginHelp-16.png)
>+  skin/classic/global/plugins/pluginHelp-16.png                      (plugins/pluginHelp-16.png)
>\ No newline at end of file

nit: Please restore one blank line at the end of this file.

>diff --git a/toolkit/themes/shared/icons/columnpicker.svg b/toolkit/themes/shared/icons/columnpicker.svg
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/themes/shared/icons/columnpicker.svg
>@@ -0,0 +1,6 @@
>+<!-- 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/. -->
>+<svg xmlns="http://www.w3.org/2000/svg" width="12" height="12" viewBox="0 0 12 12" fill="context-fill">
>+  <path d="M2.03 2.06l-.03 8h4v-1H3l.03-4H6v1.97h1V5.06h2.97v2h1v-5H2.03zm1 1H6v1H3.03v-1zm3.97 0h2.97v1H7v-1zm5.53 4.97l-5.94.03 2.91 3.35 3.03-3.38z"/>
>+</svg>
>\ No newline at end of file

nit: Please add one blank line at the end of this file.

>diff --git a/toolkit/themes/shared/jar.inc.mn b/toolkit/themes/shared/jar.inc.mn
>--- a/toolkit/themes/shared/jar.inc.mn
>+++ b/toolkit/themes/shared/jar.inc.mn
>@@ -11,6 +11,7 @@ toolkit.jar:
> % skin global classic/1.0 %skin/classic/global/
> % skin help classic/1.0 %skin/classic/help/
> % skin mozapps classic/1.0 %skin/classic/mozapps/
>+  skin/classic/global/icons/columnpicker.svg               (../../shared/icons/columnpicker.svg)
>   skin/classic/global/about.css                            (../../shared/about.css)
>   skin/classic/global/aboutCache.css                       (../../shared/aboutCache.css)
>   skin/classic/global/aboutCacheEntry.css                  (../../shared/aboutCacheEntry.css)

nit: the items in this file are sorted in alphabetical order, could you place columnpicker.svg after skin/classic/global/icons/close.svg ?
Attachment #9033625 - Flags: feedback+
(Assignee)

Comment 6

3 months ago
Posted patch bug1022039_updated.patch (obsolete) — Splinter Review
Here is the updated patch. 

Thanks
Attachment #9033625 - Attachment is obsolete: true

Updated

3 months ago
Attachment #9033645 - Flags: review?(dao+bmo)
Attachment #9033645 - Flags: feedback+

Comment 7

3 months ago
Comment on attachment 9033645 [details] [diff] [review]
bug1022039_updated.patch

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

::: toolkit/themes/osx/global/jar.mn
@@ +64,2 @@
>    skin/classic/global/plugins/pluginHelp-16.png                      (plugins/pluginHelp-16.png)
> +  
\ No newline at end of file

\ No newline at end of file

nit: remove the trailing whitespace (but keep the blank line)
(Assignee)

Comment 8

3 months ago
Updated patch
Attachment #9033645 - Attachment is obsolete: true
Attachment #9033645 - Flags: review?(dao+bmo)

Updated

3 months ago
Attachment #9033659 - Flags: review?(dao+bmo)
Attachment #9033659 - Flags: feedback+
Comment on attachment 9033659 [details] [diff] [review]
bug1022039_update2.patch

>--- a/toolkit/themes/shared/tree.inc.css
>+++ b/toolkit/themes/shared/tree.inc.css
>@@ -142,7 +142,7 @@ treechildren::-moz-tree-column(insertaft
> /* ::::: column picker :::::  */
> 
> .tree-columnpicker-icon {
>-  list-style-image: url("chrome://global/skin/tree/columnpicker.gif");
>+  list-style-image: url("chrome://global/skin/icons/columnpicker.svg");

Looks like -moz-context-properties and fill still need to be set here.
Attachment #9033659 - Flags: review?(dao+bmo) → review-

Comment 10

3 months ago
3DIndian, could you please add `-moz-context-properties: fill;` and `fill: currentColor;` where Dão mentioned ? Let us know if you don't have time though. Thanks again for your work on this bug! :)
Flags: needinfo?(3DIndian)
(Assignee)

Comment 11

3 months ago

I will do it in a day or two and update.
Thanks for the input!

Flags: needinfo?(3DIndian)

Comment 12

2 months ago

I took 3Dindian's patch and added the -moz-context-properties and fill properties.

Patch author remains unchanged :)

Attachment #9037679 - Flags: review?(dao+bmo)
(Assignee)

Comment 13

2 months ago

Thanks a lot, I was busy with my research work (Grad student woes!)

Attachment #9037679 - Flags: review?(dao+bmo) → review+

Comment 14

2 months ago
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed955b30c2f0
Replace columnpicker.gif with columnpicker.svg. r=dao

Comment 15

2 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox66: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.