Closed Bug 1703590 Opened 8 months ago Closed 8 months ago

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

Categories

(Firefox :: Address Bar, defect, P2)

Desktop
All
defect

Tracking

()

VERIFIED FIXED
90 Branch
Tracking Status
firefox87 --- disabled
firefox88 --- disabled
firefox89 --- verified
firefox90 --- verified

People

(Reporter: pablo.muir, Assigned: mak)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [proton-address-bar] [priority:2c] [a11y] [proton-uplift])

Attachments

(2 files)

Attached image 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

Whiteboard: [proton-address-bar]

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

Severity: -- → S4
Flags: needinfo?(rtestard)
Flags: needinfo?(rtestard)
Priority: -- → P2
Whiteboard: [proton-address-bar] → [proton-address-bar] [priority:2c] [a11y]

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.

Severity: S4 → S3
Summary: Proton: Custom theme missing search suggestion/oneoffs separator. → Proton: Custom theme missing border and search suggestion/oneoffs separator.

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?

Flags: needinfo?(htwyford)

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

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.

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.

Flags: needinfo?(htwyford)

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

Assignee: nobody → mak
Status: NEW → ASSIGNED

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.

Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/867ad974e957
Fix custom themes urlbar/searchbar separators. r=harry,desktop-theme-reviewers,rpl
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → 90 Branch

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:
Attachment #9216821 - Flags: approval-mozilla-beta?
Flags: qe-verify+
Whiteboard: [proton-address-bar] [priority:2c] [a11y] → [proton-address-bar] [priority:2c] [a11y] [proton-uplift]
QA Whiteboard: [qa-triaged]
Blocks: 1706618

(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

Flags: needinfo?(mak)

Forwarding to Caitlin

Flags: needinfo?(mak) → needinfo?(cneiman)

That's correct!

Flags: needinfo?(cneiman)

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

Approved for 89 Beta 3, thanks.

Attachment #9216821 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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.

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
Flags: qe-verify+

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?

Flags: needinfo?(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)

Flags: needinfo?(mak)
You need to log in before you can comment on or make changes to this bug.