For backwards compatibility, but including new API to opt-out.
Created attachment 395032 [details] Patch
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment on attachment 395032 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395032&action=review I've skimmed over the WebCore changes, since they are mostly restoring previous code. > Source/WebKit/ChangeLog:9 > + Add WebKitWebContext:use-system-appearance-for-scrollbars property. It's enabled by default to keep backwards > + compatibility. Why? Would an app ever really want to change this setting? For GTK 3, I think every app wants to use it, and no apps would want to turn it off. For GTK 4, there's no way to make it work, so every app will need to have it off. So I don't think the new property is needed. > Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:539 > + * This is enabled by default for backwards compatibility, but it's only > + * recommened to use when the application includes other widgets to ensure > + * consistency, or when consistency with other applications is required too. I can't think of any reason to turn it off...? Isn't consistency with other applications always required for GTK? I think so. > Tools/ChangeLog:8 > + Do not use system appearance for scrollbars in MiniBrowser and unit tests. Hm, I guess we could find a private way to disable the themed scrollbars for layout tests? Something that doesn't require exposing public API?
(In reply to Michael Catanzaro from comment #3) > Comment on attachment 395032 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=395032&action=review > > I've skimmed over the WebCore changes, since they are mostly restoring > previous code. > > > Source/WebKit/ChangeLog:9 > > + Add WebKitWebContext:use-system-appearance-for-scrollbars property. It's enabled by default to keep backwards > > + compatibility. > > Why? Would an app ever really want to change this setting? Apps not using other widgets, like web browsers. It's more efficient and would allow to enable threaded rendering (when it's ready). I expect ephy use the setting to disable system appearance. > For GTK 3, I > think every app wants to use it, and no apps would want to turn it off. For > GTK 4, there's no way to make it work, so every app will need to have it > off. So I don't think the new property is needed. > > > Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:539 > > + * This is enabled by default for backwards compatibility, but it's only > > + * recommened to use when the application includes other widgets to ensure > > + * consistency, or when consistency with other applications is required too. > > I can't think of any reason to turn it off...? Isn't consistency with other > applications always required for GTK? I think so. Not so important for a web browser, I would say, web sites can use their own scrollbars, so we are used to not being always consistent. The same way we are no longer consistent when rendering buttons or any other controls. Other apps like evolution would look really weird if the emails list uses one scrollbar and the main view a different one. > > Tools/ChangeLog:8 > > + Do not use system appearance for scrollbars in MiniBrowser and unit tests. > > Hm, I guess we could find a private way to disable the themed scrollbars for > layout tests? Something that doesn't require exposing public API? We are not using any api for layout tests, it's disabled by default, because layout tests don't use the glib api.
(In reply to Carlos Garcia Campos from comment #4) > Apps not using other widgets, like web browsers. It's more efficient and > would allow to enable threaded rendering (when it's ready). I expect ephy > use the setting to disable system appearance. I think native scrollbar is more important for Ephy than threaded rendering is.
Comment on attachment 395032 [details] Patch OK, r=me, but one caveat: since I don't like the new API, I'd like another GTK maintainer (probably Adrian, I wonder what he thinks about this) to approve the new API before you commit it.
(In reply to Michael Catanzaro from comment #5) > (In reply to Carlos Garcia Campos from comment #4) > > Apps not using other widgets, like web browsers. It's more efficient and > > would allow to enable threaded rendering (when it's ready). I expect ephy > > use the setting to disable system appearance. > > I think native scrollbar is more important for Ephy than threaded rendering > is. Performance is not important for ephy? I really want to disable themed scrollbars in ephy, if that's not going to happen, maybe it's better not to add api for this.
Well performance is important, certainly, but desktop integration is critical. I don't know Carlos, it's a shame we can't have both. :/
Maybe use threaded rendering only if the current theme is Adwaita? :/
(In reply to Michael Catanzaro from comment #8) > Well performance is important, certainly, but desktop integration is > critical. We have already lost the desktop integration for the rest of the themed controls. I understand scrollbars are different when apps are mixing them, but not for ephy. And it's going to be even worse with GTK4 that we can't even get selection colors. > I don't know Carlos, it's a shame we can't have both. :/ I always said desktop integration was one of the key features of WebKitGTK compared to other browsers/engines, but you (and others) convinced me that it was causing more problems than the benefits.
(In reply to Michael Catanzaro from comment #9) > Maybe use threaded rendering only if the current theme is Adwaita? :/ Threaded rendering was just an example, it's not even ready yet. Even without it, the new code is a lot more efficient. Note that it's based on Adwaita, but it's not Adwaita and it could even diverge more. The theming code (when using gtk foreign drawing) is the only place where we render directly to the cairo context, instead of using GraphicsContext API. That might cause more problems in the future.
Comment on attachment 395032 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395032&action=review I don't have a good answer. For GTK 4, where we have no choice, I guess we can blame GTK 4 when users complain. GTK 4 will be stable very soon anyway. But as long as we're using GTK 3, I think I'd prefer native scrollbars to avoid user complaints, since we can't blame GTK 4 for that. > Tools/MiniBrowser/gtk/main.c:546 > + WebKitWebContext *webContext = g_object_new(WEBKIT_TYPE_WEB_CONTEXT, "website-data-manager", manager, "process-swap-on-cross-site-navigation-enabled", TRUE, > + "use-system-appearance-for-scrollbars", FALSE, NULL); Since this property is not construct-only, and can be changed after the web process has started... why not put it into WebKitSettings instead?
(In reply to Michael Catanzaro from comment #12) > Comment on attachment 395032 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=395032&action=review > > I don't have a good answer. > > For GTK 4, where we have no choice, I guess we can blame GTK 4 when users > complain. GTK 4 will be stable very soon anyway. But as long as we're using > GTK 3, I think I'd prefer native scrollbars to avoid user complaints, since > we can't blame GTK 4 for that. I don't think this is about who to blame, but what behavior we want for WebKitGTK. > > Tools/MiniBrowser/gtk/main.c:546 > > + WebKitWebContext *webContext = g_object_new(WEBKIT_TYPE_WEB_CONTEXT, "website-data-manager", manager, "process-swap-on-cross-site-navigation-enabled", TRUE, > > + "use-system-appearance-for-scrollbars", FALSE, NULL); > > Since this property is not construct-only, and can be changed after the web > process has started... why not put it into WebKitSettings instead? Because WebKitSettings can be set per web view, but RenderScrollbarTheme is a singleton in the web process, so we can't change the setting for a particular page.
I also find it appealing to be able to make the setting controllable by the application. There are many cases in which one may want the extra performance, or the web content itself is already overriding the scrollbars (e.g. Riot/Revolt) and having the logic enabled for native-looking scrollbars is not particularly useful anyway… I am okay with the API to opt-in to use scrollbars rendered by GTK, it's not great but we already have other not-so-great opt-ins anyway in the public API (like PSON being an opt-in, too). TL;DR: I see the point in allowing the application to control which kind of scrollbars to use, the API is not great but I am okay with it.
(In reply to Adrian Perez from comment #14) > There are many cases in which one may want the > extra performance, or the web content itself is already overriding > the scrollbars (e.g. Riot/Revolt) and having the logic enabled for > native-looking scrollbars is not particularly useful anyway… OK, you convinced me that the WebKitWebContext API makes sense.
I was going to file a bug about the new scrollbar handle not having minimum height, but I guess this obsoletes it?
(In reply to Alexander Mikhaylenko from comment #16) > I was going to file a bug about the new scrollbar handle not having minimum > height, but I guess this obsoletes it? No, we should fix both scrollbar modes. This patch adds API to choose whether you get native scrollbars or not.
Committed r260065: <https://trac.webkit.org/changeset/260065>