Bug 59183 – Overlay scroller hard to see on pages with dark background

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Jon Lee]

<rdar://problem/8975367>

[Jon Lee]

Created attachment 90683 [details]
Patch

[Sam Weinig]

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.

[Jon Lee]

Created attachment 90756 [details]
Patch

[WebKit Review Bot]

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.

[Jon Lee]

Created attachment 90758 [details]
Patch

[Beth Dakin]

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.

[Beth Dakin]

Comment on attachment 90758 [details]
Patch

Oops, meant to r+ and cq- so that Jon can upload a new version with improved whitespace.

[Jon Lee]

Created attachment 90784 [details]
Patch

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 90784 [details]:

http/tests/xmlhttprequest/re-login.html bug 51987 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 90784 [details]
Patch

Clearing flags on attachment: 90784

Committed r84740: <http://trac.webkit.org/changeset/84740>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 90784 [details]:

http/tests/xmlhttprequest/basic-auth.html bug 51613 (author: [e-mail])
http/tests/appcache/update-cache.html bug 52483 (author: [e-mail])
The commit-queue is continuing to process your patch.

[Simon Fraser (smfr)]

I rolled this out via 59290, since it breaks internal builds and does not take into account feedback that I gave via email.

[Jon Lee]

Created attachment 90897 [details]
Patch

[Maciej Stachowiak]

Comment on attachment 90897 [details]
Patch

r=me

(previously reviewed by Simon and revised per his suggestions)

[WebKit Commit Bot]

Comment on attachment 90897 [details]
Patch

Clearing flags on attachment: 90897

Committed r84769: <http://trac.webkit.org/changeset/84769>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.