Bug 209805 – [GTK] Bring back support for rendering scrollbars using the system appearance

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

For backwards compatibility, but including new API to opt-out.

[Carlos Garcia Campos]

Created attachment 395032 [details]
Patch

[EWS Watchlist]

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

[Michael Catanzaro]

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?

[Carlos Garcia Campos]

(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.

[Michael Catanzaro]

(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.

[Michael Catanzaro]

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.

[Carlos Garcia Campos]

(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.

[Michael Catanzaro]

Well performance is important, certainly, but desktop integration is critical.

I don't know Carlos, it's a shame we can't have both. :/

[Michael Catanzaro]

Maybe use threaded rendering only if the current theme is Adwaita? :/

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

(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.

[Michael Catanzaro]

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?

[Carlos Garcia Campos]

(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.

[Adrian Perez]

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.

[Michael Catanzaro]

(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.

[Alice Mikhaylenko]

I was going to file a bug about the new scrollbar handle not having minimum height, but I guess this obsoletes it?

[Michael Catanzaro]

(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.

[Carlos Garcia Campos]

Committed r260065: <https://trac.webkit.org/changeset/260065>