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)

defect

Tracking

()

RESOLVED FIXED
Firefox 55
Iteration:
55.7 - Jun 12
Tracking Status
firefox55 --- fixed
firefox58 --- verified

People

(Reporter: dao, Assigned: dao)

References

Details

(Whiteboard: [reserve-photon-structure])

Attachments

(1 file)

Flags: qe-verify?
Priority: -- → P3
Whiteboard: [photon-structure][triage] → [reserve-photon-structure]
Flags: qe-verify? → qe-verify+
QA Contact: gwimberly
I think the ideal icon should be the favicon for the website instead of the bookmarks icon.
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Priority: P3 → P1
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+
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
https://hg.mozilla.org/mozilla-central/rev/05affb78c078
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
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)
(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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: