[Photon] Update sidebar tree icons on all platforms

RESOLVED FIXED in Firefox 56

Status

()

Firefox
Theme
P1
normal
RESOLVED FIXED
2 months ago
2 days ago

People

(Reporter: nhnt11, Assigned: nhnt11)

Tracking

(Depends on: 2 bugs, Blocks: 4 bugs)

Trunk
Firefox 56
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [photon-visual][p1], URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

2 months ago
Assignee: nobody → nhnt11
Status: NEW → ASSIGNED

Updated

2 months ago
Iteration: 56.1 - Jun 26 → 56.2 - Jul 10
Flags: qe-verify?
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 4

2 months ago
mozreview-review
Comment on attachment 8882514 [details]
Bug 1377011 - [Photon] Update sidebar tree icons on all platforms.

https://reviewboard.mozilla.org/r/153632/#review158744

::: browser/themes/osx/places/places.css:213
(Diff revision 1)
>     after the (title, query) selector, or it would get overridden. */
> -treechildren::-moz-tree-image(title, query, folder),
> -treechildren::-moz-tree-image(title, query, folder, open) {
> -  list-style-image: url("chrome://global/skin/tree/folder.png");
> +treechildren::-moz-tree-image(title, query, folder) {
> +  list-style-image: url("chrome://browser/skin/sidebar-folder.svg");
> +}
> -  -moz-image-region: rect(0, 16px, 16px, 0);
>  }

stray }

::: browser/themes/shared/icons/sidebar-arrow.svg:1
(Diff revision 1)
> +<svg xmlns="http://www.w3.org/2000/svg" width="8" height="8" viewBox="0 0 8 8">

License header please

::: browser/themes/shared/icons/sidebar-unfiledBookmarks.svg:1
(Diff revision 1)
> +<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16">

License header

::: browser/themes/shared/jar.inc.mn:117
(Diff revision 1)
>  #ifndef MOZ_PHOTON_THEME
>    skin/classic/browser/bookmarksMenu.svg              (../shared/icons/bookmarksMenu.svg)
>  #else
>    skin/classic/browser/bookmark-star-on-tray.svg      (../shared/icons/bookmark-star-on-tray.svg)
>  #endif
> +  skin/classic/browser/sidebar-arrow.svg              (../shared/icons/sidebar-arrow.svg)

sidebar-arrow.svg doesn't seem to be used in the Linux and Windows patches (which I think is probably correct), so it shouldn't be in shared/.
Attachment #8882514 - Flags: review?(dao+bmo) → review-

Comment 5

2 months ago
mozreview-review
Comment on attachment 8882515 [details]
Bug 1377011 - [Photon] Update sidebar tree icons on linux.

https://reviewboard.mozilla.org/r/153634/#review158746

