803633 - [Linux gtk] Honor scrollbar preference of system

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Bill Nottingham]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:16.0) Gecko/20100101 Firefox/16.0
Build ID: 20121011153642

Steps to reproduce:

Recent versions of GTK have a preference:
  gtk-primary-button-warps-slider

This setting changes the behavior when the scrollbar trough is clicked.

When this setting is true:
  Primary (left) button: warps slider to position that is clicked
  Secondary (right) button: scrolls one page

When this setting is false:
  Primary (left) button: scrolls one page
  Tertiary (middle) button: warps slider to position that is clicked

Firefox unilaterally follows the behavior that  corresponds to the setting being false. It should read the setting and respond appropriately, integrating with the rest of the system consistently.

Comment 1

[Aleksandr Seleznev]

Attachment: xulrunner-gtk-warps-slider.patch

I tried make a patch, but it's my first experience. I divided changes to few patches.

Summary of changes:

1. Remove 'pref("ui.scrollToClick", 0);' line from all.js.
2. LookAndFeel::GetInt(LookAndFeel::eIntID_ScrollToClick, false) will return true.

Comment 2

[Aleksandr Seleznev]

Attachment: xulrunner-gtk-scrollbar-behavior.patch

3. Disable Middle mouse button.
4. Enable Right button for scroll page up/down.

Comment 3

[Aleksandr Seleznev]

Attachment: xulrunner-gtk-warps-slider.patch

It's just update.

Comment 4

[Aleksandr Seleznev]

Attachment: xulrunner-gtk-scrollbar-behavior.patch

Comment 6

[Aleksandr Seleznev]

Attachment: gtk-scrollbar-behavior.patch

Update and merge patches to one.

Comment 7

[Karl Tomlinson (:karlt)]

Comment on attachment 8456843 [details] [diff] [review]
gtk-scrollbar-behavior.patch

I'm not familiar with nsSliderFrame.

Can you explain please why mInfiniteScroll is necessary for eRightButton page
scrolling, but not for eLeftButton page scrolling?
Can these both use the same mechanism?

>+            settings = gtk_settings_get_default ();
>+            g_object_get (settings,
>+                          "gtk-primary-button-warps-slider",
>+                          &warps_slider,
>+                          nullptr);

g_object_get() should only be used if the property is known to exist, but
older GTK versions don't have the gtk-primary-button-warps-slider property.
The only way I know to check whether the property exists is
g_object_class_find_property().

Comment 8

[Markus Stange [:mstange]]

I can review the nsSliderFrame change, but please split the patches into one that just implements scroll-to-click with the mouse button handling that's right for gtk, and into one that adds the mInfiniteScroll behavior. And please add a more detailed description of the resulting behavior that mInfiniteScroll is used for, because just from reading the patch I have no idea what it's supposed to do.

Comment 9

[Aleksandr Seleznev]

Attachment: gtk-scrollbar-behavior-part1.patch

Main part

Comment 10

[Aleksandr Seleznev]

Attachment: gtk-scrollbar-behavior-part2.patch

Comment 11

[Aleksandr Seleznev]

