Created attachment 448372 [details] Recording of using the reproducer from the description, showcasing the scrollbar bug. Description of problem: We observe broken scrollbars with disabled overlay scrollbars in Eclipse, see: https://bugs.eclipse.org/bugs/show_bug.cgi?id=546870 * https://bugs.eclipse.org/bugs/attachment.cgi?id=278446 (screenshot) * https://bugs.eclipse.org/bugs/attachment.cgi?id=278447 (recording) Screenshot and recording from the Eclipse bugs are from RHEL 7.4, where the behavior was slightly different. On RHEL 7.9+, the scrollbars are "represented" by black bars; on RHEL 7.4 (and RHEL 7.2 I believe), the area of the scrollbars has painting artifacts. Version-Release number of selected component (if applicable): Red Hat Enterprise Linux release 9.0 Beta (Plow) gtk3-3.24.30-3.el9.x86_64 webkit2gtk3-2.32.3-2.el9.x86_64 How reproducible: Run snippet provided below. Steps to Reproduce: 1. Create an .html file "javadoc.html" to display, with contents: <html> <head> <style CHARSET="ISO-8859-1" TYPE="text/css"> html { font-size: 10pt; } </style> </head> <body style="overflow:scroll;" text="#1a1a1a" bgcolor="#ffffff"> <h5> <div style='word-wrap: break-word; position: relative; margin-left: 20px; padding-top: 2px; '> <a href='invalid_link'> <img alt='Open Declaration' style='border:none; position: absolute; width: 16px; height: 16px; left: -21px; ' src='file:invalid_path.png'/> </a>void <a class='header' href='invalid_link'>java</a>.<a class='header' href='invalid_link'>io</a>.<a class='header' href='invalid_link'>PrintStream</a>.println(<span style='font-weight:normal;'></span>int x) </div> </h5> <br><p>Prints an integer and then terminate the line. This method behaves as though it invokes <code><a href='invalid_link'>print(int)</a></code> and then <code><a href='invalid_link'>println()</a></code>.<dl><dt>Parameters:</dt><dd><b>x</b> The <code>int</code> to be printed.</dd></dl> </body> </html> 2. Create the GTK3 snippet "browser.cpp" with contents: #include <gtk/gtk.h> #include <webkit2/webkit2.h> static void destroyWindowCb(GtkWidget* widget, GtkWidget* window); static gboolean closeWebViewCb(WebKitWebView* webView, GtkWidget* window); // gcc -g browser.cpp `pkg-config --cflags --libs webkit2gtk-4.0 gtk+-3.0` -o BrowserExample && ./BrowserExample int main(int argc, char* argv[]) { // Initialize GTK+ gtk_init(&argc, &argv); // Create a window that will contain the browser instance GtkWidget *main_window = gtk_window_new(GTK_WINDOW_TOPLEVEL); gtk_window_set_default_size(GTK_WINDOW(main_window), 400, 300); // Create a browser instance WebKitWebView *webView = WEBKIT_WEB_VIEW(webkit_web_view_new()); // Put the browser area into the main window gtk_container_add(GTK_CONTAINER(main_window), GTK_WIDGET(webView)); // Set up callbacks so that if either the main window or the browser instance is // closed, the program will exit g_signal_connect(main_window, "destroy", G_CALLBACK(destroyWindowCb), NULL); g_signal_connect(webView, "close", G_CALLBACK(closeWebViewCb), main_window); // Load a web page into the browser instance webkit_web_view_load_uri(webView, "file:///home/sandreev/javadoc.html"); // Make sure that when the browser area becomes visible, it will get mouse // and keyboard events gtk_widget_grab_focus(GTK_WIDGET(webView)); // Make sure the main window and all its contents are visible gtk_widget_show_all(main_window); // Run the main GTK+ event loop gtk_main(); return 0; } static void destroyWindowCb(GtkWidget* widget, GtkWidget* window) { gtk_main_quit(); } static gboolean closeWebViewCb(WebKitWebView* webView, GtkWidget* window) { gtk_widget_destroy(window); return TRUE; } 3. Run the snippet with command line, disable overlay scrollbars: export GTK_OVERLAY_SCROLLING=0 && gcc -g browser.cpp `pkg-config --cflags --libs webkit2gtk-4.0 gtk+-3.0` -o BrowserExample && ./BrowserExample Actual results: The scrollbars are not displayed correctly. There are black bars instead of scrollbars, for most window sizes. If the window is small enough to actually require scrollbars (if overlay scrollbars were enabled) scrollbars seem to be fine. Expected results: The scrollbars are displayed correctly. Additional info: The broken scrollbars are not always seen (e.g. with .html from bug 234871, scrollbars are fine if steppers are disabled). Something in the .html snippet above seems to cause the bug; for variations of the .html (e.g. remove font size styling) I see normal scrollbars (with Adwaita theme). Maybe its the amount of text and its formatting, I'm not sure.
On the Eclipse bug, one of the previous SWT maintainers from RHEL bisected GTK3 to find the first commit which showed the problem: https://bugs.eclipse.org/bugs/show_bug.cgi?id=546870#c8 > (In reply to Eric Williams from comment #7) > > The issue only happens on GTK3.22 or greater -- I'm currently bisecting to > > see what broke it. > > I bisected GTK and found that this patch broke it: > https://gitlab.gnome.org/GNOME/gtk/commit/ > 9d719b99893297bdd1675217ba9a7c8575cc0d80 > > Specifically lines 193/194 in gdk/gdkdisplay.h. Why this is causing the > breakage I do not know -- more investigation is needed. I'm not sure how the changes related to WebKit.
Did some debugging. For non-native scrollbars this is an easy fix - just don't early return if a scrollbar is enabled (and it's not using overlay scrollbars). For native scrollbars there's a second problem in addition to that - when not enabled, the scrollbar width is 0 pixels. I'm not really sure where that's coming from atm.
(In reply to Alexander Mikhaylenko from comment #2) > Did some debugging. > > For non-native scrollbars this is an easy fix - just don't early return if a > scrollbar is enabled (and it's not using overlay scrollbars). Ah cool, thanks for investigating. > For native scrollbars there's a second problem in addition to that - when > not enabled, the scrollbar width is 0 pixels. I'm not really sure where > that's coming from atm. I see the width as 1 pixel, not 0. I have made some other local changes to the code that could possibly affect that, though. Anyway, that's caused by this part of the code: // When using overlay scrollbars we always claim the size of the scrollbar when hovered, so when // drawing the indicator we need to adjust the rectangle to its actual size in indicator mode. if (scrollbar.orientation() == ScrollbarOrientation::Vertical) { if (rect.width() != preferredSize.width()) { if (!scrollbar.scrollableArea().shouldPlaceVerticalScrollbarOnLeft()) contentsRect.move(std::abs(rect.width() - preferredSize.width()), 0); contentsRect.setWidth(preferredSize.width()); } } else { if (rect.height() != preferredSize.height()) { contentsRect.move(0, std::abs(rect.height() - preferredSize.height())); contentsRect.setHeight(preferredSize.height()); } } I don't quite understand how it works, but I think that should execute only if we have overlay scrollbars, not always, so I surrounded it with an if (usesOverlayScrollbars()) condition and now the width looks correct. But the scrollbars are still not painted properly, and I've been struggling to figure out why.
(In reply to Alexander Mikhaylenko from comment #2) > For non-native scrollbars this is an easy fix - just don't early return if a > scrollbar is enabled (and it's not using overlay scrollbars). That change is: diff --git a/Source/WebCore/platform/adwaita/ScrollbarThemeAdwaita.cpp b/Source/WebCore/platform/adwaita/ScrollbarThemeAdwaita.cpp index 780db99a1a79..46757ee3861e 100644 --- a/Source/WebCore/platform/adwaita/ScrollbarThemeAdwaita.cpp +++ b/Source/WebCore/platform/adwaita/ScrollbarThemeAdwaita.cpp @@ -117,7 +117,7 @@ bool ScrollbarThemeAdwaita::paint(Scrollbar& scrollbar, GraphicsContext& graphic if (graphicsContext.paintingDisabled()) return false; - if (!scrollbar.enabled()) + if (!scrollbar.enabled() && usesOverlayScrollbars()) return true; IntRect rect = scrollbar.frameRect(); I'm going to attach a screenshot. It's certainly an improvement, but it looks kind of weird to have only a trough and no slider there. I wonder if perhaps we ought to draw a slider that fills the entire trough? What does Safari do?
Created attachment 451530 [details] Screenshot of proposed solution
The proposed solution looks fine to me. Safari does the same thing.
(In reply to Alexander Mikhaylenko from comment #6) > The proposed solution looks fine to me. Safari does the same thing. I have to ask, how does this "look fine"?
Created attachment 451536 [details] Safari screenshot Here's Safari for comparison. What would you expect to see instead? The scrollbars are there - they have no sliders because the thing isn't scrollable, but they are there as the page requested, and they are not glitched.
(In reply to Alexander Mikhaylenko from comment #8) > Created attachment 451536 [details] > Safari screenshot > > Here's Safari for comparison. What would you expect to see instead? The > scrollbars are there - they have no sliders because the thing isn't > scrollable, but they are there as the page requested, and they are not > glitched. OK, so there will be sliders if there is something to scroll? Let me try the fix in Eclipse.
I mean those were never broken to begin with? Your page explicitly requests scrollbars even if there's nothing to scroll - so I don't see why this would be surprising that there aren't any.
(In reply to Simeon Andreev from comment #9) > OK, so there will be sliders if there is something to scroll? Let me try the > fix in Eclipse. That's only a fix for the non-native scrollbars (webkit_web_context_set_use_system_appearance_for_scrollbars = FALSE). We're still trying to figure out how to fix native scrollbars. (In reply to Alexander Mikhaylenko from comment #10) > I mean those were never broken to begin with? Your page explicitly requests > scrollbars even if there's nothing to scroll - so I don't see why this would > be surprising that there aren't any. Our understanding is that this bug occurs if scrollbars are requested but the content is not scrollable, and everything is fine if you have enough content as to be scrollable. If that's not correct, then you must be seeing something different than what we're seeing....
(In reply to Michael Catanzaro from comment #11) > Our understanding is that this bug occurs if scrollbars are requested but > the content is not scrollable, and everything is fine if you have enough > content as to be scrollable. If that's not correct, then you must be seeing > something different than what we're seeing.... Sorry I assumed the scrollbars will have no sliders altogether (regardless of whether there is something to scroll or not). Normally there are either sliders or steppers or both, even without anything to scroll. But as long as there are sliders when there is something to scroll, probably that is enough. I'll check how the patch behaves with our theme (has steppers enabled) once I manage to build WebKit on RHEL 9.
(In reply to Simeon Andreev from comment #12) > Sorry I assumed the scrollbars will have no sliders altogether (regardless > of whether there is something to scroll or not). Normally there are either > sliders or steppers or both, even without anything to scroll. But as long as > there are sliders when there is something to scroll, probably that is enough. Ah really? Even if you have no scrollable content at all, you see the sliders and steppers in GTK apps? Maybe it would help to post a screenshot of what you see in a normal GTK app. Alexander checked the behavior in Safari and found it displays just the trough and no slider when there is nothing to scroll, so that's what we had been trying to match, but I suppose trying to match GTK's behavior probably makes more sense.
Created attachment 451568 [details] GTK scrollbar screenshot GTK does have a slider, that you can still hover etc - it just looks like there's something to scroll, but there isn't. That said, I can't remember when I saw a GTK app doing this last time (you really have to go out of your way to get this behavior), so not sure it makes sense here.
Created attachment 451573 [details] Screen recording showing "always show" and "automatic" scrollbar policies in GTK3. It depends what the GtkScrolledWindow scrollbar policy is, Eclipse usually "GTK_POLICY_ALWAYS" (scrollbars are always shown, see first part of the recording). Default I believe is "GTK_POLICY_AUTOMATIC" (2nd part of the recording).
So we have three cases: * GTK_POLICY_AUTOMATIC style: this is WebKit's normal behavior, but it gets disabled in this case because the Javadoc web content uses overflow:scroll * GTK_POLICY_ALWAYS style: show the scrollbar slider at full-width, even though there is nothing to scroll * Safari style: show only the scrollbar background WebKit doesn't let you set a scroll policy, and I see little point in adding API to allow configuring this, so we should probably just pick one way or the other. (In reply to Michael Catanzaro from comment #13) > Alexander checked the behavior in Safari and found it displays just the > trough and no slider when there is nothing to scroll, so that's what we had > been trying to match, but I suppose trying to match GTK's behavior probably > makes more sense. Apparently I didn't know what the trough was. I mistook it for the background of the scrollbar, because GTK 3 Adwaita does not have a trough. This was wrong. After Alexander set me straight, I was finally able to fix the GTK code so that the scrollbar background is shown. What I have now looks just like attachment #451530 [details]. Next, I will see how hard it is to make it show the slider as well, which would match GTK_POLICY_ALWAYS behavior rather than Safari. Honestly, that seems better to me, because I don't think the scrollbars without sliders look very good (certainly not compared to Safari). But I won't spend much more time on this: if this turns out to be hard, I'll move on, because native rendering is obsolete (gone in GTK 4 port), and non-overlay scrollbars are a non-default configuration, and because drawing the scrollbar backgrounds is sufficient to fix the plainly broken rendering. I consider the slider to be a bonus.
I was able to draw the sliders, but they don't behave quite right (e.g. prelight is broken) and I'm not immediately sure how to fix it. Let's draw just the backgrounds for now. Can always add sliders later....
Created attachment 451605 [details] Patch
(In reply to Michael Catanzaro from comment #18) > Created attachment 451605 [details] > Patch I can't wait to see how many tests this breaks. :(
(In reply to Michael Catanzaro from comment #19) > (In reply to Michael Catanzaro from comment #18) > > Created attachment 451605 [details] > > Patch > > I can't wait to see how many tests this breaks. :( Tests don't use native scrollbars IIRC
Well this changed behavior for the non-native scrollbars too, but the EWS are all fine, so I guess we must not have any tests covering this (or more likely, they are all already marked as expected fail).
Committed r289620 (247131@main): <https://commits.webkit.org/247131@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 451605 [details].
(In reply to Michael Catanzaro from comment #21) > Well this changed behavior for the non-native scrollbars too, but the EWS > are all fine, so I guess we must not have any tests covering this (or more > likely, they are all already marked as expected fail). With native I actually meant that layout tests draw its own themed scrollbars without using ScrollbarThemeAdwaita nor ScrollbarThemeGtk
Created attachment 451866 [details] Eclipse on RHEL 9 without WebKit fixes.
Created attachment 451867 [details] Eclipse on RHEL 9 with fixes. Looks like the fix for overlay scrollbar works, see attached screen recordings with Eclipse. What I can't figure out is why the word wrap is used instead of bottom scrollbar (in particular to test the changes for bug 234871). The Eclipse version I'm using doesn't have the workaround we have added: https://git.eclipse.org/c/platform/eclipse.platform.text.git/commit/?id=663a6e5f991dce44e8d68ef1007e086e051b2778 The bottom scrollbar should be used here instead of word wrap...
Created attachment 451868 [details] Double scrollbars with GTK3 snippet from the description. What is weird is that there are now double scrollbars in a GTK3 based snippet that uses a WebKit Browser view, see screen recording. I'm not sure where those are coming from.
(In reply to Simeon Andreev from comment #26) > Created attachment 451868 [details] > Double scrollbars with GTK3 snippet from the description. > > What is weird is that there are now double scrollbars in a GTK3 based > snippet that uses a WebKit Browser view, see screen recording. > > I'm not sure where those are coming from. .html contents: <!DOCTYPE html> <html> <head> <style> div.scroll { margin: 4px, 4px; padding: 4px; width: 500px; overflow: scroll; } </style> </head> <body> <div class="scroll"> "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum." </div> </body> </html> GTK3 snippet: #include <gtk/gtk.h> #include <webkit2/webkit2.h> static void destroyWindowCb(GtkWidget* widget, GtkWidget* window); static gboolean closeWebViewCb(WebKitWebView* webView, GtkWidget* window); // gcc -g browser.cpp `pkg-config --cflags --libs webkit2gtk-4.0 gtk+-3.0` -o BrowserExample && ./BrowserExample int main(int argc, char* argv[]) { // Initialize GTK+ gtk_init(&argc, &argv); // Create an 800x600 window that will contain the browser instance GtkWidget *main_window = gtk_window_new(GTK_WINDOW_TOPLEVEL); gtk_window_set_default_size(GTK_WINDOW(main_window), 300, 200); // Create a browser instance WebKitWebView *webView = WEBKIT_WEB_VIEW(webkit_web_view_new()); // Put the browser area into the main window gtk_container_add(GTK_CONTAINER(main_window), GTK_WIDGET(webView)); // Set up callbacks so that if either the main window or the browser instance is // closed, the program will exit g_signal_connect(main_window, "destroy", G_CALLBACK(destroyWindowCb), NULL); g_signal_connect(webView, "close", G_CALLBACK(closeWebViewCb), main_window); //WebKitWebContext *context = webkit_web_view_get_context(webView); //webkit_web_context_set_use_system_appearance_for_scrollbars(context, FALSE); // Load a web page into the browser instance webkit_web_view_load_uri(webView, "file:///home/sandreev/gtk/html/javadoc.html"); //WebKitWebContext *context = webkit_web_view_get_context(webView); //webkit_web_context_set_use_system_appearance_for_scrollbars(context, FALSE); // Make sure that when the browser area becomes visible, it will get mouse // and keyboard events gtk_widget_grab_focus(GTK_WIDGET(webView)); // Make sure the main window and all its contents are visible gtk_widget_show_all(main_window); // Run the main GTK+ event loop gtk_main(); return 0; } static void destroyWindowCb(GtkWidget* widget, GtkWidget* window) { gtk_main_quit(); } static gboolean closeWebViewCb(WebKitWebView* webView, GtkWidget* window) { gtk_widget_destroy(window); return TRUE; }
Let's discuss the double scrollbars in a new bug. I can't reproduce, but I guess it's probably triggered by either the margin or the padding.
Actually, sorry, I can reproduce it. But I'm not sure that it's a bug. You applied the style to just the div, not to the entire document, so the div really gets its own scrollbars separate from the scrollbars for the entire window. In your earlier example, the style was applied to the entire document body, which is why we only saw one set of scrollbars.
(In reply to Michael Catanzaro from comment #29) > Actually, sorry, I can reproduce it. But I'm not sure that it's a bug. You > applied the style to just the div, not to the entire document, so the div > really gets its own scrollbars separate from the scrollbars for the entire > window. In your earlier example, the style was applied to the entire > document body, which is why we only saw one set of scrollbars. Alright, if the .html snippet is broken that is fine. I'll double check with actual Eclipse .html just in case.
OK, with the .html from an Eclipse JavaDoc hover there is only 1 set of scrollbars (the expected ones). I still can't get the word-wrap behavior to go away; any idea why that is? .html is: <html><head><style CHARSET="ISO-8859-1" TYPE="text/css">/* Font definitions */ html { font-family: 'Sans Serif',sans-serif; font-size: 10pt; font-style: normal; font-weight: normal; } body, h1, h2, h3, h4, h5, h6, p, table, td, caption, th, ul, ol, dl, li, dd, dt { font-size: 1em; } pre { font-family: monospace; } /* Margins */ body { overflow: scroll; margin-top: 0px; margin-bottom: 0.5em; margin-left: 0.3em; margin-right: 0px; } h1 { margin-top: 0.3em; margin-bottom: 0.04em; } h2 { margin-top: 2em; margin-bottom: 0.25em; } h3 { margin-top: 1.7em; margin-bottom: 0.25em; } h4 { margin-top: 2em; margin-bottom: 0.3em; } h5 { margin-top: 0px; margin-bottom: 0px; } p { margin-top: 1em; margin-bottom: 1em; } pre { margin-left: 0.6em; } ul { margin-top: 0px; margin-bottom: 1em; margin-left: 1em; padding-left: 1em;} li { margin-top: 0px; margin-bottom: 0px; } li p { margin-top: 0px; margin-bottom: 0px; } ol { margin-top: 0px; margin-bottom: 1em; margin-left: 1em; padding-left: 1em; } dl { margin-top: 0px; margin-bottom: 1em; } dt { margin-top: 0px; margin-bottom: 0px; font-weight: bold; } dd { margin-top: 0px; margin-bottom: 0px; } /* Styles and colors */ a:link { color: #0000ee; } /* get's replaced with actual color */ a:hover { color: #0000a7; } /* get's replaced with actual color */ a:visited { text-decoration: underline; } a.header:link { text-decoration: none; color: #1a1a1a } a.header:visited { text-decoration: none; color: #1a1a1a } a.header:hover { text-decoration: underline; color: #0000a7; } /* get's replaced with actual color */ h4 { font-style: italic; } strong { font-weight: bold; } em { font-style: italic; } var { font-style: italic; } th { font-weight: bold; } /* Workarounds for new Javadoc stylesheet (1.7) */ ul.blockList li.blockList, ul.blockListLast li.blockList { list-style:none; } ul.blockList ul.blockList ul.blockList ul.blockList li.blockListLast { list-style:none; } </style> <base href='https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/System.html'> </head><body style="overflow:scroll" text="#1a1a1a" bgcolor="#ffffff"><h5><div style='position: relative; margin-left: 20px; padding-top: 2px; '><a href='eclipse-open:%E2%98%82=Test/%5C/home%5C/sandreev%5C/java%5C/jdk-17%5C/lib%5C/jrt-fs.jar%60java.base=/module=/true=/=/javadoc_location=/https:%5C/%5C/docs.oracle.com%5C/en%5C/java%5C/javase%5C/17%5C/docs%5C/api%5C/=/%3Cjava.lang(System.class%E2%98%83System'><!--[if lte IE 6]><![if gte IE 5.5]> <span alt='Open Declaration' style="border:none; position: absolute; width: 16px; height: 16px; left: -21px; filter:progid:DXImageTransform.Microsoft.AlphaImageLoader(src='file:/home/sandreev/tmp/runtime-Eclipse/.metadata/.plugins/org.eclipse.jdt.ui/jdt-images/0.png')"></span> <![endif]><![endif]--> <!--[if !IE]>--> <img alt='Open Declaration' style='border:none; position: absolute; width: 16px; height: 16px; left: -21px; ' src='file:/home/sandreev/tmp/runtime-Eclipse/.metadata/.plugins/org.eclipse.jdt.ui/jdt-images/0.png'/> <!--<![endif]--> <!--[if gte IE 7]> <img alt='Open Declaration' style='border:none; position: absolute; width: 16px; height: 16px; left: -21px; ' src='file:/home/sandreev/tmp/runtime-Eclipse/.metadata/.plugins/org.eclipse.jdt.ui/jdt-images/0.png'/> <![endif]--> </a><a class='header' href='eclipse-javadoc:%E2%98%82=Test/%5C/home%5C/sandreev%5C/java%5C/jdk-17%5C/lib%5C/jrt-fs.jar%60java.base=/module=/true=/=/javadoc_location=/https:%5C/%5C/docs.oracle.com%5C/en%5C/java%5C/javase%5C/17%5C/docs%5C/api%5C/=/%3Cjava'>java</a>.<a class='header' href='eclipse-javadoc:%E2%98%82=Test/%5C/home%5C/sandreev%5C/java%5C/jdk-17%5C/lib%5C/jrt-fs.jar%60java.base=/module=/true=/=/javadoc_location=/https:%5C/%5C/docs.oracle.com%5C/en%5C/java%5C/javase%5C/17%5C/docs%5C/api%5C/=/%3Cjava.lang'>lang</a>.System</div></h5><br><p>The <code>System</code> class contains several useful class fields and methods. It cannot be instantiated. Among the facilities provided by the <code>System</code> class are standard input, standard output, and error output streams; access to externally defined properties and environment variables; a means of loading files and libraries; and a utility method for quickly copying a portion of an array.<dl><dt>Since:</dt><dd> 1.0</dd></dl></body></html>
> I still can't get the word-wrap behavior to go away; any idea why that is? In particular on RHEL 7.9 (webkitgtk4-2.28.2-2.el7.x86_64) the same .html doesn't do word wrap.
> I still can't get the word-wrap behavior to go away; any idea why that is? No clue, sorry....