1703590 - Proton: Custom theme missing border and search suggestion/oneoffs separator.

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P2)

Description

[Pablo]

Attachment: separator.jpg

Affected versions

Firefox 88 beta 8
Nightly 89.0a1

Affected platforms

Win 7 64-bit, Mac 10.14, Ubuntu 20. windows 10 64bit

Preconditions
user.js with
user_pref("browser.proton.enabled", true);
user_pref("browser.proton.toolbar.enabled", true);
user_pref("browser.proton.urlbar.enabled", true);

Steps to reproduce

1.launch firefox
2.install "black matte red" theme https://addons.mozilla.org/en-US/firefox/addon/matte-black-red/
3.type something in url bar

Expected result

there should be a horizontal line above one offs, and below search suggestions

Actual result
I dont see any line, the line is visible with proton off.

Notes: this issue is also found on Alpenglow theme too

Comment 1

[Pablo]

Hi romain, could you please set a priority for this here?

Comment 2

[Pablo]

Please note that not only the separator is missing, all the border is missing when i click on the address bar and search suggestions appear.
On release with matte black red theme, i can see a thin grey border when i click on the address bar.

Comment 3

[Marco Bonardo [:mak]]

We are using toolbar_field_separator in the urlbar, while other panels use --panel-separator-color.
toolbar_field_separator is indeed translating to --urlbar-separator-color.
For this theme it is set to rgba(18, 18, 18, 0.2) , very faint on black. It looks like the intent of the theme was to hide the separator between the urlbar field and results.

In the past we didn't apply --urlbar-separator-color to the one-offs border, but now we do: https://searchfox.org/mozilla-central/rev/759872688df15a5d6ab305ffe39d90450590bfec/browser/themes/shared/urlbarView.inc.css#707, thus now the theme also hides the one-offs border.
We apparently fixed a bug here, because these rulers may have looked different in the past, while now they have the same color.

Though this also means some themes will look different. I'm not sure what would be the right solution, we could of course restore the old behavior for non-default themes, but that also means the one-offs top border can't be set by themes and differs from the other rules in the same panel.
Maybe themes should be able to control --panel-separator-color?

Harry, wdyt?

Comment 4

[Marco Bonardo [:mak]]

I'm also not sure why --urlbar-separator-color differs from --panel-separator-color in general, shouldn't panels be consistent in the product?

Comment 5

[Marco Bonardo [:mak]]

The lack of border is because we use arrowpanel-border-color, while we used to use --toolbar-field-focus-border-color
https://searchfox.org/mozilla-central/rev/759872688df15a5d6ab305ffe39d90450590bfec/browser/themes/shared/urlbar-searchbar.inc.css#137

Again, here we are more consistent with the other panels, indeed if you open the hamburger menu with that theme, it doesn't have a visible border. We could revert, with a consistency cost.

Comment 6

[Harry Twyford]

I think deprecating --urlbar-separator-color is reasonable. I imagine the common case for customizing these separators in a theme is to remove them, like the theme in the bug is doing. I don't think that's really a usecase we need to/should support. The separators are there to distinguish elements. IMO a theme should add style to Firefox but not make it less usable, which they can do by hiding the separators.

Let's make --urlbar-separator-color a dimmed currentColor. That way theme authors can still style it by modifying popup_color, but hiding it won't be an option.

Comment 7

[Marco Bonardo [:mak]]

In the next days Caitlin will send communication about the deprecation of toolbar_field_separator.

Comment 8

[Marco Bonardo [:mak]]

Attachment: Bug 1703590 - Fix custom themes urlbar/searchbar separators. r=harry

Stop supporting toolbar_field_separator in themes. We have no more
vertical separators in toolbar fields, and it could hide functional horizontal
separators in the urlbar panel, because it was misused there.

Introduce an autocomplete_popup_separator experimental color instead and use
a color-mix of currentColor to better adapt to LWT theme colors.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/867ad974e957
Fix custom themes urlbar/searchbar separators. r=harry,desktop-theme-reviewers,rpl

Comment 10

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/867ad974e957

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 9216821 [details]
Bug 1703590 - Fix custom themes urlbar/searchbar separators. r=harry

Beta/Release Uplift Approval Request

• User impact if declined: Part of MR1 design
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: see comment 0
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): I'll define this medium because it's deprecating a property of themes. This has been coordinated with the add-ons theme and theme authors are being notified about it. This deprecation has tests.
  Apart from that, this is mostly a low risk css colors fix.
• String changes made/needed:

Comment 12

[Pascal Chevrel:pascalc]

(In reply to Marco Bonardo [:mak] from comment #7)

In the next days Caitlin will send communication about the deprecation of toolbar_field_separator.

Was this communication done? Thanks

Comment 13

[Marco Bonardo [:mak]]

Forwarding to Caitlin

Comment 14

[Sören Hentzschel]

Was this communication done?

https://blog.mozilla.org/addons/2021/04/19/changes-to-themeable-areas-of-firefox-in-version-89/

Comment 15

[Marco Bonardo [:mak]]

That's correct!

Comment 16

[Pascal Chevrel:pascalc]

Comment on attachment 9216821 [details]
Bug 1703590 - Fix custom themes urlbar/searchbar separators. r=harry

Approved for 89 Beta 3, thanks.

Comment 17

[Pascal Chevrel:pascalc]

https://hg.mozilla.org/releases/mozilla-beta/rev/d8169020f3c5

Comment 18

[Pablo]

Verified fix on Firefox 89 Beta 3 and Firefox nightly 90.0a1. Checked on Windows10, MacOS 10.15 and Ubuntu 20. 64bit.
Now there is a separator between suggestions and one-offs. Also checked firefox built in themes and another custom external theme.

Comment 19

[Rob Wu [:robwu]]

Marco, your patch introduces the autocomplete_popup_separator color (and even mentions it in the commit description), but I don't see any reference to this color in the Firefox codebase (in particular, not even in theme.json). Where is/will this color be used? Is there a follow-up bug?

Comment 20

[Marco Bonardo [:mak]]

(In reply to Rob Wu [:robwu] from comment #19)

Marco, your patch introduces the autocomplete_popup_separator color (and even mentions it in the commit description), but I don't see any reference to this color in the Firefox codebase (in particular, not even in theme.json). Where is/will this color be used? Is there a follow-up bug?

it's an experimental color in the default themes, that just maps to --autocomplete-popup-separator-color in manifest.json, you should be able to find the latter in the codebase.
https://searchfox.org/mozilla-central/search?q=autocomplete_popup_separator&path=&case=false&regexp=false
https://searchfox.org/mozilla-central/search?q=autocomplete-popup-separator-color&path=.css
I don't think theme_experiment is directly affecting theme authors, we're not introducing an official new color here.

Note for how the default color is defined (using color-mix) the new separators may better adapt to themes (using the currentColor as base)