(In reply to Karl Tomlinson (:karlt) from comment #7)
> Can you explain please why mInfiniteScroll is necessary for eRightButton page
> scrolling, but not for eLeftButton page scrolling?

Oh... You are right, in GTK+ left button with
gtk-primary-button-warps-slider=FALSE works like right button with
gtk-primary-button-warps-slider=TRUE now.

> Can these both use the same mechanism?

I'm not sure that older GTK+ versions works like it too. Do we heed to have
different behavior for different GTK+ versions?

> g_object_class_find_property()

Thank you! I added check for it.

Comment 12

[Aleksandr Seleznev]

Attachment: right-button-spec.png

Comment 13

[Aleksandr Seleznev]

(In reply to Markus Stange [:mstange] from comment #8)
> please split the patches

Done. But "mInfiniteScroll behavior" is a part of GTK+ behavior. :(

> And please add a more detailed description of the resulting behavior that
> mInfiniteScroll is used for

When you press and hold left/right Firefox will scroll page to currect mouse
position [1]. In GTK+ - to top/bottom position of page. Please look on attached
picture and screencast [2].

I have no idea how to make it perfect. In last patch I try to set
mDestinationPoint to left.top/right.bottom point of the clientRect (not sure
that it's correct). Also this patch affected default behaviour too
(ui.scrollToClick = 0).

1: https://dl.dropboxusercontent.com/u/107670598/bugzilla.mozilla.org/bug803633/firefox-after-part1.ogg

2: https://dl.dropboxusercontent.com/u/107670598/bugzilla.mozilla.org/bug803633/gtk-press-and-hold.ogg

Comment 14

[Aleksandr Seleznev]

Attachment: gtk-scrollbar-behavior-part3.patch

Thumb should has :active state when dragging after middle click
(ui.scrollToClick=0) or left click (ui.scrollToClick=1). This patch should fix
it [1].

1: https://dl.dropboxusercontent.com/u/107670598/bugzilla.mozilla.org/bug803633/firefox-after-part3.ogg

Comment 15

[Karl Tomlinson (:karlt)]

Comment on attachment 8462548 [details] [diff] [review]
gtk-scrollbar-behavior-part1.patch

The nsLookAndFeel.cpp part is good, thanks.
I'm hoping Markus knows more about nsSliderFrame.

>+#if !defined(XP_MACOSX) && !defined(MOZ_WIDGET_GTK)
>+// On OS X, we follow the "Click in the scrollbar to:" system preference
>+// unless this preference was set manually
>+// or the "gtk-primary-button-warps-slider" property (since GTK 2.24 / 3.6)

Currently this sentence reads as if it all applies only to OS X, but of course part applies to GTK 2.  Perhaps 'We follow the "Click in the scrollbar to:" system preference on OS X and "gtk-primary-button-warps-slider" property with GTK (since 2.24 / 3.6), unless this preference is explicitly set.'
I would also move it to before the #if because it applies when that block is not compiled.

Comment 16

[Karl Tomlinson (:karlt)]

(In reply to Alexander Seleznev from comment #11)
> I'm not sure that older GTK+ versions works like it too. Do we [n]eed to have
> different behavior for different GTK+ versions?

I don't know what older GTK+ versions did but recent 2 and 3 versions seem to have the same behavior (except for default values of the pref).  I think we can use the same behavior for all versions, even if older versions were a bit different.

Comment 17

[Markus Stange [:mstange]]

Comment on attachment 8462548 [details] [diff] [review]
gtk-scrollbar-behavior-part1.patch

Review of attachment 8462548 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/xul/nsSliderFrame.cpp
@@ +535,5 @@
>      mDragStart = pos - mThumbStart;
>    }
> +#ifdef MOZ_WIDGET_GTK
> +  else if (ShouldScrollForEvent(aEvent) &&
> +           aEvent->AsMouseEvent()->button == WidgetMouseEvent::eRightButton) {

Please add a comment:
    // HandlePress and HandleRelease are usually called via
    // nsFrame::HandleEvent, but only for the left mouse button.

Comment 18

[Markus Stange [:mstange]]

Comment on attachment 8462549 [details] [diff] [review]
gtk-scrollbar-behavior-part2.patch

Review of attachment 8462549 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you very much for the helpful diagrams and videos. I'm much more confident in your patches now than I was before. I've suggested some comments that would make the code a little clearer, because much of nsSliderFrame.cpp is written in a rather confusing way.

::: layout/xul/nsSliderFrame.cpp
@@ +434,5 @@
>        if (mChange) {
>          // We're in the process of moving the thumb to the mouse,
> +        // but the mouse just moved.
> +#ifndef MOZ_WIDGET_GTK
> +        // Make sure to update our destination point.

Change this comment to:
        // On Linux the destination point is determined by the initial click
        // on the scrollbar track and doesn't change until the mouse button
        // is released.
        // On the other platforms we need to update the destination point now.

@@ +1064,5 @@
>                       : eventPoint.y < thumbRect.y)
>      change = -1;
>  
>    mChange = change;
>    DragThumb(true);

Add a comment:
  // On Linux we want to keep scrolling in the direction indicated by |change|
  // until the mouse is released. On the other platforms we want to stop
  // scrolling as soon as the scrollbar thumb has reached the current mouse
  // position.

@@ +1068,5 @@
>    DragThumb(true);
> +#ifdef MOZ_WIDGET_GTK
> +  nsRect clientRect;
> +  GetClientRect(clientRect);
> +

Add a comment:
  // Set the destination point to the very end of the scrollbar so that
  // scrolling doesn't stop halfway through.

Comment 19

[Markus Stange [:mstange]]

Comment on attachment 8462978 [details] [diff] [review]
gtk-scrollbar-behavior-part3.patch

Review of attachment 8462978 [details] [diff] [review]:
-----------------------------------------------------------------

Can you put the attribute changes into nsSliderFrame::DragThumb? Then you wouldn't need to repeat the code that sets the attribute in two places. Or does DragThumb not catch all the cases?

Comment 20

[Aleksandr Seleznev]

Attachment: gtk-scrollbar-behavior-part1.patch

// Added comment.

Comment 21

[Aleksandr Seleznev]

Attachment: gtk-scrollbar-behavior-part2.patch

// Added many comments.

Comment 22

[Aleksandr Seleznev]

Attachment: gtk-scrollbar-behavior-part3.patch

Comment 23

[Aleksandr Seleznev]

(In reply to Markus Stange [:mstange] from comment #19)
> Can you put the attribute changes into nsSliderFrame::DragThumb? Then you
> wouldn't need to repeat the code that sets the attribute in two places.

And call this method like "nsSliderFrame::DragThumb(true, true);"? Not sure
that it will look nice.

Setting [active] attribute needed only in two cases:

1. After warp slider to current mouse position after left button press on a
scroolbar (scroll-to-click). Also thumb can be dragged to new position until
button isn't released [1].

2. Firefox doesn't change thumb's state to :active when you drag them by the
right mouse button (instead of the left button).

nsSliderFrame::DragThumb called in "infinity scrolling" case too, but thumb
shouldn't have :active state.

1: https://dl.dropboxusercontent.com/u/107670598/bugzilla.mozilla.org/bug803633/scroll-to-click.ogg

Comment 24

[Aleksandr Seleznev]

Attachment: gtk-scrollbar-behavior-part4.patch

Well, I found big problem with part1.patch. It's work badly when you press two
mouse buttons in one time. When you have only one mouse button what can start
scrolling actions (like under MS Windows) it isn't problem. But after 
part1.patch simultaneous pressing mouse buttons can lead to unexpected results
(like not stopping scroll after release all buttons).

Best variant it's just ignore another clicks (like in GTK+ and MS Windows) but
I don't know how to make it correct in Firefox. After apply part4.patch
scrolling should be stoped when you press second mouse button.

I added mDragFinished property because I can't find way to detect
simultaneous press. AFAIK in dom/events/EventStateManager.cpp#5571 calls
"nsIPresShell::SetCapturingContent(nullptr, 0);" before ::HandleEvent and
as result nsSliderFrame::isDraggingThumb() return "false" after second mouse
button press.

Comment 25

[Markus Stange [:mstange]]

(In reply to Alexander Seleznev from comment #23)
> nsSliderFrame::DragThumb called in "infinity scrolling" case too, but thumb
> shouldn't have :active state.

Oh. OK then, so my suggestion won't work.

Comment 26

[Aleksandr Seleznev]

Attachment: gtk-scrollbar-behavior-part1.patch

1. Fixed event check:

>  +#ifdef MOZ_WIDGET_GTK
>  +  else if (ShouldScrollForEvent(aEvent) &&
> ++           aEvent->mClass == eMouseEventClass &&
>  +           aEvent->AsMouseEvent()->button == WidgetMouseEvent::eRightButton) {
>  +    // HandlePress and HandleRelease are usually called via
>  +    // nsFrame::HandleEvent, but only for the left mouse button.

2. modules/libpref/src/init/all.js -> modules/libpref/init/all.js

Comment 27

[Aleksandr Seleznev]

Attachment: gtk-scrollbar-behavior-part3.patch

Updated (bug 1048178)

Comment 28

[Aleksandr Seleznev]

Attachment: update-tests-v1.patch

~ toolkit/content/tests/chrome/test_scrollbar.xul

Comment 29

[Aleksandr Seleznev]

https://tbpl.mozilla.org/?tree=Try&rev=b0de8f0c8daa
https://tbpl.mozilla.org/?tree=Try&rev=3bf808979b6f

Comment 30

[Aleksandr Seleznev]

Comment on attachment 8470516 [details] [diff] [review]
gtk-scrollbar-behavior-part3.patch

Can you review nsNativeThemeGTK.cpp part too?

Comment 31

[Aleksandr Seleznev]

Comment on attachment 8470517 [details] [diff] [review]
update-tests-v1.patch

And "test_scrollbar.xul" updates. %)

// Also, all looks ready for mozilla-central for me. Or anything still required?

Comment 32

[Markus Stange [:mstange]]

Comment on attachment 8470516 [details] [diff] [review]
gtk-scrollbar-behavior-part3.patch

Review of attachment 8470516 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Alexander Seleznev from comment #30)
> Comment on attachment 8470516 [details] [diff] [review]
> gtk-scrollbar-behavior-part3.patch
> 
> Can you review nsNativeThemeGTK.cpp part too?

Sure, it's simple enough.

::: layout/xul/nsSliderFrame.cpp
@@ +528,5 @@
>      DragThumb(true);
> +
> +#ifdef MOZ_WIDGET_GTK
> +    nsCOMPtr<nsIContent> thumb;
> +    thumb = GetContentOfBox(thumbFrame);

Wow, GetContentOfBox is an impressively useless function. Just make this
nsCOMPtr<nsIContent> thumb = thumbFrame->GetContent();

@@ +888,5 @@
>    }
>  
> +#ifdef MOZ_WIDGET_GTK
> +  nsCOMPtr<nsIContent> thumb;
> +  thumb = GetContentOfBox(thumbFrame);

here too

@@ +916,5 @@
> +#ifdef MOZ_WIDGET_GTK
> +  nsIFrame* thumbFrame = mFrames.FirstChild();
> +  if (thumbFrame) {
> +    nsCOMPtr<nsIContent> thumb;
> +    thumb = GetContentOfBox(thumbFrame);

and here

Comment 33

[Markus Stange [:mstange]]

Comment on attachment 8470517 [details] [diff] [review]
update-tests-v1.patch

Review of attachment 8470517 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!

Comment 34

[Aleksandr Seleznev]

Attachment: gtk-scrollbar-behavior-part3.patch

(In reply to Markus Stange [:mstange] from comment #32)
> Wow, GetContentOfBox is an impressively useless function. Just make this
> nsCOMPtr<nsIContent> thumb = thumbFrame->GetContent();

Done.

Comment 35

[Aleksandr Seleznev]

Also I don't have permissions to add keywords, but I want to add "checkin-needed". Can somebody help me? :(

Comment 36

[Markus Stange [:mstange]]

Sure. see https://bugzilla.mozilla.org/page.cgi?id=get_permissions.html for how to request editbugs

Comment 37

[:Gijs (he/him)]

(In reply to Alexander Seleznev from comment #35)
> Also I don't have permissions to add keywords, but I want to add
> "checkin-needed". Can somebody help me? :(

I've seen several patches from you now, so I just gave you editbugs. Use this power wisely. :-)

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a689547c5fe8
https://hg.mozilla.org/integration/mozilla-inbound/rev/2617c971c55f
https://hg.mozilla.org/integration/mozilla-inbound/rev/68756e9b1f90
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e56b47fbaba

Thanks for the patches, Alexander! One request, we generally prefer that the commit message on the patch explain what each patch is doing, per the guidelines below. Thanks :)
https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Checkin_comment

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a689547c5fe8
https://hg.mozilla.org/mozilla-central/rev/2617c971c55f
https://hg.mozilla.org/mozilla-central/rev/68756e9b1f90
https://hg.mozilla.org/mozilla-central/rev/5e56b47fbaba

Comment 40

[Mike Hommey [:glandium]]

The sad consequence of this change is that this forces the new behavior when using AdWaita (default gnome 3 theme), and there's no easy way to change gtk2 applications behavior, because gtk2 sucks (setting gtk-primary-button-warps-slider = false in .gtkrc-2.0 doesn't work because it reads the theme after that, which contains gtk-primary-button-warps-slider = true). Fortunately, it's still possible to set ui.scrollToClick to 0 in Firefox.

Comment 41

[Karl Tomlinson (:karlt)]

(In reply to Mike Hommey [:glandium] from comment #40)
> The sad consequence of this change is that this forces the new behavior when
> using AdWaita (default gnome 3 theme), and there's no easy way to change
> gtk2 applications behavior,

Perhaps it is possible to set the gtk2 theme to Clearlooks in ~/.gtkrc-2.0, which hopefully looks similar but doesn't have gtk-primary-button-warps-slider set, but I don't know whether or not .gtkrc-2.0 overrides XSettings.

> because gtk2 sucks (setting
> gtk-primary-button-warps-slider = false in .gtkrc-2.0 doesn't work because
> it reads the theme after that, which contains
> gtk-primary-button-warps-slider = true).

That does seem sad.

> Fortunately, it's still possible to set ui.scrollToClick to 0 in Firefox.

Thanks!

Comment 42

[Mike Hommey [:glandium]]

Note, I figured a way that works for all gtk 2.0 applications while keeping Adwaita:

Create a $HOME/.themes/Adwaita/gtk-2.0/gtkrc file with the following content:

include "/usr/share/themes/Adwaita/gtk-2.0/gtkrc"
gtk-primary-button-warps-slider = 0