Bug 155292 – [GTK] Scrollbars are broken once again with current GTK+ master

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

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Created attachment 273553 [details]
Patch

[Michael Catanzaro]

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

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

Committed r197997: <http://trac.webkit.org/changeset/197997>