1886847 - Update sidebar theme colors and styling polish

Status: RESOLVED FIXED

(Firefox :: Sidebar, task)

Description

[Sarah Clements [:sclements]]

This should handling styling polish - so padding/alignment, hover effects (which is missing for tools currently) and updating colors and tokens to use correct variables per the spec. This should apply this styling for sidebar launcher, history, synced tabs and Customize Sidebar panels.

Note: I'll be handling vertical tabs styling in bug 1899336 since I need to do that for pinned tabs.

Comment 1

[Sarah Clements [:sclements]]

We also want to correct for the bookmarks toolbar overlapping the top most icon on the launcher. We can do this by adding the sidebar-launcher element to the existing style in browser/themes/shared/browser-shared.css here, so it will shift down along with the sidebar.

Edit: This will be handled in bug 1890680.

Comment 2

[Sarah Clements [:sclements]]

Sam, if you think there's anything we need to pay attention to post-investigation into themes that you did can you drop a comment in here?

Comment 3

[Sam Foster [:sfoster] (he/him)]

(In reply to Sarah Clements [:sclements] from comment #2)

Sam, if you think there's anything we need to pay attention to post-investigation into themes that you did can you drop a comment in here?

I wrote this up in bug 1893214, but I can repeat here if its going to be more useful. The issue is centered on the requirement to make the transition between the horizontal toolbars and the vertical sidebar launcher appear "seamless". This will work just fine in some themes and not very well with others. In practice the browser UI has a hard line between the top stuff (contents of #navigator-toolbox, which includes the tab strip, addressbar and bookmarks toolbar) and the bottom or main content (which includes the sidebar, scrollbars and tab contents.) That distinction has become baked into how we specify theme colors and how theme authors (and our documentation) make use of those color and image options.

• ... In some cases we can simply use the frame/frame_inactive colors for the sidebar launcher, with the toolbar color on top of that. (Note: not the sidebar color: that will remain in use for the sidebar panels and not for the launcher part which we are treating more like a toolbar)
• However, many (~75%) themes are built with one or more background images, and there isn't a practical way at this point to extend or seamlessly tile those images to provide the background for the sidebar.

To understand the impact of this and help contextualize decisions here, we looked at manifests for all themes available at addons.mozilla.org with over 1000 users/installs

• Themes which define a translucent toolbar (not opaque or fully transparent): 21%
• Themes which define a fully opaque toolbar color: 35%
• Themes which define either theme_frame or additional_images: 78%
• Themes which define a non-opaque toolbar color and background image(s): 20.7%

So, while some themes define an opaque toolbar color (see mdn docs on the manifest theme colors), most are using a background image. And of those most almost all are using the toolbar color as a translucent overlay. This will need to feed into how we implement the logic for how we derive the sidebar launcher's color from the theme's colors.

Comment 4

[Sarah Clements [:sclements]]

Thanks Sam, I think what we need next is a clearly defined proposal of sidebar variables with any toolbar fallbacks to run by Yulia/Ania. (Per previous discussions, this would exclude trying to seamlessly integrate background images for now.) Is this something you're interested in taking on?

Comment 5

[Sam Foster [:sfoster] (he/him)]

(In reply to Sarah Clements [:sclements] from comment #4)

Thanks Sam, I think what we need next is a clearly defined proposal of sidebar variables with any toolbar fallbacks to run by Yulia/Ania. (Per previous discussions, this would exclude trying to seamlessly integrate background images for now.) Is this something you're interested in taking on?
I can take this on, but I think I'll need to get with Yulia to discuss before I can draft anything like a clear proposal. I think I have a good understanding of the variables now, but not how we should navigate them.

Comment 6

[Ed Lee :Mardak]

I noticed the spec includes 8px rounded corners for the sidebar panels (and also tab content's top sidebar-adjacent corner per spec linked in comment 0) https://www.figma.com/design/ZFavpnAJee39j2mFf8U3D0/Sidebar-UX-Spec?node-id=10850-379448

Is this bug covering that rounding polish as well as spacing adjustments between the sidebar and tab content for both collapsed and expanded? Or are there other existing bugs (or potentially future bugs split out from this)?

Comment 7

[Sarah Clements [:sclements]]

(In reply to Ed Lee :Mardak from comment #6)

I noticed the spec includes 8px rounded corners for the sidebar panels (and also tab content's top sidebar-adjacent corner per spec linked in comment 0) https://www.figma.com/design/ZFavpnAJee39j2mFf8U3D0/Sidebar-UX-Spec?node-id=10850-379448

Is this bug covering that rounding polish as well as spacing adjustments between the sidebar and tab content for both collapsed and expanded? Or are there other existing bugs (or potentially future bugs split out from this)?

When you say spacing adjustments between the sidebar and tab content do you mean vertical tabs? I'm handling the vertical/pinned tabs styling in bug 1899336 as mentioned above since its changing pinned tabs, adding a new tab button, etc. There will be another bug filed for theming adjustments since we expect we'll need some fallbacks for themes that don't define certain variables and once UX reviews the finished work in Nightly, if they want anything adjusted. We don't have anything on file specifically for the chatbot panel though - I'm assuming you'll handle that?

Comment 8

[Ed Lee :Mardak]

Attachment: spacing around panel.png

I see the figma now has a dedicated visual spec for the spacing that I was thinking about "8px between panels and page content" although previously I didn't notice there's also 8px below. The design happens to be on "Chatbot" but I would think it applies to all panels https://www.figma.com/design/ZFavpnAJee39j2mFf8U3D0/Sidebar-UX-Spec?node-id=10850-387630

(In reply to Sarah Clements [:sclements] from comment #7)

We don't have anything on file specifically for the chatbot panel though - I'm assuming you'll handle that?

Yeah, chatbot-specific styling will be separate, but just to be clear, what's the separation? Inside content vs outside panel container? For example, there's rounded corners and shadows in the margins around the panels, so presumably chatbot doesn't need to specially set some border-radius within content, but chatbot styling should be aware of the rounding to avoid getting stuff cropped off.

Comment 9

[Nikki Sharpley (:nikkis) (she/her)]

Attachment: Bug 1886847 - Styling and UI polish for sidebar r=#sidebar-reviewers

• fixed empty states image fill and stroke
• styled search
• styled customize settings
• added rounded corners and shadows to panel
• mask top left corner of content for rounded corner effect

Comment 10

[Sarah Clements [:sclements]]

FYI Nikki, Will mentioned that this test will need to be updated for any changes we make to the sidebar: https://searchfox.org/mozilla-central/rev/8c6edfe25c094e032a27722ef30f69555f556bf8/toolkit/components/extensions/test/browser/browser_ext_themes_sidebars.js

Comment 11

[William Durand [:willdurand]]

:nikkis hey, per Comment 10, could you look into that test as part of your patch here? Thank you!

Comment 12

[Nikki Sharpley (:nikkis) (she/her)]

:willdurand, thanks for the reminder! I've had a look and it seems this test is already failing on central when I test locally, independent of my changes. Looking into why, but if you have any insights they'd be most welcome.

Comment 13

[Nikki Sharpley (:nikkis) (she/her)]

NOTE: I'm looking into sidebar and panel fallbacks as part of Bug 1906623. It might make sense to add to https://searchfox.org/mozilla-central/rev/8c6edfe25c094e032a27722ef30f69555f556bf8/toolkit/components/extensions/test/browser/browser_ext_themes_sidebars.js a test for the panels themselves once this is in place. The current fallbacks -moz-sidebar and -moz-sidebartext have not been changed and hence the current tests are unaffected.

Comment 14

[William Durand [:willdurand]]

I don't think the test is failing on central currently. What's the error you're getting? It passes for me locally and a recent Try push also confirms that. For me, it's only failing when the sidebar.revamp pref is set, which is expected given that this bug is about fixing theming properties for the new sidebar, right?

Comment 15

[Nikki Sharpley (:nikkis) (she/her)]

I'm getting the following error locally when on central, without the sidebar.revamp pref set:

  FAIL Sidebar text color should be set. - Got "rgba(0, 0, 0, 0.847)", expected "rgba(255, 255, 255, 0.847)"
Stack trace:
chrome://mochikit/content/browser-test.js:test_is:1625
chrome://mochitests/content/browser/toolkit/components/extensions/test/browser/browser_ext_themes_sidebars.js:test_sidebar_theme:136
chrome://mochitests/content/browser/toolkit/components/extensions/test/browser/browser_ext_themes_sidebars.js:test_support_sidebar_colors:221
chrome://mochikit/content/browser-test.js:handleTask:1145
chrome://mochikit/content/browser-test.js:_runTaskBasedTest:1217
chrome://mochikit/content/browser-test.js:Tester_execTest:1358
chrome://mochikit/content/browser-test.js:nextTest/<:1134
chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/<:1058

Comment 16

[Kagami Rosylight [:saschanaz] (they/them)]

Could be depending on system theme (light vs dark)? Just guessing.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/397b2df59338
Styling and UI polish for sidebar r=sidebar-reviewers,desktop-theme-reviewers,fxview-reviewers,reusable-components-reviewers,dao,settings-reviewers,tgiles,mconley,jsudiaman,kcochrane

Comment 18

[Atila Butkovits]

Backed out for causing failures at browser-custom-colors.css.

Backout link: https://hg.mozilla.org/integration/autoland/rev/640bdd44c9182ec08dade9adbe226f0522b37307

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=397b2df59338dfe7869a1b1155a7ff226f31ad7e&selectedTaskRun=Gl1fLcsUSE-nu2Bxdaj71g.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=469003385&repo=autoland&lineNumber=11227

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cad076214b85
Styling and UI polish for sidebar r=sidebar-reviewers,desktop-theme-reviewers,fxview-reviewers,reusable-components-reviewers,dao,settings-reviewers,tgiles,mconley,jsudiaman,kcochrane

Comment 20

[Nikki Sharpley (:nikkis) (she/her)]

Attachment: Bug 1886847 - Truncate labels and headings for sidebar r=#sidebar-reviewers

Comment 21

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/cad076214b85

Comment 22

[Phabricator Automation]

Comment on attachment 9417570 [details]
Bug 1886847 - Truncate labels and headings for sidebar r=#sidebar-reviewers

Revision D218469 was moved to bug 1912902. Setting attachment 9417570 [details] to obsolete.