Fix overlay scroll bar color on elements with dark background

Fix overlay scroll bar color on elements with dark background

When overlay scroll bars are used on elements with CSS `overflow` set to use
scroll bars, and the background of that element is dark, the color of the
scroll bar is now set to the "light" style. This was previously only done for
the document (body), not for elements with a scrollable area. Whenever the
background changes, the scroll bar style will be updated accordingly.

Even after this change, the scroll bar is rendered with the wrong color if the
scrollable element doesn't have a background of its own (but a parent element
uses a dark background). Fixing the issue for scrollable elements with
background set explicitly is an improvement to the current state though.

BUG=588709
R=wangxianzhu@chromium.org

Committed: https://crrev.com/f62a6f444cc44a24e110c647d0cd7ad3442a4c8a
Cr-Commit-Position: refs/heads/master@{#378843}

[koggdal, 2016-02-24 23:28:31]

Description was changed from

==========
Fix overlay scroll bar color on elements with dark background

When overlay scroll bars are used on elements with CSS `overflow` set to use
scroll bars, and the background of that element is dark, the color of the
scroll bar is now set to the "light" style. This was previously only done for
the document (body), not for elements with a scrollable area. Whenever the
background changes, the scroll bar style will be updated accordingly.

Even after this change, the scroll bar is rendered with the wrong color if the
scrollable element doesn't have a background of its own (but a parent element
uses a dark background). Fixing the issue for scrollable elements with
background set explicitly is an improvement to the current state though.

BUG=588709

R=wangxianzhu@chromium.org
==========

to

==========
Fix overlay scroll bar color on elements with dark background

When overlay scroll bars are used on elements with CSS `overflow` set to use
scroll bars, and the background of that element is dark, the color of the
scroll bar is now set to the "light" style. This was previously only done for
the document (body), not for elements with a scrollable area. Whenever the
background changes, the scroll bar style will be updated accordingly.

Even after this change, the scroll bar is rendered with the wrong color if the
scrollable element doesn't have a background of its own (but a parent element
uses a dark background). Fixing the issue for scrollable elements with
background set explicitly is an improvement to the current state though.

BUG=588709

R=wangxianzhu@chromium.org
==========

[koggdal, 2016-02-24 23:28:31]

koggdal@spotify.com changed reviewers:
+ mkwst@chromium.org

[koggdal, 2016-02-24 23:31:05]

On 2016/02/24 23:25:19, koggdal wrote:

This is my first change to Chromium, so any suggestions would be much
appreciated! I've tried to look around for tests, but couldn't really figure out
myself how this would be tested. Should I put a test somewhere?

[koggdal, 2016-02-24 23:33:42]

This is my first change to Chromium, so any suggestions would be much
appreciated! I've tried to look around for tests, but couldn't really figure out
myself how this would be tested. Should I put a test somewhere?

[Xianzhu, 2016-02-24 23:48:27]

https://codereview.chromium.org/1738503003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right):

https://codereview.chromium.org/1738503003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:866: }
Can you extract common code in here and in FrameView into ScrollArea (e.g.
recalculateScrollbarOverlayStyle(Color backgroundColor))? Then you can just unit
test the above function in ScrollableAreaTest.

[Mike West, 2016-02-25 11:17:02]

mkwst@chromium.org changed reviewers:
+ wangxianzhu@chromium.org*
- wangxianzhu@chromium.org

[Mike West, 2016-02-25 11:17:02]

Source/platform LGTM. Please wait for wangxianzhu@ to review the changes to
/paint before landing this.

Thanks for this contribution! Looking forward to more! :)

https://codereview.chromium.org/1738503003/diff/1/AUTHORS
File AUTHORS (right):

https://codereview.chromium.org/1738503003/diff/1/AUTHORS#newcode294
AUTHORS:294: Johannes Koggdal <koggdal@spotify.com>
It looks like Spotify has signed Google's CLA. Rather than adding you
specifically, please add `Spotify AB <*@spotify.com>` into the corporate CLA
signers block at the bottom of this file.

Hooray for process. *sigh*

