Closed Bug 876129 Opened 11 years ago Closed 11 years ago

[10.7] Tweak light/dark algorithm for Lion-style scrollbars

Categories

(Core :: Widget: Cocoa, defect)

Product:
Core
Component:
Widget: Cocoa
Version:
23 Branch
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla24
Tracking Flags:
Tracking Status
firefox21 --- unaffected
firefox22 --- unaffected
firefox23 --- verified
firefox24 --- verified
firefox-esr17 --- unaffected

People

(Reporter: djc, Assigned: ehsan.akhgari)

References

(
URL
)

Details

(Whiteboard: [parity-webkit][lion-scrollbars=])

Attachments

(2 files, 2 obsolete files)

Part 1: Continue walking up the frame tree when hitting a transparent frame as we're trying to determine whether a given element's background is dark or not
1.73 KB, patch
roc
: review+
lsblakk
: approval-mozilla-aurora+
ehsan.akhgari
: checkin+
Details | Diff | Splinter Review
Part 2: Prefer to look at <body> instead of <html> when determining whether the background color of a page is dark or not
2.92 KB, patch
roc
: review+
Details | Diff | Splinter Review 
While the new light style for the Lion-style scrollbar (from bug 865806) is a great improvement, it seems a little eager. For example, we now have the light version on Kickstarter (see URL), which makes it kind of hard to see there. Chrome does better on this, at least.
The current version also sucks in pdf.js, where it's using the dark scrollbar against a fairly dark grey background.
Whiteboard: [parity-webkit] → [parity-webkit][lion-scrollbars=]
(In reply to Dirkjan Ochtman (:djc) from comment #1)
> The current version also sucks in pdf.js, where it's using the dark
> scrollbar against a fairly dark grey background.

I think this at least is caused by us looking at the background for the scrollable frame, which in the case of pdf.js is a descendent of <body>.  A possible fix would be to continue walking up the frame tree if we hit a transparent backrgound.  Stephen, wanna give that a shot, or should I?
Flags: needinfo?(spohl.mozilla.bugs)
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #2)
> Stephen, wanna give that a shot, or should I?

Bug 868498 is keeping me quite busy and it looks like you have a good handle on this already. If you have the bandwidth, please go ahead and take it. Thanks! :-)
Flags: needinfo?(spohl.mozilla.bugs)
I gave this a shot last night and forgot to attach the patches!
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #755676 - Flags: review?(roc)
Comment on attachment 755676 [details] [diff] [review]
Part 1: Continue walking up the frame tree when hitting a transparent frame as we're trying to determine whether a given element's background is dark or not

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

I think you should call nsCSSRendering::FindNonTransparentBackgroundFrame.
Attachment #755676 - Flags: review?(roc) → review+
Comment on attachment 755676 [details] [diff] [review]
Part 1: Continue walking up the frame tree when hitting a transparent frame as we're trying to determine whether a given element's background is dark or not

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

Oops sorry
Attachment #755676 - Flags: review+ → review-
You probably should start by calling nsCSSRendering::FindBackground on the scrolled frame. If that returns true, use the background it returns; this covers the case where the BODY background has been propagated to the viewport. Otherwise call nsCSSRendering::FindNonTransparentBackgroundFrame.
OK, this fixes the pdf.js case, but still we need a fix like part 2 for the kickstarter case...
Attachment #755676 - Attachment is obsolete: true
Attachment #755884 - Flags: review?(roc)
Why doesn't part 1 work for the kickstarter case?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> Why doesn't part 1 work for the kickstarter case?

Because both the html and body elements have background colors.
Whiteboard: [parity-webkit][lion-scrollbars=] → [parity-webkit][lion-scrollbars=][leave open]
Comment on attachment 755884 [details] [diff] [review]
Part 1: Continue walking up the frame tree when hitting a transparent frame as we're trying to determine whether a given element's background is dark or not

https://hg.mozilla.org/integration/mozilla-inbound/rev/d921692be88f

(Landed with the wrong bug #)
Attachment #755884 - Flags: checkin+
Comment on attachment 755677 [details] [diff] [review]
Part 2: Prefer to look at <body> instead of <html> when determining whether the background color of a page is dark or not

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

Also can you refresh this patch to be on top of part 1?

::: widget/xpwidgets/nsNativeTheme.cpp
@@ +688,5 @@
> +    return nullptr;
> +  }
> +  nsIDocument* document = content->OwnerDoc();
> +  if (!document) {
> +    return nullptr;

document can't be null.

@@ +705,5 @@
>      return false;
>    }
> +  nsStyleContext* bgSC = nullptr;
> +  if (nsCSSRendering::IsCanvasFrame(aFrame)) {
> +    // For canvas frames, prefer to look at the body first

Please extend this comment to explain that if a background color is set on both the root and <body> elements, we want to use the <body> color even though it didn't propagate to the viewport.

@@ +708,5 @@
> +  if (nsCSSRendering::IsCanvasFrame(aFrame)) {
> +    // For canvas frames, prefer to look at the body first
> +    nsIFrame* bodyFrame = GetBodyFrame(aFrame);
> +    if (bodyFrame) {
> +      bgSC = bodyFrame->StyleContext();

how about if bodyFrame is non-null we just set aFrame to it and fall through to the FindBackground call below. That should work.
Backed out, because apparently I need to #include "nsIDocumentInlines.h"... (bug 878113)

http://hg.mozilla.org/integration/mozilla-inbound/rev/2222b07ab207
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [parity-webkit][lion-scrollbars=][leave open] → [parity-webkit][lion-scrollbars=]
Target Milestone: --- → mozilla24
Automated patch review result for: attachment 756317 [details] [diff] [review]
Part 2: Prefer to look at <body> instead of <html> when determining whether the background color of a page is dark or not

Patch failed to apply on tip (Might be part of patch queue or against another branch/repo)
Ehsan, I'd like to see this uplifted to aurora if you are ok with it so please nominate when you are comfortable with doing so. Thanks!
status-firefox21: --- → unaffected
status-firefox22: --- → unaffected
status-firefox23: --- → affected
status-firefox24: --- → fixed
status-firefox-esr17: --- → unaffected
Comment on attachment 755884 [details] [diff] [review]
Part 1: Continue walking up the frame tree when hitting a transparent frame as we're trying to determine whether a given element's background is dark or not

This is not very risky, fixes issues with a new feature, and has been on m-c for a couple of days.  No strings/IDL changes.
Attachment #755884 - Flags: approval-mozilla-aurora?
Attachment #755884 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: verifyme
Verified as fixed on Firefox 23 beta 7 (I see bright scroll bars when loading the given URL or pdf files):
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:23.0) Gecko/20100101 Firefox/23.0
Build ID: 20130718163513
status-firefox23: fixed → verified
QA Contact: simona.marcu
Verified as fixed on Firefox 24 beta 2:
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:24.0) Gecko/20100101 Firefox/24.0
Build ID: 20130812173056
status-firefox24: fixed → verified
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: