Closed Bug 1289139 Opened 8 years ago Closed 8 years ago

Use SVG for the button icons in the Downloads Panel

Categories

(Firefox :: Downloads Panel, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 52
Tracking Status
firefox50 --- affected
firefox51 --- affected
firefox52 --- verified

People

(Reporter: Paolo, Assigned: selee)

References

(Depends on 1 open bug)

Details

(Keywords: feature, Whiteboard: [CHE-MVP])

Attachments

(3 files)

      No description provided.
Does it include only buttons inside the panel, and without the download button itself?
To be specific, are we just going to replace the browser/themes/<osname>/downloads/buttons.png file?
Yes, it's only for the action buttons in the panel.
The styling of the buttons will be similar to the one defined in bug 1203292 for the remove button.

I can help with getting some of the properly formatted SVG glyphs for more icons when we are at that stage.
Depends on: 1288747, 1203292
Blocks: 1022550
After the offline discussion with Carol, these PNGs will be replaced with SVGs for every platform.
  browser/themes/linux/downloads/buttons.png
  browser/themes/osx/downloads/buttons.png
  browser/themes/osx/downloads/buttons@2x.png
  browser/themes/windows/downloads/buttons-XP.png
  browser/themes/windows/downloads/buttons.png

Eventually, there are five kinds of SVG icon which is based on the original design [1][2]:
  .downloadButton.downloadIconCancel
  .downloadButton.downloadIconShow
  .downloadButton.downloadIconRetry
  .downloadButton.downloadShowBlockedInfo
  .downloadButton.downloadShowBlockedInfo:-moz-locale-dir(rtl)

These SVG icons will be applied to Windows/OSX/Linux platforms, but there could be some color definitions in CSS for different platform.

[1] https://dxr.mozilla.org/mozilla-central/source/browser/themes/osx/downloads/downloads.css#66
[2] https://dxr.mozilla.org/mozilla-central/source/browser/themes/osx/downloads/allDownloadsViewOverlay.css#15
(In reply to Sean Lee [:seanlee][:weilonge] from comment #4)
>   .downloadButton.downloadIconShow

Note that this icon has two versions, one is for Linux and Windows and the other one is for Mac. In code, we just need to do something similar to what we are doing for the Geolocation permission icon.
Marked CHE-MVP per FE study result + conclusion of project meeting this week.
Whiteboard: [CHE-MVP]
Assignee: nobody → selee
Status: NEW → ASSIGNED
Comment on attachment 8788059 [details]
Bug 1289139 - Replace all PNG download button icons with SVG format icons.;

https://reviewboard.mozilla.org/r/76628/#review74718

::: browser/components/downloads/content/downloads.css
(Diff revision 1)
>  #downloadsPanel-multiView > .panel-viewcontainer > .panel-viewstack[viewtype="subview"] .download-state:not([showingsubview]) .downloadButton {
>    display: none;
>  }
> -#downloadsPanel-multiView > .panel-viewcontainer > .panel-viewstack[viewtype="subview"] .download-state[showingsubview] .downloadButton {
> -  display: inline;
> -}

This rule will display all .downloadButton and not only .downloadButton.downloadIconShow. The rule is fine in the original PNG arrow design.
In SVG icon, the icon looks like a blurry icon with all .downloadButton are in the same stack.
Removing this rule fixes the blurry icon, and I don't see any regression yet.

