Closed Bug 865806 Opened 11 years ago Closed 11 years ago

[10.7] Make lion-style scrollbars brighter on dark background

Categories

(Core :: Widget: Cocoa, defect)

x86
macOS
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla24
Tracking Status
firefox21 --- unaffected
firefox22 --- unaffected
firefox23 + verified
firefox24 --- verified
firefox-esr17 --- unaffected

People

(Reporter: spohl, Assigned: ehsan.akhgari)

References

(Depends on 1 open bug, )

Details

(Whiteboard: [lion-scrollbars+])

Attachments

(1 file, 2 obsolete files)

Broken out of bug 636564. The lion-style scrollbars need to be brighter on dark background, for example on:
http://www.klack.de/fernsehprogramm/2015-im-tv/0/free.html
Swap the gutter and indicator colors?
WebKit people are defining it by the "plain" css background color on or inherited by the scrollable element: if the sum of the RGB values is under 384, it's dark.

(So yes, a solid black image or black-to-black CSS gradient wouldn't count, but most web developers specify a backup background color in case an image doesn't load immediately or gradients aren't supported.)

IMHO, this is worth implementing, if only as a first step toward a more fail-proof solution.
Attached patch Patch (v1) (obsolete) — Splinter Review
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #751265 - Flags: review?(roc)
Comment on attachment 751265 [details] [diff] [review]
Patch (v1)

Hmm actually this heuristic is wrong, as aFrame is the button box frame for the scrollbar thumb!
Attachment #751265 - Flags: review?(roc)
Attached patch Patch (v1) (obsolete) — Splinter Review
This seems to work fine.
Attachment #751265 - Attachment is obsolete: true
Attachment #751354 - Flags: review?(roc)
Backed out because of Windows build bustage: https://hg.mozilla.org/integration/mozilla-inbound/rev/70898c96cceb
Blocks: 636564
No longer depends on: 636564
https://hg.mozilla.org/mozilla-central/rev/8aef76919ec0
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Comment on attachment 751354 [details] [diff] [review]
Patch (v1)

Review of attachment 751354 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/xpwidgets/nsNativeTheme.cpp
@@ +691,5 @@
> +  nsStyleContext* bgSC;
> +  if (nsCSSRendering::FindBackground(frame, &bgSC)) {
> +    nscolor bgColor = bgSC->StyleBackground()->mBackgroundColor;
> +    // Consider the background color dark if the sum of the r, g and b values is
> +    // less than 384 in a semi-transparent docement.  This heuristic matches what

Good ol' docements. :)
Whiteboard: [lion-scrollbars+]
Blocks: 874391
Rob, do you want me to nominate this for Aurora?
Flags: needinfo?(robert.bugzilla)
(In reply to Jared Wein [:jaws] from comment #13)
> Comment on attachment 751354 [details] [diff] [review]
> Patch (v1)
> 
> Review of attachment 751354 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/xpwidgets/nsNativeTheme.cpp
> @@ +691,5 @@
> > +  nsStyleContext* bgSC;
> > +  if (nsCSSRendering::FindBackground(frame, &bgSC)) {
> > +    nscolor bgColor = bgSC->StyleBackground()->mBackgroundColor;
> > +    // Consider the background color dark if the sum of the r, g and b values is
> > +    // less than 384 in a semi-transparent docement.  This heuristic matches what
> 
> Good ol' docements. :)

Hehe, nice catch!  Fixed in a follow-up: https://hg.mozilla.org/mozilla-central/rev/f9dc5e76d66b
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #14)
> Rob, do you want me to nominate this for Aurora?
Yes please and thank you!
Flags: needinfo?(robert.bugzilla)
Comment on attachment 751354 [details] [diff] [review]
Patch (v1)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Lion scrollbars.
User impact if declined: See the bug report? [1]
Testing completed (on m-c, etc.): m-c, note that bug 874391 was also landed which fixes a crash in this patch, and I'll roll that in this patch when landing on Aurora.
Risk to taking this patch (and alternatives if risky): Should not be very risky.
String or IDL/UUID changes made by this patch: None.

[1] <rant> the entries on this form are really really not appropriate for most bugs, as most of these questions have very obvious answers for most situations.  I'm sometimes tempted to just clear this questionnaire where it doesn't make sense. :/ </rant>
Attachment #751354 - Flags: approval-mozilla-aurora?
Comment on attachment 751354 [details] [diff] [review]
Patch (v1)

I hear your frustration but we (relman) get a lot of value from the form responses in triage because even though the questions/answers may seem obvious to you when you are familiar with a bug's history & origin, we are very aided by having a rollup of the info in a single comment.  Also some of the questions are intended to prompt/remind the nominator of things they might overlook like UUID/string changes. That being said, if you have proposals for ways to request the info that would feel less aggravating please do reach out and propose them.  We could certainly change the look/feel/length of the form to improve the reception.
Attachment #751354 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
No longer blocks: 874391
Depends on: 874391
(In reply to Ryan VanderMeulen [:RyanVM][Out May 23-27] from comment #19)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/59f9f7a8bd8e

I backed this out of aurora because it landed without the crashfix from bug 874391. I would have preferred to just land that fix, but it didn't have a+ yet, and when I noticed this, no approvers were around and I was told on IRC this cset ought to be backed out as approval should have been requested on a landable patch instead of this one. (Please don't shoot the messenger, I'm just trying to make sure we don't get really crashy aurora builds.)
Carrying over earlier r+'s from roc, pretty much the same approval request:

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Lion scrollbars.
User impact if declined: scrollbars are neigh-invisible on dark backgrounds
Testing completed (on m-c, etc.): m-c, note that bug 874391 was also landed which fixes a crash in the original patch. This patch contains both the original and the crashfix.
Risk to taking this patch (and alternatives if risky): Should not be very risky.
String or IDL/UUID changes made by this patch: None.
Attachment #751354 - Attachment is obsolete: true
Attachment #753126 - Flags: review+
Attachment #753126 - Flags: approval-mozilla-aurora?
(In reply to lsblakk@mozilla.com [:lsblakk] from comment #18)
> Comment on attachment 751354 [details] [diff] [review]
> Patch (v1)
> 
> I hear your frustration but we (relman) get a lot of value from the form
> responses in triage because even though the questions/answers may seem
> obvious to you when you are familiar with a bug's history & origin, we are
> very aided by having a rollup of the info in a single comment.  Also some of
> the questions are intended to prompt/remind the nominator of things they
> might overlook like UUID/string changes. That being said, if you have
> proposals for ways to request the info that would feel less aggravating
> please do reach out and propose them.  We could certainly change the
> look/feel/length of the form to improve the reception.

Yeah, I totally understand how this is helpful to you guys, and I did not mean to be dismissive of that fact.  However, I think it should be OK to drop the questions that are irrelevant.  For example, just delete the UUID/string question instead of just saying no.  Presumably that delivers the same amount of information to you guys.  Or, just saying "trivial patch to fix a crasher" instead of filling in all of the existing questions to the same effect.  Would that make sense?
Comment on attachment 753126 [details] [diff] [review]
Patch including the crashfix

Thanks for creating the rolled up patch, Gijs!  But I don't think this beauacracy is really needed as I mentioned that I want to land the roll-up patch in comment 17 -- it just happened that RyanVM beat me to landing it.  :-)

So I'll just go ahead and assume a+.
Attachment #753126 - Flags: approval-mozilla-aurora?
Depends on: 876129
I mistakenly landed this patch <https://hg.mozilla.org/integration/mozilla-inbound/rev/d921692be88f> which was really for bug 876129 with this bug #.  Sorry!
Blocks: 884593
User Agent :Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:23.0) Gecko/20100101 Firefox/23.0
Build ID: 20130711122148
Tested on FX 23-B5 using http://www.klack.de/fernsehprogramm/2015-im-tv/0/free.html

For dark Background, I see light scroll-bar.
User Agent :Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:25.0) Gecko/20130712 Firefox/25.0
Build ID: 20130712030203

User Agent :Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:24.0) Gecko/20130712 Firefox/24.0
Build ID: 20130712004003

Tested on latest nightly and Aurora using http://www.klack.de/fernsehprogramm/2015-im-tv/0/free.html
For dark Background, I see light scroll-bar.
Status: RESOLVED → VERIFIED
User Agent : Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:23.0) Gecko/20100101 Firefox/23.0
Build ID: 20130715155216

User Agent : Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:25.0) Gecko/20130718 Firefox/25.0
Build ID: 20130718030201

User Agent : Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:24.0) Gecko/20130718 Firefox/24.0
Build ID:  20130718004004

Tested on latest nightly and Aurora using http://www.klack.de/fernsehprogramm/2015-im-tv/0/free.html
For dark Background, I see light scroll-bar.
Depends on: 903839
You need to log in before you can comment on or make changes to this bug.