Closed
Bug 1368945
Opened 7 years ago
Closed 7 years ago
Sidebar header shouldn't use the same icon for web content as for the Bookmarks sidebar
Categories
(Firefox :: Theme, defect, P1)
Firefox
Theme
Tracking
()
People
(Reporter: dao, Assigned: dao)
References
Details
(Whiteboard: [reserve-photon-structure])
Attachments
(1 file)
Updated•7 years ago
|
Flags: qe-verify?
Priority: -- → P3
Whiteboard: [photon-structure][triage] → [reserve-photon-structure]
Updated•7 years ago
|
Flags: qe-verify? → qe-verify+
QA Contact: gwimberly
Comment 1•7 years ago
|
||
I think the ideal icon should be the favicon for the website instead of the bookmarks icon.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Updated•7 years ago
|
Priority: P3 → P1
Comment 3•7 years ago
|
||
mozreview-review |
Comment on attachment 8873324 [details] Bug 1368945 - Use default favicon for web content loaded in the sidebar. https://reviewboard.mozilla.org/r/144788/#review148778 Thanks! ::: browser/themes/shared/sidebar.inc.css:117 (Diff revision 1) > #sidebar-switcher-bookmarks > .toolbarbutton-icon, > -#sidebar-box[sidebarcommand="viewWebPanelsSidebar"] > #sidebar-header > #sidebar-switcher-target > #sidebar-icon, > #sidebar-box[sidebarcommand="viewBookmarksSidebar"] > #sidebar-header > #sidebar-switcher-target > #sidebar-icon { > list-style-image: url(chrome://browser/skin/bookmark.svg); > + -moz-context-properties: fill; > + fill: currentColor; How about deduplicating the fill related rules? #sidebar-box:not([sidebarcommand="viewWebPanelsSidebar"]) #sidebar-icon { -moz-context-properties: fill; fill: currentColor; opacity: 0.8; } This doesn't sound as great after writing this comment (because the selector is long if you want to include the intermediate > bla > bla), so I'm not opening an issue.
Attachment #8873324 -
Flags: review?(nhnt11) → review+
Assignee | ||
Comment 4•7 years ago
|
||
Yeah, I tried something similar, but the end result wasn't really cleaner than what my patch is doing now. I think I'll keep it as is.
Pushed by dgottwald@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/05affb78c078 Use default favicon for web content loaded in the sidebar. r=nhnt11
Comment 6•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/05affb78c078
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Updated•7 years ago
|
Iteration: --- → 55.7 - Jun 12
Hi Dão, Can you clarify on the STR needed to verify the bug? Are we checking the icons for the other Headers? (ie: History, Synced Tabs, etc) If not, can you clarify what I'm supposed to be looking for, here? I see websites being mentioned in other comments, but not entirely sure how to get that on the sidebar.
Flags: needinfo?(dao+bmo)
Assignee | ||
Comment 8•7 years ago
|
||
(In reply to Grover Wimberly IV [:Grover-QA] from comment #7) > Hi Dão, > > Can you clarify on the STR needed to verify the bug? Are we checking the > icons for the other Headers? (ie: History, Synced Tabs, etc) If not, can you > clarify what I'm supposed to be looking for, here? I see websites being > mentioned in other comments, but not entirely sure how to get that on the > sidebar. Open a bookmark's properties dialog, check "Load this bookmark in the sidebar", then open the bookmark.
Flags: needinfo?(dao+bmo)
Thanks. Verified with Windows 10 64bit, Mac OS X 10.13, Ubuntu 16.04 64bit using Nightly 58.0a1 (64-bit) with today's build.
status-firefox58:
--- → verified
You need to log in
before you can comment on or make changes to this bug.
Description
•