Created attachment 450738 [details] test case [WinCairo] transparent scrollbar layer doesn't repaint correctly WinCairo can't render YouTube's custom scrollbar correctly.
Created attachment 460919 [details] test case
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.
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);
Created attachment 461033 [details] test case
Only scrollbar corner is filled with the background color. https://github.com/WebKit/WebKit/blob/4c3c8c892f788fe62ab12e103a5b3d0f1132b108/Source/WebCore/page/FrameView.cpp#L4268
Created attachment 461039 [details] Patch
Created attachment 461071 [details] Patch
Created attachment 461072 [details] Patch
Created attachment 461108 [details] Patch
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.
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.
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.
Created attachment 461202 [details] Patch for landing
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].
<rdar://problem/97572054>
Re-opening for pull request https://github.com/WebKit/WebKit/pull/2352
Re-opened since this is blocked by bug 243306
Created attachment 461289 [details] Patch for landing
Created attachment 461290 [details] Patch for landing
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.
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.
Created attachment 461411 [details] Patch
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].
This regressed GTK overlay scrollbars, they now have a random background.
Created attachment 461465 [details] [Screenshot] GTK regression
Thank you for reporting. Will revert the change.
Created attachment 461469 [details] Patch
Created attachment 461474 [details] Patch
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.
What do you mean? Do you still have the same problem with the new patch? I tested the patch with GTK MiniBrowser.
Oh wait. Sorry, I didn't realize there was a new patch, only saw a revert. 🤦️
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.
Comment on attachment 461474 [details] Patch Thank you very much for testing.
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)
Created attachment 461494 [details] Test case patch Here's the patch I used to remove the GTK scrollbar bg to use these colors.
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?
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.
Comment on attachment 461474 [details] Patch It seems to be a separate issue which should be filed separately.
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].