Closed Bug 1377011 Opened 7 years ago Closed 7 years ago

[Photon] Update sidebar tree icons on all platforms

Categories

(Firefox :: Theme, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 56
Iteration:
56.4 - Aug 1
Tracking Status
firefox56 --- fixed

People

(Reporter: nhnt11, Assigned: nhnt11)

References

()

Details

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

Attachments

(2 files, 2 obsolete files)

      No description provided.
Assignee: nobody → nhnt11
Status: NEW → ASSIGNED
Iteration: 56.1 - Jun 26 → 56.2 - Jul 10
Flags: qe-verify?
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 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 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)
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)
Iteration: 56.2 - Jul 10 → 56.3 - Jul 24
Attachment #8882515 - Attachment is obsolete: true
Attachment #8882516 - Attachment is obsolete: true
Depends on: 1381453
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-
(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 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
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?
Attached 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)
(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 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-
(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 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+
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)
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
Pushed by nhnt11@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1d04c6559f71
[Photon] Update sidebar tree icons on all platforms. r=dao
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)
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
Depends on: 1385519
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 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+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6ee53b46474f
[Photon] Update sidebar tree icons on all platforms. r=dao
https://hg.mozilla.org/mozilla-central/rev/6ee53b46474f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
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+
Depends on: 1404621
No longer depends on: 1404621
You need to log in before you can comment on or make changes to this bug.