[Photon] Update sidebar tree icons on all platforms

VERIFIED FIXED in Firefox 56

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: nhnt11, Assigned: nhnt11)

Tracking

(Depends on 1 bug)

Trunk
Firefox 56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

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

Attachments

(2 attachments, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

2 years ago
Assignee: nobody → nhnt11
Status: NEW → ASSIGNED
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 years 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 years 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 years 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

2 years 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)

Comment 8

2 years ago
(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)
Iteration: 56.2 - Jul 10 → 56.3 - Jul 24
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8882515 - Attachment is obsolete: true
(Assignee)

Updated

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

Updated

2 years ago
Depends on: 1381453

Comment 11

2 years 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

2 years 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

2 years 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-
Flags: qe-verify? → qe-verify+
QA Contact: brindusa.tot
(Assignee)

Updated

2 years ago
Blocks: 1377003
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 18

2 years 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?
Posted image 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.
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.
Iteration: 56.3 - Jul 24 → 56.4 - Aug 1
Comment hidden (mozreview-request)

Comment 23

2 years 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

2 years 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

2 years 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

2 years ago
Blocks: 1384504

Comment 28

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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 hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Blocks: 1385518
(Assignee)

Updated

2 years ago
Depends on: 1385519
(Assignee)

Comment 37

2 years 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

2 years 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

2 years 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6ee53b46474f
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56

Comment 41

2 years 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
Status: RESOLVED → VERIFIED
Flags: qe-verify+

Updated

2 years ago
Depends on: 1404621
No longer depends on: 1404621
You need to log in before you can comment on or make changes to this bug.