Color differences between List All Tabs and Open a New Tab buttons, and the '<' & '>' button

RESOLVED FIXED in Firefox 65

Status

()

defect
P2
normal
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: itiel_yn8, Assigned: chandranvishwaak, Mentored)

Tracking

({good-first-bug, regression})

unspecified
Firefox 65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox63 unaffected, firefox64 unaffected, firefox65 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

Reporter

Description

7 months ago
This is a regression from bug 1505311.

There are 2 STRs for this, which may all be the same:

STR #1:
1. Enable the Dark theme
2. Open many tabs so the tab strip would overflow
3. Scroll the tab strip to the middle, and focus one of the tabs
4. There is a subtle color difference between List All Tabs and Open a New Tab buttons, and the '<', '>' & X button (on the focused tab).

STR #2:
1. Enable the Light theme
2. Open many tabs so the tab strip would overflow
3. Scroll the tab strip to the middle, and focus one of the tabs
4. There is a noticeable color difference between List All Tabs, Open a New Tab & the focused tab X button, and the '<', '>' buttons (I'm not sure what is the intended behaviour here, but previously all buttons were black-ish)
Reporter

Comment 1

7 months ago
Posted image STR #1
Reporter

Comment 2

7 months ago
Posted image STR #2
Reporter

Updated

7 months ago
Has STR: --- → yes
Flags: needinfo?(dao+bmo)
Assignee

Comment 3

7 months ago
i would like to take this up as my first bug. Can i start? and i would like some mentoring
(In reply to chandranvishwaak from comment #3)
> i would like to take this up as my first bug. Can i start? and i would like
> some mentoring

Have you built Firefox already?

Here's what I think is needed to fix this. In browser/themes/windows/compacttheme.css, in the @media (-moz-windows-accent-color-in-titlebar) block (https://searchfox.org/mozilla-central/rev/c0b26c40769a1e5607a1ae8be37fe64df64fc55e/browser/themes/windows/compacttheme.css#7-12), add this rule:

  :root[tabsintitlebar]:not(:-moz-window-inactive) #titlebar {
    --lwt-toolbarbutton-icon-fill: currentColor;
  }
Assignee

Comment 5

7 months ago
i added  the rule and successfully built it on my local system(nightly) with desirable changes. Shall i send my patch ?
Sure!
Assignee: nobody → chandranvishwaak
Mentor: dao+bmo
Flags: needinfo?(dao+bmo)
Keywords: good-first-bug
Priority: -- → P2
Assignee

Comment 7

7 months ago
i am not sure about the process of sending the patch . can i get some help on it ?
Assignee

Comment 9

7 months ago
Posted patch patch_v1.txt (obsolete) — Splinter Review
Attachment #9025766 - Flags: review+
Assignee

Comment 10

7 months ago
Shall i push the patch ?
Ideally you should put the patch up for review at phabricator: https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html

Also, I thought about this a bit more and now think you shouldn't use :not(:-moz-window-inactive) in your new rule. If you drop that part, this should improve performance.

Updated

7 months ago
Attachment #9025766 - Attachment is patch: true
Attachment #9025766 - Flags: review+
Assignee

Comment 12

7 months ago
Comment on attachment 9026049 [details] [diff] [review]
Improved patch

Looks good.
Attachment #9026049 - Flags: review+

Comment 14

7 months ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/335eb23dc594
Override --lwt-toolbarbutton-icon-fill in the title bar with Dark and Light themes. r=dao

Comment 15

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/335eb23dc594
Status: NEW → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Attachment #9025766 - Attachment is obsolete: true
Attachment #9026049 - Attachment is patch: true
Reporter

Comment 16

7 months ago
The issue seems to be fixed, but these icons are now whiter than they used to be, and inconsistent with the other icons color in the toolbar (also the icons on the Customize page seem to be whiter). Was that intended?
See attached.
Flags: needinfo?(dao+bmo)
Reporter

Comment 17

7 months ago
Posted image After the fix
Reporter

Comment 18

7 months ago
(In reply to Itiel from comment #16)
> The issue seems to be fixed, but these icons are now whiter than they used
> to be, and inconsistent with the other icons color in the toolbar (also the
> icons on the Customize page seem to be whiter). Was that intended?
> See attached.

^^ This is relevant to the dark theme.

In the light theme, these icons are all white, while previously they were black-ish. Also the icons on the Customize page are now blacker than they were.
(In reply to Itiel from comment #16)
> The issue seems to be fixed, but these icons are now whiter than they used
> to be, and inconsistent with the other icons color in the toolbar (also the
> icons on the Customize page seem to be whiter). Was that intended?

Yes.

(In reply to Itiel from comment #18)
> Also the icons on the Customize page are now blacker than they were.

I don't think this patch changed anything there.
Flags: needinfo?(dao+bmo)
Reporter

Comment 20

7 months ago
(In reply to Dão Gottwald [::dao] from comment #19)
> (In reply to Itiel from comment #16)
> > The issue seems to be fixed, but these icons are now whiter than they used
> > to be, and inconsistent with the other icons color in the toolbar (also the
> > icons on the Customize page seem to be whiter). Was that intended?
> 
> Yes.

Just to be sure, you're saying there actually _should_ be a color difference between the toolbar buttons and the tab bar buttons? Prior to bug 1505311 they were all grey-ish when in dark theme.

(In reply to Dão Gottwald [::dao] from comment #19)
> (In reply to Itiel from comment #18)
> > Also the icons on the Customize page are now blacker than they were.
> 
> I don't think this patch changed anything there.

Yep, you're right, this is again part of bug 1505311. I'll file a separate bug for this.
Reporter

Updated

7 months ago
Flags: needinfo?(dao+bmo)
(In reply to Itiel from comment #20)
> (In reply to Dão Gottwald [::dao] from comment #19)
> > (In reply to Itiel from comment #16)
> > > The issue seems to be fixed, but these icons are now whiter than they used
> > > to be, and inconsistent with the other icons color in the toolbar (also the
> > > icons on the Customize page seem to be whiter). Was that intended?
> > 
> > Yes.
> 
> Just to be sure, you're saying there actually _should_ be a color difference
> between the toolbar buttons and the tab bar buttons? Prior to bug 1505311
> they were all grey-ish when in dark theme.

The color should be the same as with the default theme when both use the same titlebar background.
Flags: needinfo?(dao+bmo)
Reporter

Comment 22

7 months ago
(In reply to Dão Gottwald [::dao] from comment #21)
> The color should be the same as with the default theme when both use the
> same titlebar background.

Well, that's not the case. The tabbar buttons color is the same for Dark and Light (white), but for the default theme it's the same as Dark theme's toolbar buttons color (grey-ish).
Will file a bug for this as well.
Reporter

Updated

7 months ago
Depends on: 1509321
You need to log in before you can comment on or make changes to this bug.