Simonas Kazlauskas [:nagisa/:simukis] Reporter
	Description • 9 years ago

As mentioned in https://help.gnome.org/misc/release-notes/3.18/more.html.en#scrolling behaviour when clicking on scrollbar has changed and Firefox seems to still have the old behaviour.

Namely:

1. Middle-click for auto scroll does not work;
2. Holding down the handle does not enter into precision scrolling mode (introduced in 3.16, I believe)

	Simonas Kazlauskas [:nagisa/:simukis] Reporter
	Comment 1 • 9 years ago

Shift+click seems to properly scroll a single page, though.

	Karl Tomlinson (:karlt)
	Comment 3 • 7 years ago
mozreview-review

Comment on attachment 8885289 [details]
Bug 1213609 - Adding support for GTK's autoscroll;

https://reviewboard.mozilla.org/r/156160/#review163970

This reminds me of scrolling during selection, where scroll speed depends on pointer speed.
I wonder where that is implemented and whether it is worth checking for ideas.

GTK 3.18 is old enough that I agree it is OK to have this behavior even for
remaining older versions.

::: layout/generic/nsFrame.cpp:3358
(Diff revision 1)
> -       aEvent->AsMouseEvent()->button == WidgetMouseEvent::eLeftButton) ||
> +       (aEvent->AsMouseEvent()->button == WidgetMouseEvent::eLeftButton ||
> +       acceptRightButton &&
> +       aEvent->AsMouseEvent()->button == WidgetMouseEvent::eRightButton) ) ||

I'm not clear why this is required, because nsSliderFrame::HandleEvent() appears to call HandlePress() when necessary to initiate and StopDrag() to stop.  Is this about nsIScrollbarMediator::ScrollbarReleased()?

Is don't know why nsFrame has these special cases, and so I think we'd need a
layout peer to review this change.

Is it necessary to modify the base class method here?

Could this be handled in nsSliderFrame::HandleEvent() instead?

::: layout/xul/nsRepeatService.h:20
(Diff revision 1)
> -#define REPEAT_DELAY        50
> +# ifdef XP_LINUX
> +     // From gtkrange.c - TIMEOUT_REPEAT
> +#    define REPEAT_DELAY        250

Looks like this will increase the repeat delay for
HTMLInputElement::StartNumberControlSpinnerSpin() also, but gtkspinbutton.c
uses 50 for its TIMEOUT_REPEAT.

I don't know what is appropriate for nsAutoRepeatBoxFrame/autorepeatbutton and
the nsScrollbarButtonFrame.  Nor do I don't know what autorepeatbutton is used
for, but having a repeat delay 5x that of winnt could be a problem if this is
not scrolling by pages.  Have you looked at this?

Looks like GTK scrollbar buttons autoscroll now, similar to the trough, but
different.  That's a separate issue and not required to support autoscroll in
the trough, but changing the delay for buttons may again not be desirable if
scrolling by less than pages.  Have you looked at this?

I wonder whether limiting changes in delay to nsSliderFrame may be safer, if
practical.

::: layout/xul/nsRepeatService.h:21
(Diff revision 1)
> +     // From gtkrange.c - TIMEOUT_REPEAT
> +#    define REPEAT_DELAY        250
> +     // From gtkrange.c - TIMEOUT_INITIAL
> +#    define INITAL_REPEAT_DELAY 500

nsRepeatService does what I'd expect with REPEAT_DELAY and
INITAL_REPEAT_DELAY: It schedules the first callback after INITAL_REPEAT_DELAY
and subsequent callbacks after subsequent REPEAT_DELAY intervals.

gtkrange.c (version 3.22.15 at least) is different, however.
gtk_range_scroll() is not scheduled immediately after TIMEOUT_INITIAL but only
after subsequent TIMEOUT_REPEAT intervals.

INITAL_REPEAT_DELAY corresponds to TIMEOUT_INITIAL + TIMEOUT_REPEAT.