[Mike West, 2016-02-25 11:50:17]

mkwst@chromium.org changed reviewers:
+ wangxianzhu@chromium.org
- wangxianzhu@chromium.org*

[Mike West, 2016-02-25 11:50:17]

mkwst@chromium.org changed required reviewers:
+ wangxianzhu@chromium.org

[koggdal, 2016-02-26 01:50:00]

On 2016/02/25 11:50:18, Mike West wrote:

Thank you for the comments!

There's now a new patch set with some code extracted into ScrollableArea, a unit
test added and the fix for the AUTHORS entry.

[Xianzhu, 2016-02-26 17:56:52]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-02-26 17:56:53]

lgtm

[Xianzhu, 2016-02-26 17:56:53]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1738503003/#ps40001
(title: "Extract code + unit test + fix AUTHORS")

[commit-bot: I haz the power, 2016-02-26 17:57:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1738503003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1738503003/40001

[commit-bot: I haz the power, 2016-02-26 18:28:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-26 18:28:56]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)

[koggdal, 2016-03-01 08:37:48]

Are the tests flaky? The Android failure doesn't look related as far as I can
see. The Mac failure mentions failing tests around scroll bar, but I can't
reproduce them locally (although I get quite a lot of other failures with and
without my patch).

[Xianzhu, 2016-03-01 18:41:32]

On 2016/03/01 08:37:48, koggdal wrote:
> Are the tests flaky? The Android failure doesn't look related as far as I can
> see. The Mac failure mentions failing tests around scroll bar, but I can't
> reproduce them locally (although I get quite a lot of other failures with and
> without my patch).

I believe the error on Android is flaky and you can just ignore it.

The failures on Mac are assertion failures, so you must run a build with
assertions enabled to reproduce them. I suggest to run a debug build.

fwiw, you can run the failed test locally using the following steps:
 1. Click "layout_test_results" link in the "archive_layout_test_results" step
in the try bot result
(https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel...);
 2. Click "flag all"
 3. At the bottom of the page, after "Flagged tests", uncheck "Use newlines" and
copy all contents below "Flagged Tests".
 4. run-webkit-tests --debug <paste>

I think your change makes didAddScrollbar() also called for custom scrollbars,
causing the assertion failures. Is the change about didAddScrollbar() needed?

[koggdal, 2016-03-01 23:34:50]

Thanks a lot for explaining, and the hint for the issue! :)

Managed to run only these tests, reproduced the failures and fixed the issue. I
seem to need to call didAddScrollbar after creating it for it to take effect
(otherwise the scroll bar color will be wrong for the first page load, but
adjust when the background color of the element changes). So I extracted the
check for whether to use a custom scroll bar style into a function to easily
check it from multiple places.

[Xianzhu, 2016-03-02 00:03:51]

https://codereview.chromium.org/1738503003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right):

https://codereview.chromium.org/1738503003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1507:
m_scrollableArea->didAddScrollbar(*m_hBar, HorizontalScrollbar);
I suggest to check the condition in ScrollableArea::didAddScrollbar() because
the function doesn't explicitly disallow custom scrollbars based on its name.

In ScrollbarArea::didAddScrollbar() you can add:

  if (scrollbar.isCustomScrollbar())
     return;

https://codereview.chromium.org/1738503003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1539:
PassRefPtrWillBeRawPtr<Scrollbar>
PaintLayerScrollableArea::ScrollbarManager::createScrollbar(ScrollbarOrientation
orientation, bool useCustomScrollbarStyle)
I suggest to avoid the new parameter and call useCustomScrollbarStyle() here.
This avoids a bool parameter which is not suggested in our coding style.

[koggdal, 2016-03-02 17:22:58]

I tried checking it inside didAddScrollbar instead, but it seems like it needs
to run even for custom scroll bar for FrameView. The test
"scrollbars/scrollbar-initial-position.html" fails when I have the check in
didAddScrollbar. So what I did now was to just use the isCustomScrollbar
function instead of implementing my own function for checking it, so the check
still happens in PaintLayerScrollableArea before calling didAddScrollbar.