::: browser/themes/shared/downloads/allDownloadsViewOverlay.inc.css:96
(Diff revision 1)
> +  height: 16px;
> +}
> +
> +/*** Button icons ***/
> +
> +.downloadButton.downloadIconCancel {

The color in these rule is for easier recognition of the state. HighlightText or currentColor should be used here to satisfy different themes.
These rules could be simplified eventually with merging rules into one.

currentColor used here is for changing themes easily, but visual designer wants to show the color #858585 here. I think this should have a high contrast version definition.
Comment on attachment 8788059 [details]
Bug 1289139 - Replace all PNG download button icons with SVG format icons.;

Hi Paolo,

For SVG icon color, visual design wants a different one with description text.
Could you give a suggestion for the color definition example to satisfy theme changing requirement? Thank you.
Attachment #8788059 - Flags: feedback?(paolo.mozmail)
(In reply to Sean Lee [:seanlee][:weilonge] from comment #9)
> Could you give a suggestion for the color definition example to satisfy
> theme changing requirement? Thank you.

For some colors that aren't available in the platform we sometimes implement a special case for the default theme, for example "@media (-moz-windows-default-theme)" on Windows. In many cases we simply use platform colors and update the design accordingly.

These buttons should eventually match the design of the "X" button for removing permissions that we have in the Control Center:

https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/controlcenter/panel.inc.css#399

As you can see, these buttons just use platform colors on a platform background. However, in the Downloads Panel the background color is not always a platform color, so we cannot always use the same colors as the Control Center right now.

In all cases where we use a custom background color, we should continue to use the foreground color found in the corresponding PNG file. We can have a separate bug to update the colors to always be platform colors, and we can do that when we update the hover behavior to match the specification.
Comment on attachment 8788059 [details]
Bug 1289139 - Replace all PNG download button icons with SVG format icons.;

Carol, is it intentional that the left and right arrow to open and close the subview for blocked downloads are replaced with an "i" icon? This is different from what we use elsewhere for buttons that open subviews, and the result may be surprising. We also just implemented the blocked downloads specification that required the use of arrows.

We also need a folder icon on Windows and Linux in place of the magnifying glass, to match the platform expectation for the "open folder" action.

Is the thickness of the stroke in the magnifying glass the correct one? It seems lighter when seen near the other icons, but maybe it's intentional.

Sean, note that we'll probably implement the SVG as glyphs in a single file, and there is no need to spend time optimizing the current versions before we have confirmed the final look of the assets.
Flags: needinfo?(chuang)
Attachment #8788059 - Flags: feedback?(paolo.mozmail)
(In reply to :Paolo Amadini from comment #12)
> Comment on attachment 8788059 [details]
> Bug 1289139 - Replace all PNG download button icons with SVG format icons.
> 
> We also need a folder icon on Windows and Linux in place of the magnifying
> glass, to match the platform expectation for the "open folder" action.
I forgot to remove folder.svg in shared theme. Carol did provide folder.svg and magnifier.svg, and they are applied to all platforms in the current patch:
windows: show.svg (folder.svg)
linux: show.svg (folder.svg)
osx: show.svg (magnifier.svg)

> 
> Sean, note that we'll probably implement the SVG as glyphs in a single file,
> and there is no need to spend time optimizing the current versions before we
> have confirmed the final look of the assets.
got it.
(In reply to Sean Lee [:seanlee][:weilonge] from comment #13)
> I forgot to remove folder.svg in shared theme. Carol did provide folder.svg
> and magnifier.svg, and they are applied to all platforms in the current
> patch:
> windows: show.svg (folder.svg)
> linux: show.svg (folder.svg)
> osx: show.svg (magnifier.svg)

Ah, thanks, indeed I saw four icons in the shared folder and overlooked the platform ones.
(In reply to :Paolo Amadini from comment #12)
> Comment on attachment 8788059 [details]
> Bug 1289139 - Replace all PNG download button icons with SVG format icons.
> 
> Sean, note that we'll probably implement the SVG as glyphs in a single file,
> and there is no need to spend time optimizing the current versions before we
> have confirmed the final look of the assets.

Could this be an example for SVG glyphs? sounds like we should merge these SVG pathes into one SVG file. 
http://searchfox.org/mozilla-central/source/dom/svg/crashtests/723441-resource.svg
Hello Paolo,
Using the "i" for blocked download was intentional. Because I considered that we will have another "arrow" on the bottom(dropdown menu) in the new design. There might be too many "directions" on the whole panel, thats why I wanted to use "info" for the new icon. But I looked at the behavior again, and you are right, I think using "i" icon might cause some inconsistency. Also the malware download situations might not happen often. I will change back using the left/right arrow for open and close the subview. Thanks for the comment Paolo!

I did provide folder icons for Windows and Linux. I think Sean will update the assets as his comment above.

And yes I updated the magnifying glass. That's a new/correct one. Thank you!!
Flags: needinfo?(chuang)
Hi Paolo, bug 950058 is also relative to main menu. could you suggest the landing dependency of bug 950058 and this bug? Thank you.
Flags: needinfo?(paolo.mozmail)
(In reply to Sean Lee [:seanlee][:weilonge] from comment #15)
> Could this be an example for SVG glyphs? sounds like we should merge these
> SVG pathes into one SVG file.

We need to add the paths to this file:

https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/panel-icons.svg

And we need to add the styling to select the glyph:

https://hg.mozilla.org/mozilla-central/file/a1558d048e76/browser/themes/shared/glyphs.svg#l7
Flags: needinfo?(paolo.mozmail)
(In reply to Sean Lee [:seanlee][:weilonge] from comment #17)
> Hi Paolo, bug 950058 is also relative to main menu. could you suggest the
> landing dependency of bug 950058 and this bug? Thank you.

The main interaction between the two bugs is related to the background color and the hover behavior.

If we land this bug first, we have to keep the colors from the PNG files and the round button hover effect for the various states. This might be the easiest incremental way forward, and the styling of the button would be similar to the Control Center until we land the other bug.

To land the other bug first, we would have to use new colors for the PNG states, but this might not be possible until the assets are converted to SVG. So the other option is to land the two bugs together.
Comment on attachment 8788059 [details]
Bug 1289139 - Replace all PNG download button icons with SVG format icons.;

Hello Paolo,

This patch is with the following changes:
1. Add some new pathes into panel-icons.svg
2. Add a round background under icons.

Some tasks should be done then:
Simplify icon CSS rules.

Could you give a feedback for this patch?

Thank you.
Attachment #8788059 - Flags: feedback?(paolo.mozmail)
Comment on attachment 8788059 [details]
Bug 1289139 - Replace all PNG download button icons with SVG format icons.;

It's going in the right direction!

Let me know if you plan to do further updates soon. I'm going to try this with bug 950058 as well, and see what we can do for the colors, but in the meantime if you have already a suggestion on how to proceed feel free to share it!
Attachment #8788059 - Flags: feedback?(paolo.mozmail) → feedback+
(In reply to :Paolo Amadini from comment #22)
> Comment on attachment 8788059 [details]
> Bug 1289139 - Replace all PNG download button icons with SVG format icons.
> 
> It's going in the right direction!
I am glad that you say this. :-)

> 
> Let me know if you plan to do further updates soon. I'm going to try this
> with bug 950058 as well, and see what we can do for the colors, but in the
> meantime if you have already a suggestion on how to proceed feel free to
> share it!
I am going to simplify the icon CSS rules. There are too many redundant rules can be merged.

For the icon color, I suppose this is a temporary solution in this bug so I wish we don't have to spend too much time on the correct color. Since this patch is still not satisfied the specification until we land both this patch and bug 950058's.

For the landing time, how do you think if we land this patch and the one in bug 950058 after Firefox 52 branch out date (9/13, if I remember correctly)?
Hey Paolo,

When I am optimizing css rules, a thought comes to my mind...
How do you think if you try the patch with bug 950058's first?
Since we don't have a final decision for icon colors, I suppose the separated rules would be easier for us to try any solution.
Flags: needinfo?(paolo.mozmail)
(In reply to Sean Lee [:seanlee][:weilonge] from comment #23)
> For the landing time, how do you think if we land this patch and the one in
> bug 950058 after Firefox 52 branch out date (9/13, if I remember correctly)?

Sounds good to me.

(In reply to Sean Lee [:seanlee][:weilonge] from comment #24)
> When I am optimizing css rules, a thought comes to my mind...
> How do you think if you try the patch with bug 950058's first?
> Since we don't have a final decision for icon colors, I suppose the
> separated rules would be easier for us to try any solution.

Yes, we can experiment without the rules being merged yet.
Flags: needinfo?(paolo.mozmail)
Hey Paolo,

Based on comment 22, the patch in this bug has been rebased to bug 950058 for you to try in local. Since I'm not familiar with mozreview, I think using github link would be easier. Here are the patches:

[1st PATCH] Bug 950058 - Split each download item so that all of the right part of it activates the action. r=paolo
https://github.com/weilonge/gecko/commit/962da8417d76fc6bf64040c6123cc9874bce1abe.patch

[2nd PATCH] Bug 1289139 - Replace all PNG download button icons with SVG format icons.
https://github.com/weilonge/gecko/commit/e0df34a18e2edd81971064ac22024aed8d59d692.patch

github branch link here for reference:
https://github.com/weilonge/gecko/commits/rexboy/DownloadsPanel/Bug950058
Thanks for the links to the patches! I've tried them together, and I think initially we just need these foreground colors for the SVG in the buttons:

- "graytext" on the default background, basically only when we are not hovering.
- "white" for the special case of the red background.
- "inherit", that maps to "-moz-fieldtext", in all other cases.

You can use "fill: currentColor;" and use the "color" property like we do in the Control Center, so you can leverage the inherited colors.
The patch [1] has been updated with the suggestion in comment 27. It will be rebased again once the patch in Bug 950058 is updated.

[1] https://github.com/weilonge/gecko/commit/4f227a86c3a944b9ac01b6ca68ddb7eeeb74ba3e
The patch in this bug has been rebased to bug 950058 comment 15.

[1st PATCH] Bug 950058 - Split each download item so that all of the right part of it activates the action. r=paolo
https://github.com/weilonge/gecko/commit/a2ec97408f40df84ef627391f00579fa21eb2dd3.patch

[2nd PATCH] Bug 1289139 - Replace all PNG download button icons with SVG format icons.
https://github.com/weilonge/gecko/commit/81dec91ad97edbe7c6338964f8d1e9ab7cfa7778.patch

github branch link here for reference:
https://github.com/weilonge/gecko/commits/rexboy/DownloadsPanel/Bug950058_v2
(In reply to Sean Lee [:seanlee][:weilonge] from comment #29)
> The patch in this bug has been rebased to bug 950058 comment 15.

I'm not sure if this is reviewable because I saw other comments after this one. When you have the latest combination for review, just ensure both patches are up to date on MozReview and send me the new links, thanks!
Flags: needinfo?(selee)
Hey Paolo,

The latest patch is updated to mozreview, and it includes these changes:
[downloads.css]
  Remove the css rules for button icons in Windows, Linux, and OSX
  Use SVG for button icons with cross-platform design.

[allDownloadsViewOverlay.css]
  Remove the css rules for button icons in Windows, Linux, and OSX
  Use SVG for button icons with cross-platform design.

======================================
github link is just for your reference:
[1st PATCH] Bug 950058 - Split each download item so that all of the right part of it activates the action. r=paolo
https://github.com/weilonge/gecko/commit/a0f4db865a5f71af8c24633287389f3068fb9b17.patch

[2nd PATCH] Bug 1289139 - Replace all PNG download button icons with SVG format icons.
https://github.com/weilonge/gecko/commit/01315e97477c404b90dc09c25e4f88db7921f4a3.patch

github branch link:
https://github.com/weilonge/gecko/commits/rexboy/DownloadsPanel/Bug950058_v3
Flags: needinfo?(selee)
Your patch appears to be using the descendant selector excessively. Can you please avoid that?
Comment on attachment 8788059 [details]
Bug 1289139 - Replace all PNG download button icons with SVG format icons.;

https://reviewboard.mozilla.org/r/76628/#review79436

I've reviewed bug 950058, and the same general observation applies here, the resulting CSS is still much more complex and less readable than it could be.

In particular you'll likely not need to care about which icon shape is displayed to set the color, you can use more generic rules. Try to use "fill: currentColor;" and "color: inherit;" as much as possible, and try to control the foreground text color in the same rules that change the background color, for example for the malware case. Even if in some cases they may be different rules, they should be placed close together.

I'll do another review pass after the comments in the two bugs have been addressed, but before that please check with Rex which rules can be simplified, clustered, or reordered so that the end result is more readable. The rules should generally follow the order of the XUL elements, and reuse the same rules and selectors that are defined in the other bug when possible.
Attachment #8788059 - Flags: review?(paolo.mozmail)
To clarify, the existing rules should be kept in place so the the diff is minimal, and the new rules should be added in logical places.
Blocks: 1307064
Hi Paolo,

The patch in this bug has been rebased to bug 950058 comment 44, and I will ask you review it after I verify it on Linux/Windows and simplify the css rules.

Would you like to see the latest version? I am fine if you want to try the final version, so you can just ignore the current changes.

Here are the patches:

[1st PATCH] Bug 950058 - Split each download item so that all of the right part of it activates the action. r=paolo
https://github.com/weilonge/gecko/commit/b6970709193cb3e4d9cd95d4617b52518d45c1ac.patch

[2nd PATCH] Bug 1289139 - Replace all PNG download button icons with SVG format icons.
https://github.com/weilonge/gecko/commit/c8685b9a804ec90814c0826f4b9f434506878a67.patch

github branch link here for reference:
https://github.com/weilonge/gecko/commits/rexboy/DownloadsPanel/Bug950058_v4
Flags: needinfo?(paolo.mozmail)
(In reply to Sean Lee [:seanlee][:weilonge] from comment #36)
> The patch in this bug has been rebased to bug 950058 comment 44, and I will
> ask you review it after I verify it on Linux/Windows and simplify the css
> rules.

If I can use this version to test bug 950058 I'll do that, thanks. I'll do the reviews tomorrow anyways so if the simplification is ready in the meantime then even better, I can review both patches.
Flags: needinfo?(paolo.mozmail)
Hi Paolo, could you review the latest patch with simplified css rules? Thank you!
Comment on attachment 8788059 [details]
Bug 1289139 - Replace all PNG download button icons with SVG format icons.;

https://reviewboard.mozilla.org/r/76628/#review82212

::: browser/components/downloads/content/downloads.css:219
(Diff revision 4)
>  #downloadsPanel-multiView > .panel-viewcontainer > .panel-viewstack[viewtype="subview"] .download-state:not([showingsubview]) .downloadButton {
>    display: none;
>  }
> -#downloadsPanel-multiView > .panel-viewcontainer > .panel-viewstack[viewtype="subview"] .download-state[showingsubview] .downloadButton {
> -  display: inline;
> -}

Good find! It seems to me that changing the remaining rule to use "visibility: hidden;" gives better results because it keeps the empty space where the button was. Otherwise we have to hide the toolbarseparator too.

::: browser/themes/osx/downloads/downloads.css:53
(Diff revision 4)
>  @notKeyfocus@ @itemFinished@[exists]:hover {
>    cursor: pointer;
>  }

This rule can be removed since we don't need the hyperlink cursor anymore. You can also remove the other "cursor" properties in other files, leaving just the ones for the download summary on Windows 7 because they will be removed in bug 1301384.

::: browser/themes/shared/downloads/allDownloadsViewOverlay.inc.css:117
(Diff revision 4)
> +
> +.downloadButton > .button-box > .button-text {
> +  display: none;
> +}
> +
> +/*** Button icons ***/

Thanks for updating the rules in allDownloadsViewOverlay.inc.css, but they will need to be simplified too.

Because the Library window is not the focus of the visual redesign, you can file a separate bug to figure out the final styling, but what is important in this patch is that the code is short and the behavior is consistent. While testing I noticed that in some hover and focus combinations the button icon disappeared.

::: browser/themes/shared/downloads/downloads.inc.css
(Diff revision 4)
>    height: var(--downloads-item-height);
>    margin: 0;
>    border: none;
>    background: transparent;
>    padding: 8px;
> -  list-style-image: url("chrome://browser/skin/downloads/buttons.png");

The buttons.png file and the other unused PNG images should now be removed from the file system and the manifests.

::: browser/themes/shared/downloads/downloads.inc.css:276
(Diff revision 4)
>  
> +.downloadButton > .button-box > .button-icon {
> +  width: 16px;
> +  height: 16px;
> +  margin: 1px;
> +  fill: graytext;

Please use "fill: currentColor;" after the "filter" property. You can then change the "color" where necessary on the button icon. I don't think you need to set colors on the ".button-icon", they would just inherit the right one, but please verify this as it may vary across platforms.

I think that according to the design you need to set "color: graytext;" explicitly only for the cases where the background is left to its default color, normally white on default themes.

When the background is dimmed, you should just use the default foreground color of the item, in other words you don't need to change anything assuming the text color is already right. This color would be black on default themes.

::: browser/themes/shared/downloads/downloads.inc.css:326
(Diff revision 4)
> +  fill: HighlightText;
> +}
> +
> +/*** Button icons ***/
> +
> +.downloadButton.downloadIconCancel .button-icon {

All of the .button-icon rules should use the child selector, for example ".downloadIconCancel > .button-box > .button-icon". This is because the descendant selector is expensive to compute and we only use this in cases when there is a large number of elements in-between that will likely change and using the child selector would make the code less maintainable or readable.

There are other instances where you should use the child selector as well, please read through the rules you added and update them by keeping the above recommendation in mind.
Attachment #8788059 - Flags: review?(paolo.mozmail)
Attached image library_linux
Hi Paolo,

The issues you mentioned are all fixed in the latest patch. I added a graytext color for :active in the action buttons in Library.

The overall behavior in OSX/Linux/Windows are correct in the current patch for Downloads Panel, Library and about:downloads.

But the color of Library icon in linux is weird (see attachment 8798352 [details]). The round button is gone, and the color is not the same with about:downloads (it's correct).

Could you help to give suggestions? Thank you!
Depends on: 950058
Hi Paolo, After adding "-moz-appearance: none;" to the rule ".downloadButton > .button-box" in allDownloadsViewOverlay.inc.css, the issue mentioned in comment 44 is fixed. Please help to review the patch again. Thank you! :)
Thanks for the patience, it took some time but I've finished the entire review now. I've published my recommended changes for both bugs as a patch:

https://reviewboard.mozilla.org/r/85288/diff/#index_header

Feel free to take and incorporate the parts that are relevant to this bug. For now I've only tested on Mac, please let me know if there is something that I missed for other platforms.

The colors for the button in the Library window are the best I could find. I've made some cases "inherit" so they work when the window is not selected. Eventually, we'll probably make the Library window follow the same styling as the Downloads Panel.

As you've mentioned to me, the color for the buttons in the panel is kept as "graytext" even when the background is dimmed, according to the visual design.
Attachment #8788059 - Flags: review?(paolo.mozmail)
Hi Paolo, thanks a lot for providing the patch to improve the detail. Could you help to review it again?
BTW, after verifying three platforms, I think the behavior is correct in all platforms except the dot-line box after clicking action button in linux.
(In reply to Sean Lee [:seanlee][:weilonge] from comment #51)
> BTW, after verifying three platforms, I think the behavior is correct in all
> platforms except the dot-line box after clicking action button in linux.

I tried using ":-moz-focusring" instead of ":focus" but it doesn't help, the focus indication is always shown even when the button is clicked with the mouse. As far as I can tell, this may be by design for Linux. It doesn't look nice visually, but I'm not sure if there is a solution.
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/49b4e3cb8e7e
Replace all PNG download button icons with SVG format icons. r=paolo
Attachment #8788059 - Flags: review?(paolo.mozmail) → review+
https://hg.mozilla.org/mozilla-central/rev/49b4e3cb8e7e
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
No longer blocks: 1022550
Depends on: 1022550
No longer blocks: 1307064
Keywords: feature
I can confirm the button icons use the SVG format(I used Developer Tools to verify). I verified using the latest nightly(Fx52.0a1, build ID:20161031030202) on Windows 10 x64, Ubuntu 14.04 LTS x64, and Mac OS X 10.9.5.
I also ran a few tests to verify that the changes did not affect the intended behavior on various themes.
The issue is fixed on nightly.

Cheers!
Depends on: 1347315
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: