Closed
Bug 1377011
Opened 7 years ago
Closed 7 years ago
[Photon] Update sidebar tree icons on all platforms
Categories
(Firefox :: Theme, defect, P1)
Firefox
Theme
Tracking
()
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 | ||
Updated•7 years ago
|
Assignee: nobody → nhnt11
Status: NEW → ASSIGNED
Updated•7 years 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•7 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•7 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•7 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•7 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)
(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•7 years ago
|
Iteration: 56.2 - Jul 10 → 56.3 - Jul 24
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8882515 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8882516 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment 11•7 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•7 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•7 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-
Updated•7 years ago
|
Flags: qe-verify? → qe-verify+
QA Contact: brindusa.tot
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 18•7 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?
Comment 19•7 years ago
|
||
(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•7 years ago
|
Attachment #8882514 -
Flags: review?(dao+bmo)
Comment hidden (mozreview-request) |
Comment 21•7 years ago
|
||
(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•7 years ago
|
Iteration: 56.3 - Jul 24 → 56.4 - Aug 1
Comment hidden (mozreview-request) |
Comment 23•7 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•7 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)
Comment 25•7 years ago
|
||
(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•7 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+
Comment 28•7 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
Comment 29•7 years ago
|
||
backed out for frequent failures like https://treeherder.mozilla.org/logviewer.html#?job_id=118013437&repo=autoland
Flags: needinfo?(nhnt11)
Comment 30•7 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•7 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•7 years ago
|
Flags: needinfo?(nhnt11)
Comment 33•7 years ago
|
||
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•7 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 35•7 years ago
|
||
backout bugherder |
backed out from m-c https://hg.mozilla.org/mozilla-central/rev/7f9034bfd0c6
Comment hidden (mozreview-request) |
Assignee | ||
Comment 37•7 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•7 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•7 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6ee53b46474f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Comment 41•7 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]
Updated•7 years ago
|
QA Contact: brindusa.tot → ovidiu.boca
Updated•7 years ago
|
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in
before you can comment on or make changes to this bug.
Description
•