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

Status: RESOLVED FIXED

(Firefox :: Theme, defect, P2)

Description

[Itiel]

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)

Comment 1

[Itiel]

Attachment: STR #1

Comment 2

[Itiel]

Attachment: STR #2

Comment 3

[chandranvishwaak]

i would like to take this up as my first bug. Can i start? and i would like some mentoring

Comment 4

[Dão Gottwald [:dao]]

(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;
  }

Comment 5

[chandranvishwaak]

i added  the rule and successfully built it on my local system(nightly) with desirable changes. Shall i send my patch ?

Comment 6

[Dão Gottwald [:dao]]

Sure!

Comment 7

[chandranvishwaak]

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

Comment 8

[Dão Gottwald [:dao]]

Have you read https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch ?

Comment 9

[chandranvishwaak]

Attachment: patch_v1.txt

Comment 10

[chandranvishwaak]

Shall i push the patch ?

Comment 11

[Dão Gottwald [:dao]]

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.

Comment 12

[chandranvishwaak]

Attachment: Improved patch

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 9026049 [details] [diff] [review]
Improved patch

Looks good.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
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

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/335eb23dc594

Comment 16

[Itiel]

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.

Comment 17

[Itiel]

Attachment: After the fix

Comment 18

[Itiel]

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

Comment 19

[Dão Gottwald [:dao]]

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

Comment 20

[Itiel]

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

Comment 21

[Dão Gottwald [:dao]]

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

Comment 22

[Itiel]

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