However, GTK's initial delay seems very long to me.  If you'd to keep the constants here, then I think that would be fine, as long as the comments indicate the difference from gtkrange.c.

::: layout/xul/nsRepeatService.h:57
(Diff revision 1)
> +             uint32_t aDelay = REPEAT_DELAY);
>    // Stop dispatching timer events to the callback. If the repeat service
>    // is not currently configured with the given callback and data, this
>    // is just ignored.
>    void Stop(Callback aCallback, void* aData);
> +  uint32_t GetDelay();

Keep this private, as it is not used elsewhere, or just access mDelay directly.

::: layout/xul/nsRepeatService.h:73
(Diff revision 1)
>  
>    Callback           mCallback;
>    void*              mCallbackData;
>    nsCString          mCallbackName;
>    nsCOMPtr<nsITimer> mRepeatTimer;
> +  unsigned long      mDelay;

This is set from and used for uint32_t, and so store as uint32_t here.

::: layout/xul/nsSliderFrame.h:189
(Diff revision 1)
>    }
> +  void AutoScroll(nscoord aChange);
>    void PageScroll(nscoord aChange);
>  
>    nsPoint mDestinationPoint;
> +  nsPoint mStartPoint;

#ifdef MOZ_WIDGET_GTK, to avoid warnings on other platforms.

::: layout/xul/nsSliderFrame.h:190
(Diff revision 1)
> +  void AutoScroll(nscoord aChange);
>    void PageScroll(nscoord aChange);
>  
>    nsPoint mDestinationPoint;
> +  nsPoint mStartPoint;
> +  nscoord mIncrement;

Pack this with other members of nscoord type.

::: layout/xul/nsSliderFrame.h:224
(Diff revision 1)
> +  // determine scroll unit in Notify method
> +  nsIScrollableFrame::ScrollUnit mScrollUnit;

Please move this to with the other instance member variables, keeping them separate from, the class variables at the end.