[Xianzhu, 2016-03-02 19:27:37]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-03-02 19:27:38]

lgtm

[Xianzhu, 2016-03-02 19:27:38]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1738503003/#ps80001
(title: "Fix custom scroll bar check")

[commit-bot: I haz the power, 2016-03-02 19:28:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1738503003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1738503003/80001

[commit-bot: I haz the power, 2016-03-02 19:33:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-02 19:33:39]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[koggdal, 2016-03-02 20:03:49]

There was a merge conflict from this: https://codereview.chromium.org/1746283002

Rebased and fixed the problem.

[Xianzhu, 2016-03-02 20:31:48]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-03-02 20:31:48]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org,
wangxianzhu@chromium.org
Link to the patchset: https://codereview.chromium.org/1738503003/#ps120001
(title: "Fix test after rebase")

[commit-bot: I haz the power, 2016-03-02 20:32:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1738503003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1738503003/120001

[commit-bot: I haz the power, 2016-03-02 21:48:43]

Description was changed from

==========
Fix overlay scroll bar color on elements with dark background

When overlay scroll bars are used on elements with CSS `overflow` set to use
scroll bars, and the background of that element is dark, the color of the
scroll bar is now set to the "light" style. This was previously only done for
the document (body), not for elements with a scrollable area. Whenever the
background changes, the scroll bar style will be updated accordingly.

Even after this change, the scroll bar is rendered with the wrong color if the
scrollable element doesn't have a background of its own (but a parent element
uses a dark background). Fixing the issue for scrollable elements with
background set explicitly is an improvement to the current state though.

BUG=588709

R=wangxianzhu@chromium.org
==========

to

==========
Fix overlay scroll bar color on elements with dark background

When overlay scroll bars are used on elements with CSS `overflow` set to use
scroll bars, and the background of that element is dark, the color of the
scroll bar is now set to the "light" style. This was previously only done for
the document (body), not for elements with a scrollable area. Whenever the
background changes, the scroll bar style will be updated accordingly.

Even after this change, the scroll bar is rendered with the wrong color if the
scrollable element doesn't have a background of its own (but a parent element
uses a dark background). Fixing the issue for scrollable elements with
background set explicitly is an improvement to the current state though.

BUG=588709

R=wangxianzhu@chromium.org
==========

[commit-bot: I haz the power, 2016-03-02 21:48:44]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-03-02 21:51:08]

Description was changed from

==========
Fix overlay scroll bar color on elements with dark background

When overlay scroll bars are used on elements with CSS `overflow` set to use
scroll bars, and the background of that element is dark, the color of the
scroll bar is now set to the "light" style. This was previously only done for
the document (body), not for elements with a scrollable area. Whenever the
background changes, the scroll bar style will be updated accordingly.

Even after this change, the scroll bar is rendered with the wrong color if the
scrollable element doesn't have a background of its own (but a parent element
uses a dark background). Fixing the issue for scrollable elements with
background set explicitly is an improvement to the current state though.

BUG=588709

R=wangxianzhu@chromium.org
==========

to

==========
Fix overlay scroll bar color on elements with dark background

When overlay scroll bars are used on elements with CSS `overflow` set to use
scroll bars, and the background of that element is dark, the color of the
scroll bar is now set to the "light" style. This was previously only done for
the document (body), not for elements with a scrollable area. Whenever the
background changes, the scroll bar style will be updated accordingly.

Even after this change, the scroll bar is rendered with the wrong color if the
scrollable element doesn't have a background of its own (but a parent element
uses a dark background). Fixing the issue for scrollable elements with
background set explicitly is an improvement to the current state though.

BUG=588709

R=wangxianzhu@chromium.org

Committed: https://crrev.com/f62a6f444cc44a24e110c647d0cd7ad3442a4c8a
Cr-Commit-Position: refs/heads/master@{#378843}
==========

[commit-bot: I haz the power, 2016-03-02 21:51:10]

Patchset 7 (id:??) landed as
https://crrev.com/f62a6f444cc44a24e110c647d0cd7ad3442a4c8a
Cr-Commit-Position: refs/heads/master@{#378843}