Closed Bug 1698814 Opened 2 years ago Closed 1 year ago

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

Categories

(Firefox :: Theme, task, P3)

task
Points:
2

Tracking

()

RESOLVED FIXED
92 Branch
Iteration:
91.1 - May 31 - Jun 13
Tracking Status
firefox92 --- fixed

People

(Reporter: ntim, Assigned: bugzilla)

References

(Blocks 1 open bug)

Details

(Whiteboard: [proton-cleanups])

Attachments

(7 files, 1 obsolete file)

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.

Blocks: proton-cleanups
No longer blocks: proton-address-bar
Whiteboard: [proton-address-bar] → [proton-cleanups]
Priority: -- → P3

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

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?

Flags: needinfo?(htwyford)

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.

Assignee: nobody → htwyford
Severity: -- → N/A
Status: NEW → ASSIGNED
Iteration: --- → 91.1 - May 31 - Jun 13
Points: --- → 1
Flags: needinfo?(htwyford)
Points: 1 → 2
Attachment #9225634 - Attachment description: WIP: Bug 1698814 - Part 1 - Consolidate toolbar-field background color variables. r?#desktop-theme-reviewers! → Bug 1698814 - Part 1 - Consolidate toolbar-field background color variables. r?#desktop-theme-reviewers!
Attachment #9225635 - Attachment description: WIP: Bug 1698814 - Part 2 - Consolidate toolbar-field color variables. r?#desktop-theme-reviewers! → Bug 1698814 - Part 2 - Consolidate toolbar-field color variables. r?#desktop-theme-reviewers!
Attachment #9225636 - Attachment description: WIP: Bug 1698814 - Part 3 - Consolidate toolbar-field border variables. r?#desktop-theme-reviewers! → Bug 1698814 - Part 3 - Consolidate toolbar-field border variables. r?#desktop-theme-reviewers!
Attachment #9225637 - Attachment description: WIP: Bug 1698814 - Part 4 - Consolidate toolbar-field highlight variables. r?#desktop-theme-reviewers! → Bug 1698814 - Part 4 - Consolidate toolbar-field highlight variables. r?#desktop-theme-reviewers!
Attachment #9225638 - Attachment description: WIP: Bug 1698814 - Part 5 - Remove !proton blocks in urlbar-searchbar and further consolidate rules. r?#desktop-theme-reviewers! → Bug 1698814 - Part 5 - Remove !proton blocks in urlbar-searchbar and further consolidate rules. r?#desktop-theme-reviewers!
Attachment #9225637 - Attachment description: Bug 1698814 - Part 4 - Consolidate toolbar-field highlight variables. r?#desktop-theme-reviewers! → Bug 1698814 - Part 4 - Rename toolbar-field highlight variables to match new naming scheme. r?#desktop-theme-reviewers!
Attachment #9225638 - Attachment description: Bug 1698814 - Part 5 - Remove !proton blocks in urlbar-searchbar and further consolidate rules. r?#desktop-theme-reviewers! → Bug 1698814 - Part 5 - Clean up urlbar-searchbar.inc.css. r?#desktop-theme-reviewers!
Attachment #9225637 - Attachment is obsolete: true
Attachment #9225638 - Attachment description: Bug 1698814 - Part 5 - Clean up urlbar-searchbar.inc.css. r?#desktop-theme-reviewers! → Bug 1698814 - Part 4 - Clean up urlbar-searchbar.inc.css. r?#desktop-theme-reviewers!
Pushed by htwyford@mozilla.com:
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
Regressions: 1724121

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?

Flags: needinfo?(htwyford)

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
htwyford@mozilla.com
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

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

(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.

Flags: needinfo?(htwyford)
Regressions: 1724878

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!

(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?

Flags: needinfo?(htwyford)
Attached image fonts.png

Please don't change it back!

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

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.

Flags: needinfo?(htwyford) → needinfo?(grayshade)

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").

Flags: needinfo?(grayshade)

@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.

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

Regressions: 1725896
Blocks: 1742147
You need to log in before you can comment on or make changes to this bug.