Bug 236061 – The background of the main frame's transparent custom scrollbar should be filled with the view background color if !HAVE(RUBBER_BANDING)

• Component: Compositing
• Status: RESOLVED FIXED

[Fujii Hironori]

Created attachment 450738 [details]
test case

[WinCairo] transparent scrollbar layer doesn't repaint correctly

WinCairo can't render YouTube's custom scrollbar correctly.

[Fujii Hironori]

Created attachment 460919 [details]
test case

[Fujii Hironori]

Mac WK2 enables HAVE_RUBBER_BANDING and ENABLE_RUBBER_BANDING,
There is m_layerForOverhangAreas layer.

Mac WK1 and iOS don't seem to support custom scrollbar.

[Fujii Hironori]

In GTK port, the background of the custom scrollbar is always black, because it clears color buffers with black.
It doesn't have the repainting issue because it clears the background for every frames.
If I disable it, it also has the same issue with WinCairo.

diff --git a/Source/WebKit/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp b/Source/WebKit/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp
index 57537cf53881..2f133b10e61f 100644
--- a/Source/WebKit/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp
+++ b/Source/WebKit/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp
@@ -234,7 +234,7 @@ void ThreadedCompositor::renderLayerTree()
         glViewport(0, 0, viewportSize.width(), viewportSize.height());
 
     glClearColor(0, 0, 0, 0);
-    glClear(GL_COLOR_BUFFER_BIT);
+    //glClear(GL_COLOR_BUFFER_BIT);
 
     m_scene->applyStateChanges(states);
     m_scene->paintToCurrentGLContext(viewportTransform, FloatRect { FloatPoint { }, viewportSize }, m_paintFlags);

[Fujii Hironori]

Created attachment 461033 [details]
test case

[Fujii Hironori]

Only scrollbar corner is filled with the background color.
https://github.com/WebKit/WebKit/blob/4c3c8c892f788fe62ab12e103a5b3d0f1132b108/Source/WebCore/page/FrameView.cpp#L4268

[Fujii Hironori]

Created attachment 461039 [details]
Patch

[Fujii Hironori]

Created attachment 461071 [details]
Patch

[Fujii Hironori]

Created attachment 461072 [details]
Patch

[Fujii Hironori]

Created attachment 461108 [details]
Patch

[Darin Adler]

Comment on attachment 461108 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=461108&action=review

> Source/WebCore/rendering/RenderLayerCompositor.cpp:3655
> +    if (backgroundColor.isValid())

In cases like this, I think it’s good to use isVisible() instead of isValid(). That’s guaranteed to return false for invalid color, but also returns false for fully transparent colors which don’t need painting.

On the other hand, maybe fillRect() already does an isVisible() check (I didn’t look). In that case, we don’t need the isValid() check either.

[Fujii Hironori]

Comment on attachment 461108 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=461108&action=review

>> Source/WebCore/rendering/RenderLayerCompositor.cpp:3655
>> +    if (backgroundColor.isValid())
> 
> In cases like this, I think it’s good to use isVisible() instead of isValid(). That’s guaranteed to return false for invalid color, but also returns false for fully transparent colors which don’t need painting.
> 
> On the other hand, maybe fillRect() already does an isVisible() check (I didn’t look). In that case, we don’t need the isValid() check either.

Color::isVisible doesn't explicitly check Valid flag, but assuming the invalid color has black transparent. So, isVisible returns false for the invalid color.

GraphicsContextCG::fillRect doesn't check the invalid color, it fills with black transparent in case of the invalid color.

[Darin Adler]

Comment on attachment 461108 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=461108&action=review

>>> Source/WebCore/rendering/RenderLayerCompositor.cpp:3655
>>> +    if (backgroundColor.isValid())
>> 
>> In cases like this, I think it’s good to use isVisible() instead of isValid(). That’s guaranteed to return false for invalid color, but also returns false for fully transparent colors which don’t need painting.
>> 
>> On the other hand, maybe fillRect() already does an isVisible() check (I didn’t look). In that case, we don’t need the isValid() check either.
> 
> Color::isVisible doesn't explicitly check Valid flag, but assuming the invalid color has black transparent. So, isVisible returns false for the invalid color.
> 
> GraphicsContextCG::fillRect doesn't check the invalid color, it fills with black transparent in case of the invalid color.

