Closed Bug 1507574 Opened 2 years ago Closed 2 years ago

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

Categories

(Firefox :: Theme, defect, P2)

defect

Tracking

()

RESOLVED FIXED
Firefox 65
Tracking Status
firefox-esr60 --- unaffected
firefox63 --- unaffected
firefox64 --- unaffected
firefox65 --- fixed

People

(Reporter: itiel_yn8, Assigned: chandranvishwaak, Mentored)

References

Details

(Keywords: good-first-bug, regression)

Attachments

(4 files, 1 obsolete file)

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)
Attached image STR #1
Attached image STR #2
Has STR: --- → yes
Flags: needinfo?(dao+bmo)
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;
  }
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
i am not sure about the process of sending the patch . can i get some help on it ?
Attached patch patch_v1.txt (obsolete) — Splinter Review
Attachment #9025766 - Flags: review+
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.
Attachment #9025766 - Attachment is patch: true
Attachment #9025766 - Flags: review+
Attached patch Improved patchSplinter Review
Comment on attachment 9026049 [details] [diff] [review]
Improved patch

Looks good.
Attachment #9026049 - Flags: review+
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
https://hg.mozilla.org/mozilla-central/rev/335eb23dc594
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Attachment #9025766 - Attachment is obsolete: true
Attachment #9026049 - Attachment is patch: true
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)
Attached image After the fix
(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)
(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.
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)
(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.
Depends on: 1509321
You need to log in before you can comment on or make changes to this bug.