[Windows 7] Disabled overflow scroll buttons are too dark

VERIFIED FIXED in Firefox 57

Status

()

P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: johannh, Assigned: johannh)

Tracking

57 Branch
Firefox 58
All
Windows 7
Points:
---

Firefox Tracking Flags

(firefox57+ verified, firefox58 verified)

Details

(Whiteboard: [reserve-photon-visual])

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
+++ This bug was initially created as a clone of Bug #1391227 +++

(Quouting Mark Straver from bug 1391227 comment #13)
> Thanks, it's better by using the light fill for controls when using my
> default color tint for glass, but there's still an issue, that is now shared
> with all themes since the buttons are the same now for all: When the tab
> scroll arrow is disabled with tab overflow, it becomes invisible or only
> barely visible, depending on the background the window is over.[1]
> [1] https://www.dropbox.com/s/izwi7p1hn11an7n/issue2.png?dl=0

[Tracking Requested - why for this release]:
Overflow doesn't look great on Windows 7.
Flags: qe-verify+
Comment hidden (mozreview-request)
QA Contact: ovidiu.boca
tracking-firefox57: ? → +

Comment 2

2 years ago
mozreview-review
Comment on attachment 8912180 [details]
Bug 1403110 - Fix disabled button color on Windows 7 aero.

https://reviewboard.mozilla.org/r/183558/#review189024

::: browser/themes/windows/compacttheme.css:43
(Diff revision 1)
> -    color: hsl(240,9%,98%);
> +      color: hsl(240,9%,98%);
> -  }
> +    }
>  
> -  /* Keep showing the correct color inside the tabs. */
> +    /* Keep showing the correct color inside the tabs. */
> -  .tabbrowser-tab {
> +    .tabbrowser-tab {
> -    color: var(--chrome-color) !important;
> +      color: var(--chrome-color) !important;

Can you explain why this rule is in the new @media query but the other ones below stay in the old one?
Attachment #8912180 - Flags: review?(nhnt11) → review+
(Assignee)

Comment 3

2 years ago
mozreview-review-reply
Comment on attachment 8912180 [details]
Bug 1403110 - Fix disabled button color on Windows 7 aero.

https://reviewboard.mozilla.org/r/183558/#review189024

> Can you explain why this rule is in the new @media query but the other ones below stay in the old one?

This was a mistake in bug 1399498, it should be consistent with the version in browser.css, to keep buttons white on both aero glass and basic in light theme as well. Now it's the same.

The menu bar highlight on the other hand is only required on aero glass, because the menu bar looks fine on aero basic.

Comment 4

2 years ago
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3725c908a238
Fix disabled button color on Windows 7 aero. r=nhnt11

Comment 5

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3725c908a238
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Hi Johann, should we uplift this fix to Beta57?
Flags: needinfo?(jhofmann)
(Assignee)

Comment 7

2 years ago
(In reply to Ritu Kothari (:ritu) from comment #6)
> Hi Johann, should we uplift this fix to Beta57?

Yes, absolutely!
Flags: needinfo?(jhofmann)
(Assignee)

Comment 8

2 years ago
Comment on attachment 8912180 [details]
Bug 1403110 - Fix disabled button color on Windows 7 aero.

Approval Request Comment
[Feature/Bug causing the regression]: Photon Redesign
[User impact if declined]: Disabled Overflow buttons in Windows 7 look bad
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Not yet VERIFIED, but it looks fine on my Nightly.
[Needs manual test from QE? If yes, steps to reproduce]: Not really, it's easy to see when having tab overflow and scrolling to either end of the tab list.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Very simple CSS fix.
[String changes made/needed]: None
Attachment #8912180 - Flags: approval-mozilla-beta?

Comment 9

2 years ago
mozreview-review
Comment on attachment 8912180 [details]
Bug 1403110 - Fix disabled button color on Windows 7 aero.

https://reviewboard.mozilla.org/r/183558/#review190048

::: browser/themes/windows/browser.css:120
(Diff revision 1)
>        --tabs-border: #4A4A4F;
>      }
>  
> -    #TabsToolbar:not(:-moz-lwtheme) {
> +    #TabsToolbar:not(:-moz-lwtheme),
> +    #TabsToolbar:not(:-moz-lwtheme) toolbarbutton[disabled="true"] {
>        color: hsl(240,9%,98%);

This doesn't look right. Toolbarbuttons should inherit color. If this currently doesn't work for disabled buttons, then this probably doesn't just affect the win7 default theme and should be fixed in toolbarbuttons.inc.css.
(Assignee)

Comment 10

2 years ago
(In reply to Dão Gottwald [::dao] from comment #9)
> Comment on attachment 8912180 [details]
> Bug 1403110 - Fix disabled button color on Windows 7 aero.
> 
> https://reviewboard.mozilla.org/r/183558/#review190048
> 
> ::: browser/themes/windows/browser.css:120
> (Diff revision 1)
> >        --tabs-border: #4A4A4F;
> >      }
> >  
> > -    #TabsToolbar:not(:-moz-lwtheme) {
> > +    #TabsToolbar:not(:-moz-lwtheme),
> > +    #TabsToolbar:not(:-moz-lwtheme) toolbarbutton[disabled="true"] {
> >        color: hsl(240,9%,98%);
> 
> This doesn't look right. Toolbarbuttons should inherit color. If this
> currently doesn't work for disabled buttons, then this probably doesn't just
> affect the win7 default theme and should be fixed in toolbarbuttons.inc.css.

Aah good point, it's much harder to notice on Windows 10 but it's there. I'll file a follow-up to handle the general case.
(Assignee)

Comment 11

2 years ago
Comment on attachment 8912180 [details]
Bug 1403110 - Fix disabled button color on Windows 7 aero.

Let's uplift the new bug.
Attachment #8912180 - Flags: approval-mozilla-beta?
(Assignee)

Updated

2 years ago
Depends on: 1404246
(Assignee)

Comment 12

2 years ago
Comment on attachment 8912180 [details]
Bug 1403110 - Fix disabled button color on Windows 7 aero.

This needs uplift for bug 1404246 and bug 1391227 to apply cleanly (should not be risky).

See comment 8 for the uplift request.
Attachment #8912180 - Flags: approval-mozilla-beta?
Comment on attachment 8912180 [details]
Bug 1403110 - Fix disabled button color on Windows 7 aero.

Polish photon, taking it.
should be in 57b5
Attachment #8912180 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 14

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/96181880ab72
status-firefox57: affected → fixed
I verified this on Windows 7 x64 with FF Nightly 58.0a1(2017-10-03) and FF beta 57.0b5 and the arrow buttons on either side of the tab strip are white. During the verification I found out another issue, on the left side of the tab bar, a vertical line is displayed, I'll log a new bug for this.
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
status-firefox58: fixed → verified
Flags: qe-verify+
If the windows theme is "Windows 7 Basic" the browser theme is Light and the title bar is enabled the arrow buttons that appears when the tab strip is overflowed, are not visible. Johann I assume that I need to log another bug for this right? 
Please see the actual result: https://imgur.com/a/VGOGl
Flags: needinfo?(jhofmann)
(Assignee)

Comment 18

2 years ago
(In reply to ovidiu boca[:Ovidiu] from comment #16)
> If the windows theme is "Windows 7 Basic" the browser theme is Light and the
> title bar is enabled the arrow buttons that appears when the tab strip is
> overflowed, are not visible. Johann I assume that I need to log another bug
> for this right? 
> Please see the actual result: https://imgur.com/a/VGOGl

Dark/Light theme should have the same window background as default theme on Windows 7 Basic. Please file a new bug.
Flags: needinfo?(jhofmann)
You need to log in before you can comment on or make changes to this bug.