Yes, invalid is guaranteed to also be transparent black. I suggest changing this to isVisible.

[Fujii Hironori]

Created attachment 461202 [details]
Patch for landing

[EWS]

Committed 252803@main (386d6b252a24): <https://commits.webkit.org/252803@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 461202 [details].

[Radar WebKit Bug Importer]

<rdar://problem/97572054>

[Robert Jenner]

Re-opening for pull request https://github.com/WebKit/WebKit/pull/2352

[WebKit Commit Bot]

Re-opened since this is blocked by bug 243306

[Fujii Hironori]

Created attachment 461289 [details]
Patch for landing

[Fujii Hironori]

Created attachment 461290 [details]
Patch for landing

[Darin Adler]

Comment on attachment 461290 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=461290&action=review

> Source/WebCore/rendering/RenderLayerCompositor.cpp:3660
> +#if HAVE(RUBBER_BANDING)
> +    UNUSED_PARAM(backgroundColor);
> +#else

You can’t use an #if for WebKit1 vs. WebKit2, because both share the same underlying WebCore.

[Fujii Hironori]

This patch has no effect for Cocoa ports.
EWS reported a test failure for the new test. This is another problem. I fixed it in bug#243422.
I'm going to submit the patch for EWS again.

[Fujii Hironori]

Created attachment 461411 [details]
Patch

[EWS]

Committed 253142@main (a905abaab2bd): <https://commits.webkit.org/253142@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 461411 [details].

[Alice Mikhaylenko]

This regressed GTK overlay scrollbars, they now have a random background.

[Alice Mikhaylenko]

Created attachment 461465 [details]
[Screenshot] GTK regression

[Fujii Hironori]

Thank you for reporting. Will revert the change.

[Fujii Hironori]

Created attachment 461469 [details]
Patch

[Fujii Hironori]

Created attachment 461474 [details]
Patch

[Alice Mikhaylenko]

FWIW I would like to have this at some point for GTK port for non-overlay scrollbars only.

And looking at the patch, this looks like it should already happen with the `scrollbar->isOverlayScrollbar()` check, but doesn't for some reason. No idea why, at least Adwaita scrollbars do properly set it.

[Fujii Hironori]

What do you mean? Do you still have the same problem with the new patch? I tested the patch with GTK MiniBrowser.

[Alice Mikhaylenko]

Oh wait. Sorry, I didn't realize there was a new patch, only saw a revert. 🤦️

[Alice Mikhaylenko]

I can confirm it doesn't break overlay scrollbars. The non-overlay scrollbar variant obscures this background with its own background. If removed it works perfectly, I'll submit that patch in a separate bug once this one is in.

[Fujii Hironori]

Comment on attachment 461474 [details]
Patch

Thank you very much for testing.

[Alice Mikhaylenko]

Created attachment 461493 [details]
Corner issue screenshot

Unfortunately I have missed one thing though: it's missing the scrollbar corner, it needs the same color as well.

(Sorry for delays, webkit is just taking forever to build on my laptop)

[Alice Mikhaylenko]

Created attachment 461494 [details]
Test case patch

Here's the patch I used to remove the GTK scrollbar bg to use these colors.

[Fujii Hironori]

I think ScrollbarThemeAdwaita should paint the scrollbar background with the theme color.
Why do you want to change the behavior? Does any browsers do so?

[Alice Mikhaylenko]

That scrollbar only has its own background because there was no way to get the page one. It's supposed to blend with the page the same way overlay scrollbars do.

[Fujii Hironori]

Comment on attachment 461474 [details]
Patch

It seems to be a separate issue which should be filed separately.

[EWS]

Committed 253258@main (dd1137d3994e): <https://commits.webkit.org/253258@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 461474 [details].