Summary: | Overlay scroller hard to see on pages with dark background | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jon Lee <jonlee> | ||||||||||||
Component: | Layout and Rendering | Assignee: | Jon Lee <jonlee> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | bdakin, commit-queue, jonlee, simon.fraser, webkit.review.bot | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | PC | ||||||||||||||
OS: | OS X 10.5 | ||||||||||||||
Bug Depends on: | 59290 | ||||||||||||||
Bug Blocks: | |||||||||||||||
Attachments: |
|
Description
Jon Lee
2011-04-22 02:09:17 PDT
Created attachment 90683 [details]
Patch
Comment on attachment 90683 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=90683&action=review > Source/WebCore/platform/mac/ScrollbarThemeMac.mm:214 > + if (!scrollbar->parent()->isFrameView()) > + return ScrollbarOverlayStyleDefault; > + > + FrameView* frameView = static_cast<FrameView*>(scrollbar->parent()); > + Document* document = frameView->frame()->document(); > + Element* rootElementToUse = document->body(); > + if (!rootElementToUse) > + rootElementToUse = document->documentElement(); > + if (!rootElementToUse) > + return ScrollbarOverlayStyleDefault; > + > + RenderObject* renderer = rootElementToUse->renderer(); > + if (!renderer) > + return ScrollbarOverlayStyleDefault; > + Color color = renderer->style()->visitedDependentColor(CSSPropertyBackgroundColor); > + if (!color.isValid()) > + return ScrollbarOverlayStyleDefault; > + > + // reduce the background color from RGB to a lightness value > + // and determine which scrollbar style to use based on a magic > + // lightness heuristic > + double h, s, l; > + color.getHSL(h, s, l); > + if (l < .5) > + return ScrollbarOverlayStyleLight; This is a layering violation, since classes in WebCore/platform are not supposed to know about rendering, the DOM, or the frame tree. Instead, I think you should add a virtual method to ScrollableArea which FrameView can implement to return the background color. This will also make this possible to implement for overflow areas in the future. Created attachment 90756 [details]
Patch
Attachment 90756 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/page/FrameView.cpp:332: l is incorrectly named. Don't use the single letter 'l' as an identifier name. [readability/naming] [4]
Total errors found: 1 in 14 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 90758 [details]
Patch
Comment on attachment 90758 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=90758&action=review > Source/WebCore/platform/ScrollableArea.cpp:201 > +ScrollbarOverlayStyle ScrollableArea::recommendedScrollbarOverlayStyle() Since this is a one-line function, it would be nice to have it implemented in the header. > Source/WebCore/platform/mac/ScrollbarThemeMac.mm:39 > + Unnecessary whitespace. You should remove this. > Source/WebCore/platform/mac/ScrollbarThemeMac.mm:-449 > - Arguably this whitespace was valuable. But I'll leave that up to you. :-) > Source/WebCore/platform/mac/WebCoreSystemInterface.h:230 > + Unnecessary tab-whitespace. Should remove. Comment on attachment 90758 [details]
Patch
Oops, meant to r+ and cq- so that Jon can upload a new version with improved whitespace.
Created attachment 90784 [details]
Patch
The commit-queue encountered the following flaky tests while processing attachment 90784 [details]: http/tests/xmlhttprequest/re-login.html bug 51987 (author: ap@webkit.org) The commit-queue is continuing to process your patch. Comment on attachment 90784 [details] Patch Clearing flags on attachment: 90784 Committed r84740: <http://trac.webkit.org/changeset/84740> All reviewed patches have been landed. Closing bug. The commit-queue encountered the following flaky tests while processing attachment 90784 [details]: http/tests/xmlhttprequest/basic-auth.html bug 51613 (author: ap@webkit.org) http/tests/appcache/update-cache.html bug 52483 (author: ap@webkit.org) The commit-queue is continuing to process your patch. I rolled this out via 59290, since it breaks internal builds and does not take into account feedback that I gave via email. Created attachment 90897 [details]
Patch
Comment on attachment 90897 [details]
Patch
r=me
(previously reviewed by Simon and revised per his suggestions)
Comment on attachment 90897 [details] Patch Clearing flags on attachment: 90897 Committed r84769: <http://trac.webkit.org/changeset/84769> All reviewed patches have been landed. Closing bug. |