1698814 - Clean up --lwt-toolbar-field-* variables after proton

Status: RESOLVED FIXED

(Firefox :: Theme, task, P3)

Description

[Tim Nguyen :ntim]

Example for --lwt-toolbar-field-background-color

// browser.css:
:root {
  --toolbar-field-background-color: Field;
}
:root:-moz-lwtheme {
  --toolbar-field-background-color: <fallback-for-lwtheme>; // I think it's reasonable to remove the fallback hover state for lwtheme, since the default theme doesn't have it.
}

// browser-custom-colors.css:
:root {
  --toolbar-field-background-color: <default-proton-background>;
}

// Update LightweightThemeConsumer/ThemeVariableMap to refer to --toolbar-field-background-color:
https://searchfox.org/mozilla-central/rev/b3b6a9b1d0f54b8a432f11ced9283bd43b434bb0/toolkit/modules/LightweightThemeConsumer.jsm#110

--toolbar-field-non-lwt-bgcolor & --lwt-toolbar-field-background-color can both removed for this specific case.

I'm sure more can be done with other --lwt-toolbar-field-* vars.

Comment 1

[Harry Twyford]

There's more context about this change in this review comment.

Comment 2

[:Gijs (he/him)]

I don't really follow comment 0. comment 1 links to a phab comment that repeats:

it's historically been there to support the fallback hover state for lightweight themes that don't define the var, but I think with proton, it's reasonable to remove that hover state.

but I don't understand that either - why can we remove the hover state with proton? Toolbar buttons still have (and should have) hover states...

We're removing non-proton code at the moment, so comment #0 separating out Field and a proton value is also confusing.

Harry, if you understand what needs to happen here, can you explain what the purpose of this bug is (perhaps adjusting the summary), and assign a point value corresponding to the rough number of days it would take?

Comment 3

[Harry Twyford]

Currently, we have both --toolbar-field-non-lwt-bgcolor and --lwt-toolbar-field-background-color. They have different assignments, but are used for the same thing. As far as I understand it, this bug and bug 1698820 are about consolidating these two variables into one. Now is the time to do this because we have a better pattern for these variables in Proton: defining a fallback in browser.inc.css or global.inc.css, then overriding it in:

1. LightweightThemeConsumer
2. browser-custom-colors.inc.css
3. In the built-in theme manifests as an experimental property, if necessary.

As a result, we can set the bgcolor of toolbar fields in a single place, rather than in both a :-moz-lwtheme block and a normal block. I'll take this one, which could then be used as a template for bug 1698820.

As for the hover state mentioned, I think Tim was referring to how we used to subtly change the bgcolor of the address bar when it was hovered. That hover state was removed in Proton.

Comment 4

[Harry Twyford]

Attachment: Bug 1698814 - Part 1 - Consolidate toolbar-field background color variables. r?#desktop-theme-reviewers!

Comment 5

[Harry Twyford]

Attachment: Bug 1698814 - Part 2 - Consolidate toolbar-field color variables. r?#desktop-theme-reviewers!

Depends on D117028

Comment 6

[Harry Twyford]

Attachment: Bug 1698814 - Part 3 - Consolidate toolbar-field border variables. r?#desktop-theme-reviewers!

Depends on D117029

Comment 7

[Harry Twyford]

Attachment: Bug 1698814 - Part 4 - Rename toolbar-field highlight variables to match new naming scheme. r?#desktop-theme-reviewers!

Depends on D117030

Comment 8

[Harry Twyford]

Attachment: Bug 1698814 - Part 4 - Clean up urlbar-searchbar.inc.css. r?#desktop-theme-reviewers!

Depends on D117031

Comment 9

[Harry Twyford]

https://treeherder.mozilla.org/jobs?repo=try&revision=094be62586ab5ac636d3fe6d3ee1085c6fee3aab

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/adcdec3642b1
Part 1 - Consolidate toolbar-field background color variables. r=desktop-theme-reviewers,dao
https://hg.mozilla.org/integration/autoland/rev/06438078a0f8
Part 2 - Consolidate toolbar-field color variables. r=desktop-theme-reviewers,dao
https://hg.mozilla.org/integration/autoland/rev/26c024a20ea2
Part 3 - Consolidate toolbar-field border variables. r=desktop-theme-reviewers,dao
https://hg.mozilla.org/integration/autoland/rev/c7687ceeff3f
Part 4 - Clean up urlbar-searchbar.inc.css. r=desktop-theme-reviewers,Itiel

Comment 11

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/adcdec3642b1
https://hg.mozilla.org/mozilla-central/rev/06438078a0f8
https://hg.mozilla.org/mozilla-central/rev/26c024a20ea2
https://hg.mozilla.org/mozilla-central/rev/c7687ceeff3f

Comment 12

[:Gijs (he/him)]

This landed the day before soft freeze, and already has regressions filed against it. Are we comfortable that those regressions and any other ones that get filed can be addressed before 92 goes to beta, or should we back out from nightly and reland on Monday, so we have more time to deal with the fallout?

Comment 13

[[e-mail]]

Attachment: This patchset is causing the urlsearch bar to have a light colored background in my dark themed nightly.

I've been having an issue with a white flash on startup, https://bugzilla.mozilla.org/show_bug.cgi?id=1720768. Today, after compiling the latest nightly, I see this happening in the urlsearchbar. I reported it there, but think it is more suitable here after I ran mozregression.

Here is the regression information.
Was this integration build good, bad, or broken? (type 'good', 'bad', 'skip', 'retry', 'back' or 'exit' and press Enter): good
5:44.71 INFO: Narrowed integration regression window from [a1b96df5, c7687cee] (4 builds) to [6ffeb2a3, c7687cee] (2 builds) (~1 steps left)
5:44.71 INFO: No more integration revisions, bisection finished.
5:44.71 INFO: Last good revision: 6ffeb2a3d80381264c0ad9329b4d07b3f343a4ca
5:44.71 INFO: First bad revision: c7687ceeff3f99555aa7d3f744da33952aef236f
5:44.71 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=6ffeb2a3d80381264c0ad9329b4d07b3f343a4ca&tochange=c7687ceeff3f99555aa7d3f744da33952aef236f

Mercurial > integration > autoland / pushlog
summary | shortlog | changelog | pushlog | graph | tags | bookmarks | branches | files | zip | help
Page 1
From: To:
Changes pushed after changeset 6ffeb2a3d80381264c0ad9329b4d07b3f343a4ca, up to and including changeset c7687ceeff3f99555aa7d3f744da33952aef236f
User
Push date [To Local] Changeset Patch author — Commit message
[e-mail]
Wed Aug 04 18:03:51 2021 +0000 c7687ceeff3f99555aa7d3f744da33952aef236f Harry Twyford — Bug 1698814 - Part 4 - Clean up urlbar-searchbar.inc.css. r=desktop-theme-reviewers,Itiel
26c024a20ea26f7fea6f38f25e1b9a368e0f56e2 Harry Twyford — Bug 1698814 - Part 3 - Consolidate toolbar-field border variables. r=desktop-theme-reviewers,dao
06438078a0f88795cec086b57332009af80f6811 Harry Twyford — Bug 1698814 - Part 2 - Consolidate toolbar-field color variables. r=desktop-theme-reviewers,dao
adcdec3642b1c71ee9344ceb0b207035aaca4742 Harry Twyford — Bug 1698814 - Part 1 - Consolidate toolbar-field background color variables. r=desktop-theme-reviewers,dao
Page 1
autoland
Deployed from 3ea513891845 at 2021-07-08T16:45:28Z.
RSS Atom

Comment 14

[[e-mail]]

Attachment: The custom colors I am using in my nightly instance.

Setting these colors in nightly should allow someone to reproduce the issue.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

That is bug 1724115.

Comment 16

[Harry Twyford]

(In reply to :Gijs (he/him) from comment #12)

This landed the day before soft freeze, and already has regressions filed against it. Are we comfortable that those regressions and any other ones that get filed can be addressed before 92 goes to beta, or should we back out from nightly and reland on Monday, so we have more time to deal with the fallout?

The two regressions are the same issue, and Emilio already has a patch up. I'm confident we can get it fixed before 92 beta. If that fix doesn't land tomorrow or if more regressions are filed, let's back it out.

Comment 17

[Laurențiu Nicola]

This change increased the address bar font weight for me (on Arch Linux). I think it looks a lot better than before, but was it intentional (it's not obvious from the discussion)? If yes, thanks!

Comment 18

[:Gijs (he/him)]

(In reply to Laurențiu Nicola from comment #17)

This change increased the address bar font weight for me (on Arch Linux). I think it looks a lot better than before, but was it intentional (it's not obvious from the discussion)? If yes, thanks!

Harry, can you clarify?

Comment 19

[Laurențiu Nicola]

Attachment: fonts.png

Please don't change it back!

Comment 20

[Laurențiu Nicola]

I don't understand how it works, but it got "fixed" in bug 1724115 :-(.

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Which GTK theme are you using? That's a theming issue, at best. I assume you prefer the left appearance in this case?

I'd just file a separate bug with a title like "Low contrast between 'Field' and 'FieldText' system colors in some gtk themes". I'm happy to poke at it, feel free to ni? me.

Comment 22

[Laurențiu Nicola]

It's Arc. You're right, the text on Arc is actually a bit gray everywhere. I still prefer the bolder version in my screenshot, but on Adwaita, I think it looks more like on Windows ("as intended").

Comment 23

[Laurențiu Nicola]

@emilio I filed https://github.com/jnsh/arc-theme/issues/153, let's wait to see where that goes before trying to work around it in Firefox.

Comment 24

[[e-mail]]

This issue is no longer occurring for me. I think it is as Laurentiu described in @20.