::: layout/xul/nsSliderFrame.cpp:617
(Diff revision 1)
> +#else
> +        // For GTK 3.18+ we need to compute mIncrement to implement autoscroll.
> +        // The distance of the point where the mouse has been clicked determines
> +        // autoscroll speed.
> +        // Only when we're autoscrolling:
> +        if (mRepeating && mScrollUnit == nsIScrollableFrame::DEVICE_PIXELS) {

Is the mRepeating check here (and variable) necessary?

I'm guessing the existing isDraggingThumb() check is close enough to equivalent?

::: layout/xul/nsSliderFrame.cpp:625
(Diff revision 1)
> +          if (isHorizontal) {
> +            delta = (eventPoint.y - mStartPoint.y) / appUnitsPerPixel;
> +          } else {
> +            delta = (eventPoint.x - mStartPoint.x) / appUnitsPerPixel;
> +          }
> +          if (delta > 20 || delta < -20) {

These numbers should be GTK pixels rather than device pixels, but CSS pixels at default zoom would be OK I think.

::: layout/xul/nsSliderFrame.cpp:635
(Diff revision 1)
> +            if (nsIScrollableFrame* scrollFrame = do_QueryFrame(scrollbarBox->GetParent())) {
> +              float t = mIncrement / 100.0;
> +              nsSize lineScroll = scrollFrame->GetLineScrollAmount() / appUnitsPerPixel;
> +              nsSize pageScroll = scrollFrame->GetPageScrollAmount() / appUnitsPerPixel;

I don't see any other code in the file looking for a nsIScrollableFrame.  I don't know whether that always exists.  What if it doesn't?

Can this use GetPageIncrement(), like the rest of the file?

::: layout/xul/nsSliderFrame.cpp:640
(Diff revision 1)
> +            if (nsIScrollableFrame* scrollFrame = do_QueryFrame(scrollbarBox->GetParent())) {
> +              float t = mIncrement / 100.0;
> +              nsSize lineScroll = scrollFrame->GetLineScrollAmount() / appUnitsPerPixel;
> +              nsSize pageScroll = scrollFrame->GetPageScrollAmount() / appUnitsPerPixel;
> +              mIncrement = (1 - t) * lineScroll.height + t * pageScroll.height;
> +              mIncrement /= 20; // This is defined in gtkrange.c as AUTOSCROLL_FACTOR.

If lineScroll.height can be < 20, then mIncrement may be zero.  If fractional increments are not going to work, then some other mechanism will be required.

::: layout/xul/nsSliderFrame.cpp:642
(Diff revision 1)
> +          } else {
> +            mIncrement = 1;

I don't see this discontinuity at 20 in autoscroll_cb in gtkrange.c.  GTK clamps to 20 and uses the same path as above.

::: layout/xul/nsSliderFrame.cpp:834
(Diff revision 1)
> +    case nsIScrollableFrame::DEVICE_PIXELS:
> +      increment = mIncrement;
> +      break;
> +    case nsIScrollableFrame::PAGES:
> +      increment = GetPageIncrement(scrollbar);
> +      break;

ScrollUpDown() is called only from PageScroll() and AutoScroll(), and so those
functions can factor in the appropriate multiplier for a single |int32_t change| parameter.
That would keep the logic in one place for each the different scrolls.

::: layout/xul/nsSliderFrame.cpp:1518
(Diff revision 1)
> +    // The GTK use 60Hz refresh which is approximatelly 17ms
> +    repeatRate = 17;
> +    AutoScroll(change);

Ideally this would use the refresh driver, instead of the repeat service, but I don't know exactly how to do that.

step-increment from gtk_text_view_set_hadjustment_values() is one tenth of a page, and AUTOSCROLL_FACTOR is 20 in gtkrange.c.
It seems a page-fraction-based scroll would be more consistent with GTK.

That still permits rates of less than one CSS pixel per frame, and so some accumulator of fractions may be required.

::: layout/xul/nsSliderFrame.cpp:1655
(Diff revision 1)
> +{
> +  if (mContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::dir,
> +                            nsGkAtoms::reverse, eCaseMatters)) {
> +    aChange = -aChange;
> +  }
> +  mScrollUnit = nsIScrollableFrame::DEVICE_PIXELS;

SetCurrentPositionInternal() assumes CSS pixels, and so calling this device pixels is confusing.
Perhaps a local AUTO/PAGE enum may be more appropriate.

The rate of change measured in device pixels should be independent of page zoom, but dependent on the number of device pixels per CSS pixel at unit zoom.
Using tenths of pages for minumum auto scroll would provide that.

	Jan Horak [:jhorak]
	Comment 4 • 7 years ago
mozreview-review-reply

Comment on attachment 8885289 [details]
Bug 1213609 - Adding support for GTK's autoscroll;

https://reviewboard.mozilla.org/r/156160/#review163970

> I'm not clear why this is required, because nsSliderFrame::HandleEvent() appears to call HandlePress() when necessary to initiate and StopDrag() to stop.  Is this about nsIScrollbarMediator::ScrollbarReleased()?
> 
> Is don't know why nsFrame has these special cases, and so I think we'd need a
> layout peer to review this change.
> 
> Is it necessary to modify the base class method here?
> 
> Could this be handled in nsSliderFrame::HandleEvent() instead?

This indeed is not required to change.

> Looks like this will increase the repeat delay for
> HTMLInputElement::StartNumberControlSpinnerSpin() also, but gtkspinbutton.c
> uses 50 for its TIMEOUT_REPEAT.
> 
> I don't know what is appropriate for nsAutoRepeatBoxFrame/autorepeatbutton and
> the nsScrollbarButtonFrame.  Nor do I don't know what autorepeatbutton is used
> for, but having a repeat delay 5x that of winnt could be a problem if this is
> not scrolling by pages.  Have you looked at this?
> 
> Looks like GTK scrollbar buttons autoscroll now, similar to the trough, but
> different.  That's a separate issue and not required to support autoscroll in
> the trough, but changing the delay for buttons may again not be desirable if
> scrolling by less than pages.  Have you looked at this?
> 
> I wonder whether limiting changes in delay to nsSliderFrame may be safer, if
> practical.

It seems right to not touch this ATM. It should not be required for this bug.

> nsRepeatService does what I'd expect with REPEAT_DELAY and
> INITAL_REPEAT_DELAY: It schedules the first callback after INITAL_REPEAT_DELAY
> and subsequent callbacks after subsequent REPEAT_DELAY intervals.
> 
> gtkrange.c (version 3.22.15 at least) is different, however.
> gtk_range_scroll() is not scheduled immediately after TIMEOUT_INITIAL but only
> after subsequent TIMEOUT_REPEAT intervals.
> 
> INITAL_REPEAT_DELAY corresponds to TIMEOUT_INITIAL + TIMEOUT_REPEAT.
> 
> However, GTK's initial delay seems very long to me.  If you'd to keep the constants here, then I think that would be fine, as long as the comments indicate the difference from gtkrange.c.

Hm, good catch actually. I would leave it to another bug.

> Keep this private, as it is not used elsewhere, or just access mDelay directly.

Hm, I'm unable to build with using private mDelay, because of the lambda function of where it is used:
 0:13.68 /home/jhorak/m/m-c/layout/xul/nsRepeatService.cpp:103:27: error: ‘this’ was not captured for this lambda function
 0:13.68      rs->InitTimerCallback(mDelay);
 0:13.68                            ^~~~~~
 0:13.68 /home/jhorak/m/m-c/layout/xul/nsRepeatService.cpp:103:27: error: invalid use of non-static data member ‘nsRepeatService::mDelay’

> Is the mRepeating check here (and variable) necessary?
> 
> I'm guessing the existing isDraggingThumb() check is close enough to equivalent?

Yes, not necessary.

> These numbers should be GTK pixels rather than device pixels, but CSS pixels at default zoom would be OK I think.

Yes, in GTK the value is scaled according to scaling, so on HiDPI 2x it's actually is 40 pixels. Is using the AppUnitsPerDevPixel() correct there?

> I don't see any other code in the file looking for a nsIScrollableFrame.  I don't know whether that always exists.  What if it doesn't?
> 
> Can this use GetPageIncrement(), like the rest of the file?

GetPageIncrement() can be used, but I can't find how to replace GetLineScrollAmount.

> If lineScroll.height can be < 20, then mIncrement may be zero.  If fractional increments are not going to work, then some other mechanism will be required.

I'm addressing the fractal part by using mIncrementLeftover now.

> I don't see this discontinuity at 20 in autoscroll_cb in gtkrange.c.  GTK clamps to 20 and uses the same path as above.

Yes, you're right, I've followed the logic from gtkrange comments.

> ScrollUpDown() is called only from PageScroll() and AutoScroll(), and so those
> functions can factor in the appropriate multiplier for a single |int32_t change| parameter.
> That would keep the logic in one place for each the different scrolls.

I hope that's what you meant (in the next patch).

> Ideally this would use the refresh driver, instead of the repeat service, but I don't know exactly how to do that.
> 
> step-increment from gtk_text_view_set_hadjustment_values() is one tenth of a page, and AUTOSCROLL_FACTOR is 20 in gtkrange.c.
> It seems a page-fraction-based scroll would be more consistent with GTK.
> 
> That still permits rates of less than one CSS pixel per frame, and so some accumulator of fractions may be required.

I hope that will be fixed by introducing mIncrementLeftover

	Jan Horak [:jhorak]
	Comment 6 • 7 years ago

Need to fix the failing tests first. I also screw up the commit to the review board. So until that is fixed, this does not need your attention.

	Firefox Bug Husbandry Bot
	Comment 7 • 6 years ago

Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336

	Sylvestre Ledru [:Sylvestre]
	Comment 8 • 6 years ago

Moving to p3 because no activity for at least 24 weeks.