|
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.
|
||
Updated•5 years ago
|
||
|
|
||
Updated•5 years ago
|
||
|
||
Updated•5 years ago
|
||
|
(Assignee) | |
Comment 1•4 years ago
|
||
Created attachment 825392 [details] [diff] [review] 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.
|
|
(Assignee) | |
Comment 2•4 years ago
|
||
Created attachment 825394 [details] [diff] [review] xulrunner-gtk-scrollbar-behavior.patch 3. Disable Middle mouse button. 4. Enable Right button for scroll page up/down.
|
|
(Assignee) | |
Comment 3•4 years ago
|
||
Created attachment 8382464 [details] [diff] [review] xulrunner-gtk-warps-slider.patch It's just update.
|
|
(Assignee) | |
Comment 4•4 years ago
|
||
Created attachment 8382465 [details] [diff] [review] xulrunner-gtk-scrollbar-behavior.patch
|
|
(Assignee) | |
Comment 6•3 years ago
|
||
Created attachment 8456843 [details] [diff] [review] gtk-scrollbar-behavior.patch Update and merge patches to one.
|
|
(Assignee) | |
Updated•3 years ago
|
||
|
|
||
Comment 7•3 years ago
|
||
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•3 years ago
|
||
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.
|
|
(Assignee) | |
Comment 9•3 years ago
|
||
Created attachment 8462548 [details] [diff] [review] gtk-scrollbar-behavior-part1.patch Main part
|
|
(Assignee) | |
Comment 10•3 years ago
|
||
Created attachment 8462549 [details] [diff] [review] gtk-scrollbar-behavior-part2.patch
|
|
(Assignee) | |
Comment 11•3 years ago
|
||
(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.
|
|
(Assignee) | |
Comment 12•3 years ago
|
||
Created attachment 8462551 [details] right-button-spec.png
|
|
(Assignee) | |
Comment 13•3 years ago
|
||
(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
|
|
(Assignee) | |
Comment 14•3 years ago
|
||
Created attachment 8462978 [details] [diff] [review] 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•3 years ago
|
||
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.
|
|
||
Updated•3 years ago
|
||
|
|
||
Comment 16•3 years ago
|
||
(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•3 years ago
|
||
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•3 years ago
|
||
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•3 years ago
|
||
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?
|
|
(Assignee) | |
Comment 20•3 years ago
|
||
Created attachment 8465267 [details] [diff] [review] gtk-scrollbar-behavior-part1.patch // Added comment.
|
|
(Assignee) | |
Comment 21•3 years ago
|
||
Created attachment 8465268 [details] [diff] [review] gtk-scrollbar-behavior-part2.patch // Added many comments.
|
|
(Assignee) | |
Comment 22•3 years ago
|
||
Created attachment 8465302 [details] [diff] [review] gtk-scrollbar-behavior-part3.patch
|
|
(Assignee) | |
Comment 23•3 years ago
|
||
(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
|
|
(Assignee) | |
Comment 24•3 years ago
|
||
Created attachment 8465309 [details] [diff] [review] 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.
|
|
(Assignee) | |
Updated•3 years ago
|
||
|
|
(Assignee) | |
Updated•3 years ago
|
||
|
|
||
Comment 25•3 years ago
|
||
(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.
|
|
(Assignee) | |
Comment 26•3 years ago
|
||
Created attachment 8470515 [details] [diff] [review] 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
|
|
(Assignee) | |
Comment 27•3 years ago
|
||
Created attachment 8470516 [details] [diff] [review] gtk-scrollbar-behavior-part3.patch Updated (bug 1048178)
|
|
(Assignee) | |
Comment 28•3 years ago
|
||
Created attachment 8470517 [details] [diff] [review] update-tests-v1.patch ~ toolkit/content/tests/chrome/test_scrollbar.xul
|
|
(Assignee) | |
Comment 29•3 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=b0de8f0c8daa https://tbpl.mozilla.org/?tree=Try&rev=3bf808979b6f
|
|
(Assignee) | |
Comment 30•3 years ago
|
||
Comment on attachment 8470516 [details] [diff] [review] gtk-scrollbar-behavior-part3.patch Can you review nsNativeThemeGTK.cpp part too?
|
|
(Assignee) | |
Comment 31•3 years ago
|
||
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•3 years ago
|
||
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•3 years ago
|
||
Comment on attachment 8470517 [details] [diff] [review] update-tests-v1.patch Review of attachment 8470517 [details] [diff] [review]: ----------------------------------------------------------------- Nice!
|
|
(Assignee) | |
Comment 34•3 years ago
|
||
Created attachment 8472316 [details] [diff] [review] 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.
|
|
(Assignee) | |
Comment 35•3 years ago
|
||
Also I don't have permissions to add keywords, but I want to add "checkin-needed". Can somebody help me? :(
|
|
||
Comment 36•3 years ago
|
||
Sure. see https://bugzilla.mozilla.org/page.cgi?id=get_permissions.html for how to request editbugs
|
||
Comment 37•3 years ago
|
||
(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•3 years ago
|
||
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•3 years ago
|
||
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•3 years ago
|
||
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•3 years ago
|
||
(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•3 years ago
|
||
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
Description
•