::: browser/themes/linux/places/places.css:49
(Diff revision 1)
>    width: 0;
>    height: 0;
>  }
>  
>  treechildren::-moz-tree-image(title, container) {
> -  list-style-image: url("moz-icon://stock/gtk-directory?size=menu");
> +  list-style-image: url("chrome://browser/skin/sidebar-folder.svg");

The icon uses context-fill but you don't set a fill color, which means we'd use black regardless of the OS theme, which might be okay on Mac but not on Linux or Windows.

::: browser/themes/linux/places/places.css:76
(Diff revision 1)
> -  -moz-image-region: auto;
>  }
>  
>  /* query-nodes should be styled even if they're not expandable */
>  treechildren::-moz-tree-image(title, query) {
>    list-style-image: url("chrome://browser/skin/places/query.png");

This is present in new profiles and looks out of place now. Not sure about the other remaining pngs.
Attachment #8882515 - Flags: review?(dao+bmo) → review-

Comment 6

2 months ago
mozreview-review
Comment on attachment 8882516 [details]
Bug 1377011 - [Photon] Update sidebar tree icons on Windows.

https://reviewboard.mozilla.org/r/153636/#review158748

This likely suffers from the same issues as the Linux patch. Canceling review for now.
Attachment #8882516 - Flags: review?(dao+bmo)

Comment 7

a month ago
According to the spec, the same icons should be in the bookmarks toolbar and bookmarks sidebar.


See https://people-mozilla.org/~shorlander/projects/photon/Mockups/windows-10.html

(Expand the Bookmarks Toolbar folder in the Sidebar, and see that the icons are identical to the ones in the bookmarks toolbar)
(In reply to Tim Nguyen :ntim from comment #7)
> According to the spec, the same icons should be in the bookmarks toolbar and
> bookmarks sidebar.
> 
> 
> See
> https://people-mozilla.org/~shorlander/projects/photon/Mockups/windows-10.
> html
> 
> (Expand the Bookmarks Toolbar folder in the Sidebar, and see that the icons
> are identical to the ones in the bookmarks toolbar)

I imagine they should also be used in the library window as they are currently the same (and look blurry on hidpi screens in the three places)

Updated

a month ago
Iteration: 56.2 - Jul 10 → 56.3 - Jul 24
Comment hidden (mozreview-request)
(Assignee)

Updated

a month ago
Attachment #8882515 - Attachment is obsolete: true
(Assignee)

Updated

a month ago
Attachment #8882516 - Attachment is obsolete: true
Comment hidden (mozreview-request)
(Assignee)

Updated

a month ago
Depends on: 1381453

Comment 11

a month ago
mozreview-review
Comment on attachment 8882514 [details]
Bug 1377011 - [Photon] Update sidebar tree icons on all platforms.

https://reviewboard.mozilla.org/r/153632/#review163422

1:38.34 make[6]: *** /home/dao/mozilla/central/objdir-frontend/browser/themes/shared/icons/: No such file or directory.  Stop.
1:38.34 /home/dao/mozilla/central/config/faster/rules.mk:75: recipe for target '/home/dao/mozilla/central/objdir-frontend/browser/themes/shared/icons/sidebar-folder-live.svg' failed
1:38.34 make[5]: *** [/home/dao/mozilla/central/objdir-frontend/browser/themes/shared/icons/sidebar-folder-live.svg] Error 2
Attachment #8882514 - Flags: review?(dao+bmo) → review-
Comment hidden (mozreview-request)
(Assignee)

Comment 13

a month ago
(In reply to Dão Gottwald [::dao] from comment #11)
> Comment on attachment 8882514 [details]
> Bug 1377011 - [Photon] Update sidebar tree icons on all platforms.
> 
> https://reviewboard.mozilla.org/r/153632/#review163422
> 
> 1:38.34 make[6]: ***
> /home/dao/mozilla/central/objdir-frontend/browser/themes/shared/icons/: No
> such file or directory.  Stop.
> 1:38.34 /home/dao/mozilla/central/config/faster/rules.mk:75: recipe for
> target
> '/home/dao/mozilla/central/objdir-frontend/browser/themes/shared/icons/
> sidebar-folder-live.svg' failed
> 1:38.34 make[5]: ***
> [/home/dao/mozilla/central/objdir-frontend/browser/themes/shared/icons/
> sidebar-folder-live.svg] Error 2

Boooooooooo, thought I did hg addremove

Comment 14

a month ago
mozreview-review
Comment on attachment 8882514 [details]
Bug 1377011 - [Photon] Update sidebar tree icons on all platforms.

https://reviewboard.mozilla.org/r/153632/#review163484

::: browser/themes/shared/icons/sidebar-bookmarksMenu.svg:8
(Diff revision 4)
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg width="16" height="16" viewBox="0 0 16 16" xmlns="http://www.w3.org/2000/svg">
> +  <path fill="context-fill" fill-opacity=".05" d="M15,2H1v12c0,0.6,0.5,1,1,1h12c0.6,0,1-0.4,1-1V2L15,2z"/>
> +  <path fill="context-fill" d="M3,5v1h2V5H3z M3,9h2V8H3V9z M3,12h2v-1H3V12z"/>
> +  <path fill="context-fill" fill-opacity=".9" d="M6,5v1h7V5H6z M12,8H6v1h6V8z M6,12h7v-1H6V12z"/>
> +  <path fill="context-fill" d="M7,2V1H1v1v1v11c0,0.5,0.5,1,1,1h12c0.5,0,1-0.5,1-1V2H7z M13.5,14h-11C2.2,14,2,13.8,2,13.5V3h12v10.5 C14,13.8,13.8,14,13.5,14z"/>

just put fill="context-fill" on the parent svg node

::: browser/themes/shared/jar.inc.mn:137
(Diff revision 4)
> +  skin/classic/browser/sidebar-bookmarksToolbar.svg   (../shared/icons/sidebar-bookmarksToolbar.svg)
> +  skin/classic/browser/sidebar-folder.svg             (../shared/icons/sidebar-folder.svg)
> +  skin/classic/browser/sidebar-folder-live.svg        (../shared/icons/sidebar-folder-live.svg)
> +  skin/classic/browser/sidebar-folder-smart.svg       (../shared/icons/sidebar-folder-smart.svg)
> +  skin/classic/browser/sidebar-history.svg            (../shared/icons/sidebar-history.svg)
> +  skin/classic/browser/sidebar-unfiledBookmarks.svg   (../shared/icons/sidebar-unfiledBookmarks.svg)

Please put these icons in shared/places/

::: browser/themes/shared/places/tree-icons.inc.css:8
(Diff revision 4)
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +treechildren::-moz-tree-image(title) {
> +  list-style-image: url("chrome://mozapps/skin/places/defaultFavicon.svg");
> +  padding-inline-end: 2px;
> +  margin: 0px 2px;

nit: margin: 0 2px;

::: browser/themes/shared/places/tree-icons.inc.css:15
(Diff revision 4)
> +  height: 16px;
> +}
> +
> +treechildren:-moz-tree-image {
> +  -moz-context-properties: fill;
> +  fill: #4c4c4c;

need to use a platform color

::: browser/themes/shared/places/tree-icons.inc.css:19
(Diff revision 4)
> +  -moz-context-properties: fill;
> +  fill: #4c4c4c;
> +}
> +
> +treechildren:-moz-tree-image(selected,focus) {
> +  fill: #fff;

ditto

this also seems wrong on Ubuntu when an item is selected but the tree isn't focused

::: browser/themes/shared/places/tree-icons.inc.css:98
(Diff revision 4)
> +  list-style-image: url("chrome://browser/skin/sidebar-folder.svg");
> +}
> +
> +treechildren::-moz-tree-cell-text(title, separator) {
> +  color: ThreeDShadow;
> +  margin: 0px 5px;

nit: margin: 0 5px;

::: browser/themes/shared/places/tree-icons.inc.css:107
(Diff revision 4)
> +  color: HighlightText;
> +}
> +
> +treechildren::-moz-tree-twisty(title, separator) {
> +  -moz-appearance: none;
> +  padding: 0px;

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

Updated

a month ago
Flags: qe-verify? → qe-verify+
QA Contact: brindusa.tot
(Assignee)

Updated

a month ago
Blocks: 1377003
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 18

a month ago
mozreview-review-reply
Comment on attachment 8882514 [details]
Bug 1377011 - [Photon] Update sidebar tree icons on all platforms.

https://reviewboard.mozilla.org/r/153632/#review163484

> ditto
> 
> this also seems wrong on Ubuntu when an item is selected but the tree isn't focused

Why? The item remains highlighted, shouldn't the highlight fill also persist?
Created attachment 8887866 [details]
screenshot

(In reply to Nihanth Subramanya [:nhnt11] from comment #18)
> Comment on attachment 8882514 [details]
> Bug 1377011 - [Photon] Update sidebar tree icons on all platforms.
> 
> https://reviewboard.mozilla.org/r/153632/#review163484
> 
> > ditto
> > 
> > this also seems wrong on Ubuntu when an item is selected but the tree isn't focused
> 
> Why? The item remains highlighted, shouldn't the highlight fill also persist?

The problem is that the highlight fill doesn't persist.

Updated

a month ago
Attachment #8882514 - Flags: review?(dao+bmo)
Comment hidden (mozreview-request)
(In reply to Dão Gottwald [::dao] from comment #19)
> Created attachment 8887866 [details]
> screenshot
> 
> (In reply to Nihanth Subramanya [:nhnt11] from comment #18)
> > Comment on attachment 8882514 [details]
> > Bug 1377011 - [Photon] Update sidebar tree icons on all platforms.
> > 
> > https://reviewboard.mozilla.org/r/153632/#review163484
> > 
> > > ditto
> > > 
> > > this also seems wrong on Ubuntu when an item is selected but the tree isn't focused
> > 
> > Why? The item remains highlighted, shouldn't the highlight fill also persist?
> 
> The problem is that the highlight fill doesn't persist.

This is still an issue in the Library window.

Updated

25 days ago
Iteration: 56.3 - Jul 24 → 56.4 - Aug 1
Comment hidden (mozreview-request)

Comment 23

25 days ago
mozreview-review
Comment on attachment 8882514 [details]
Bug 1377011 - [Photon] Update sidebar tree icons on all platforms.

https://reviewboard.mozilla.org/r/153632/#review166246

::: browser/themes/shared/places/tree-icons.inc.css:19
(Diff revision 9)
> +  -moz-context-properties: fill;
> +  fill: -moz-FieldText;
> +}
> +
> +treechildren:-moz-tree-image(selected,focus) {
> +  fill: HighlightText;

Turns out this is wrong on Windows where we don't use highlighttext for selected tree items. Depends on the OS theme though.
Attachment #8882514 - Flags: review?(dao+bmo) → review-
(Assignee)

Comment 24

24 days ago
(In reply to Dão Gottwald [::dao] from comment #23)
> Comment on attachment 8882514 [details]
> Bug 1377011 - [Photon] Update sidebar tree icons on all platforms.
> 
> https://reviewboard.mozilla.org/r/153632/#review166246
> 
> ::: browser/themes/shared/places/tree-icons.inc.css:19
> (Diff revision 9)
> > +  -moz-context-properties: fill;
> > +  fill: -moz-FieldText;
> > +}
> > +
> > +treechildren:-moz-tree-image(selected,focus) {
> > +  fill: HighlightText;
> 
> Turns out this is wrong on Windows where we don't use highlighttext for
> selected tree items. Depends on the OS theme though.

In the interest of streamlining my workflow, what do you think about landing this and fixing the OS specific breakage in the respective sidebar restyle bugs? e.g. bug 1377003 for linux.

The patch there already had the fix for the highlight fill not persisting, I moved it to this patch after your review comment.
Flags: needinfo?(dao+bmo)
(In reply to Nihanth Subramanya [:nhnt11] from comment #24)
> In the interest of streamlining my workflow, what do you think about landing
> this and fixing the OS specific breakage in the respective sidebar restyle
> bugs? e.g. bug 1377003 for linux.

We could do that, but beta uplift is early next week. So we might have to uplift followups.
Flags: needinfo?(dao+bmo)
Comment hidden (mozreview-request)

Comment 27

24 days ago
mozreview-review
Comment on attachment 8882514 [details]
Bug 1377011 - [Photon] Update sidebar tree icons on all platforms.

https://reviewboard.mozilla.org/r/153632/#review166690
Attachment #8882514 - Flags: review?(dao+bmo) → review+
(Assignee)

Updated

24 days ago
Blocks: 1384504

Comment 28

24 days ago
Pushed by nhnt11@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/53c50961ee5c
[Photon] Update sidebar tree icons on all platforms. r=dao
backed out for frequent failures like https://treeherder.mozilla.org/logviewer.html#?job_id=118013437&repo=autoland
Flags: needinfo?(nhnt11)

Comment 30

24 days ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e3dadcaf2726
Backed out changeset 53c50961ee5c for test failures in browser_all_files_referenced.js | there should be no unreferenced files - Got 2, expected 0
Comment hidden (mozreview-request)

Comment 32

23 days ago
Pushed by nhnt11@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1d04c6559f71
[Photon] Update sidebar tree icons on all platforms. r=dao
(Assignee)

Updated

23 days ago
Flags: needinfo?(nhnt11)
Backed out for failing browser-chrome's browser/base/content/test/static/browser_parsable_css.js on Linux because allBookmarks.png missing:

https://hg.mozilla.org/integration/autoland/rev/7f9034bfd0c60fc5ec4097640f1197833e060f4a

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=1d04c6559f7174af05c527917bd3b30fb25d8c7c&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=118524409&repo=autoland
> TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_parsable_css.js | missing chrome://browser/skin/places/allBookmarks.png referenced from chrome://browser/skin/places/places.css -

This fails because the Linux jar.mn doesn't include allBookmarks.png: https://hg.mozilla.org/integration/autoland/file/1d04c6559f71/browser/themes/linux/jar.mn
But this gets referenced now by the tree-icons.inc.css in the Linux places.css: https://hg.mozilla.org/integration/autoland/rev/1d04c6559f7174af05c527917bd3b30fb25d8c7c#l3.58

(Haven't checked the status on OS X.)
Flags: needinfo?(nhnt11)

Comment 34

22 days ago
Backout by kwierso@gmail.com:
https://hg.mozilla.org/mozilla-central/rev/0c61f00309fc
Backed out changeset 1d04c6559f71 for failing browser-chrome's browser/base/content/test/static/browser_parsable_css.js on Linux because allBookmarks.png missing. r=backout

Comment 35

22 days ago
backoutbugherder
backed out from m-c
https://hg.mozilla.org/mozilla-central/rev/7f9034bfd0c6
Comment hidden (mozreview-request)
(Assignee)

Updated

21 days ago
Blocks: 1385518
(Assignee)

Updated

21 days ago
Depends on: 1385519
(Assignee)

Comment 37

20 days ago
Comment on attachment 8882514 [details]
Bug 1377011 - [Photon] Update sidebar tree icons on all platforms.

Dao, there was a mistake in the original patch: tree-icons.inc.css used the styles from macOS, which used allBookmarks.png. This was fine on Windows, but on Linux we were using Toolbar-small.png for that image.

In the latest patch I've cropped out the relevant 16x16px image from Toolbar-small.png and added it (after pngcrushing) as allBookmarks.png in Linux. That was the last remaining use of Toolbar-small.png anyway. We can replace the last few png images from tree-icons.inc.css in bug 1385519.

Re-requesting review for this change.
Flags: needinfo?(nhnt11)
Attachment #8882514 - Flags: review+ → review?(dao+bmo)

Comment 38

20 days ago
mozreview-review
Comment on attachment 8882514 [details]
Bug 1377011 - [Photon] Update sidebar tree icons on all platforms.

https://reviewboard.mozilla.org/r/153632/#review168114
Attachment #8882514 - Flags: review?(dao+bmo) → review+

Comment 39

20 days ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6ee53b46474f
[Photon] Update sidebar tree icons on all platforms. r=dao

Comment 40

19 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6ee53b46474f
Status: ASSIGNED → RESOLVED
Last Resolved: 19 days ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56

Comment 41

9 days ago
I can see the "Update sidebar tree icons"  in Latest Nightly 57.0a1 on Windows 10, 64-bit  

Build ID    :	20170809100326
User Agent  : 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
QA Whiteboard: [bugday-20170802]
QA Contact: brindusa.tot → ovidiu.boca
You need to log in before you can comment on or make changes to this bug.