Bug 155292 - [GTK] Scrollbars are broken once again with current GTK+ master
Summary: [GTK] Scrollbars are broken once again with current GTK+ master
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2016-03-10 01:43 PST by Carlos Garcia Campos
Modified: 2016-03-10 22:59 PST (History)
1 user (show)

See Also:


Attachments
Patch (11.99 KB, patch)
2016-03-10 02:45 PST, Carlos Garcia Campos
mcatanzaro: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2016-03-10 01:43:27 PST
So, basically most of the trouch theming properties have been moved to the scrollbar, and a new gadget "contents" has been added between the scrollbar and its children.
Comment 1 Carlos Garcia Campos 2016-03-10 02:45:46 PST
Created attachment 273553 [details]
Patch
Comment 2 Michael Catanzaro 2016-03-10 07:03:50 PST
Comment on attachment 273553 [details]
Patch

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

I'm surprised that some of these changes are not guarded by GTK_CHECK_VERSION, but I presume you've tested it with 3.16 or 3.18.

> Source/WebCore/ChangeLog:8
> +        Most of the trouch theming properties have been moved to the

trough

> Source/WebCore/platform/gtk/ScrollbarThemeGtk.cpp:217
> +    gtk_widget_path_iter_add_class(path.get(), -1, orientation == VerticalScrollbar ? "right" : "bottom");

Are you sure this is right? I thought these were used to indicate the position of the scrollbar within its trough, not the position of the scrollbar in the window itself. So a vertical scrollbar should start with the "top" style class rather than "right", and the horizontal scrollbar would start with "left" rather than "bottom". Then once the scrollbar moves any distance on the trough, that style class should be removed, and restored only if it reaches the very top/left/right/bottom again. A pain, so I thought it OK to ignore.

However I didn't check the code, and if you tell me these are now "overloaded" and necessary to get the border correct, then that's fine....
Comment 3 Carlos Garcia Campos 2016-03-10 08:15:37 PST
(In reply to comment #2)
> Comment on attachment 273553 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=273553&action=review
> 
> I'm surprised that some of these changes are not guarded by
> GTK_CHECK_VERSION, but I presume you've tested it with 3.16 or 3.18.

Yes, I tested with the gtk+ version in the internal jhbuild. I tried to recude the amount of ifdefed code.

> > Source/WebCore/ChangeLog:8
> > +        Most of the trouch theming properties have been moved to the
> 
> trough
> 
> > Source/WebCore/platform/gtk/ScrollbarThemeGtk.cpp:217
> > +    gtk_widget_path_iter_add_class(path.get(), -1, orientation == VerticalScrollbar ? "right" : "bottom");
> 
> Are you sure this is right? I thought these were used to indicate the
> position of the scrollbar within its trough, not the position of the
> scrollbar in the window itself. So a vertical scrollbar should start with
> the "top" style class rather than "right", and the horizontal scrollbar
> would start with "left" rather than "bottom". Then once the scrollbar moves
> any distance on the trough, that style class should be removed, and restored
> only if it reaches the very top/left/right/bottom again. A pain, so I
> thought it OK to ignore.
> 
> However I didn't check the code, and if you tell me these are now
> "overloaded" and necessary to get the border correct, then that's fine....

This is in Adwaita:

  // scrollbar border
  &.top { border-bottom: 1px solid $borders_color; }
  &.bottom { border-top: 1px solid $borders_color; }
  &.left { border-right: 1px solid $borders_color; }
  &.right { border-left: 1px solid $borders_color; }

So, if the top class is present it has 1 pixel border at the bottom, if bottom at the top, if left at the right and if right at the left. Thats' what we do now. We should fix this to support RTL scrollbars, but ATM vertical is always left (has a right border) and horizontal is bottom (has a top border).
Comment 4 Carlos Garcia Campos 2016-03-10 22:59:53 PST
Committed r197997: <http://trac.webkit.org/changeset/197997>