[Linux gtk] Honor scrollbar preference of system

RESOLVED FIXED in mozilla34

Status

()

defect
RESOLVED FIXED
7 years ago
3 years ago

People

(Reporter: notting, Assigned: seleznev.ru)

Tracking

(Depends on 2 bugs)

Trunk
mozilla34
All
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(5 attachments, 12 obsolete attachments)

36.37 KB, image/png
Details
2.38 KB, patch
Details | Diff | Splinter Review
6.22 KB, patch
Details | Diff | Splinter Review
3.09 KB, patch
mstange
: review+
Details | Diff | Splinter Review
4.15 KB, patch
Details | Diff | Splinter Review
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.
Component: Untriaged → Widget: Gtk
Product: Firefox → Core
Hardware: x86_64 → All
Version: 16 Branch → Trunk
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
3. Disable Middle mouse button.
4. Enable Right button for scroll page up/down.
It's just update.
Attachment #825392 - Attachment is obsolete: true
Attachment #825394 - Attachment is obsolete: true
Duplicate of this bug: 939284
Posted patch gtk-scrollbar-behavior.patch (obsolete) — Splinter Review
Update and merge patches to one.
Attachment #8382464 - Attachment is obsolete: true
Attachment #8382465 - Attachment is obsolete: true
Attachment #8456843 - Flags: review?(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().
Attachment #8456843 - Flags: review?(karlt)
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.
Main part
Attachment #8456843 - Attachment is obsolete: true
(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.
(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
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 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.
Attachment #8462548 - Flags: review?(mstange)
Attachment #8462548 - Flags: review+
(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 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.
Attachment #8462548 - Flags: review?(mstange) → review+
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.
Attachment #8462549 - Flags: review+
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?
// Added comment.
Attachment #8462548 - Attachment is obsolete: true
// Added many comments.
Attachment #8462549 - Attachment is obsolete: true
Attachment #8462978 - Attachment is obsolete: true
(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
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.
Attachment #8465309 - Flags: review?(mstange)
Attachment #8465309 - Flags: review?(mstange)
Attachment #8465309 - Attachment is obsolete: true
(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.
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
Attachment #8465267 - Attachment is obsolete: true
Updated (bug 1048178)
Attachment #8465302 - Attachment is obsolete: true
~ toolkit/content/tests/chrome/test_scrollbar.xul
Comment on attachment 8470516 [details] [diff] [review]
gtk-scrollbar-behavior-part3.patch

Can you review nsNativeThemeGTK.cpp part too?
Attachment #8470516 - Flags: review?(mstange)
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?
Attachment #8470517 - Flags: review?(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
Attachment #8470516 - Flags: review?(mstange) → review+
Comment on attachment 8470517 [details] [diff] [review]
update-tests-v1.patch

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

Nice!
Attachment #8470517 - Flags: review?(mstange) → review+
(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.
Attachment #8470516 - Attachment is obsolete: true
Also I don't have permissions to add keywords, but I want to add "checkin-needed". Can somebody help me? :(
(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. :-)
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.
(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!
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
Depends on: 1134578
Depends on: 1223391
Depends on: 1197590
You need to log in before you can comment on or make changes to this bug.