1331390 - Over scrolling when mouse down on scrollbar if smooth scroll is enabled

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Alice0775 White]

Build Identifier:
https://hg.mozilla.org/mozilla-central/rev/5ce3882eec21be3a70e4afc050959ca2f76bfa76
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:53.0) Gecko/20100101 Firefox/53.0 ID:20170115030210

The problem is noticeable on long page. 
Over scrolling when mouse down on scrollbar. It is not intuitive behavior.
Disabled smooth scroll fix the problem.

Reproducible: always

Steps To Reproduce:
1. Open long page (e.g., https://developer.mozilla.org/en-US/docs/Web/Events )
2. Mouse down and keep on the empty area of vertical scrollbar and do not move mouse pointer
3. wait to stop scroll
   --- observe page scroll position, and thumb position
4. Mouse up and click (also do not move mouse pointer)
   --- observe page scroll position, and thumb position

Actual Results:
Page is over scrolling.
The thumb is overshoot beyond position the mouse pointer.
After Step4, scroll position of page is going back.

Expected Results:
Page should not be over scrolling. (Edge and Chrome does).
After Step4, scroll position of page should not be going back.

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ec4622e6e1fa&tochange=4ef03db1385b

Regressed by: Bug 957445

Comment 1

[Mats Palmgren (inactive)]

Maybe the same underlying issue as in bug 1328053?

Comment 2

[:kip (Kearwood Gilbert)]

Indeed, this may be the same root cause.  I am adding a patch to Bug 1328053, which may fix this as well.  Once Bug 1328053 lands, please re-test to see if it is fully fixed.

Comment 3

[:kip (Kearwood Gilbert)]

I have pushed a series of try builds out for the proposed fix to Bug 1328053.  Hopefully we can use these builds to verify that it fixes this bug also.

Comment 4

[:kip (Kearwood Gilbert)]

I've reproduced this (Windows 10, FF Nightly).  It appears the fix to Bug 1328053 does not correct it.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Still reproducible, but I think this is edge-case enough that I'm calling it fix-optional for 52. That said, we still have time to take a low-risk fix for 52 if you have more time to look into this, Kip :)

Comment 6

[:kip (Kearwood Gilbert)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #5)
> Still reproducible, but I think this is edge-case enough that I'm calling it
> fix-optional for 52. That said, we still have time to take a low-risk fix
> for 52 if you have more time to look into this, Kip :)

I won't likely have time to investigate this one before the trains run, but would be glad to take a look for the following release.

I'll take the bug for now, but please feel free to steal this from me if anyone wants to get it in for 52.

Comment 7

[Liz Henry (:lizzard) (relman/hg->git project)]

Too late for a fix for 53, fix-optional for 54, minor carryover regression.

Comment 10

[Jonathan Watt [:jwatt]]

Mass bug change to replace various 'parity' whiteboard flags with the new canonical keywords. (See bug 1443764 comment 13.)

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:dholbert, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 12

[Daniel Holbert [:dholbert]]

Probably the same as bug 1686414 (though this bug here was filed a while before). Looks like we've got a screencast of the issue over there, and a suggested place-to-start-debugging in bug 1686414 comment 2.

I can reproduce using Firefox 98 on Win10 (tested remotely in BrowserStack since I'm natively using Linux), if I click (and hold) in the scroll-track while the page is still loading. If I wait until the page has finished loading and things are stable, I can't reproduce.

Alice0775 White, I wonder if that matches your experience, too? (Do you only see this if you start scrolling during pageload, or when your system is under load or other identifiable conditions?)

Comment 13

[Alice0775 White]

This is of particular concern after the page has finished loading and the CPU/HDD has settled down.

Steps To Reproduce:

1. Open long page (e.g., https://ftp.mozilla.org/pub/firefox/nightly/2022/01/2022-01-01-09-53-22-mozilla-central/ )
2. Wait for page load
3. Mouse down and keep on the empty area of vertical scrollbar and do not move mouse pointer
4. wait to stop scroll

Actual results:
Scroll thumb goes too far past the mouse pointer position

Chrome works as expected.

Comment 14

[Razvan Cojocaru]

Taking a quick glance at the code in question (as discussed with Botond on Matrix), here is where the computation happens:

  // See if the thumb has moved past our destination point.
  // if it has we want to stop.
  if (isHorizontal) {
    if (mChange < 0) {
      if (thumbRect.x < mDestinationPoint.x) stop = true;
    } else {
      if (thumbRect.x + thumbRect.width > mDestinationPoint.x) stop = true;
    }
  } else {
    if (mChange < 0) {
      if (thumbRect.y < mDestinationPoint.y) stop = true;
    } else {
      if (thumbRect.y + thumbRect.height > mDestinationPoint.y) stop = true;
    }
  }

If stop is not true, the function calls PageScroll(mChange);, which does this:

void nsSliderFrame::PageScroll(nscoord aChange) {
  if (mContent->AsElement()->AttrValueIs(kNameSpaceID_None, nsGkAtoms::dir,
                                         nsGkAtoms::reverse, eCaseMatters)) {
    aChange = -aChange;
  }
  nsIFrame* scrollbar = GetScrollbar();
  nsScrollbarFrame* sb = do_QueryFrame(scrollbar);
  if (sb) {
    nsIScrollbarMediator* m = sb->GetScrollbarMediator();
    sb->SetIncrementToPage(aChange);
    if (m) {
      m->ScrollByPage(sb, aChange,
                      ScrollSnapFlags::IntendedDirection |
                          ScrollSnapFlags::IntendedEndPosition);
      return;
    }
  }
  PageUpDown(aChange);
}

Specifically, we care about the m->ScrollByPage() call, which appears to be set up with mScrollUnit = ScrollUnit::PAGES;.

So, with the current logic it looks like it's highly unlikely that the thumb would ever land nicely under the cursor, since it would always be on a page boundary.

Comment 15

[Razvan Cojocaru]

I should probably note (for the benefit of people other than Botond and myself) that, as suggested on Matrix, I'm testing on a Linux system with this #ifdef suitably modified, and by keeping Shift pressed while also keeping the left mouse button pressed. It may also matter that with this setup it doesn't matter if smooth scrolling is enabled or not, the behaviour is the same.

Comment 16

[Razvan Cojocaru]

Unless I've misdiagnosed this, it would appear, then, that the solution might be something along the lines of: scroll by one page at each step only if current thumb position + page length < mDestinationPoint, otherwise scroll by abs(current thumb position - mDestinationPoint)?

Comment 17

[Botond Ballo [:botond]]

(In reply to Razvan Cojocaru from comment #16)

Unless I've misdiagnosed this, it would appear, then, that the solution might be something along the lines of: scroll by one page at each step only if current thumb position + page length < mDestinationPoint, otherwise scroll by abs(current thumb position - mDestinationPoint)?

Yup, that sounds reasonable.

I'm not deeply familiar with the various abstractions like nsIScrollbarMediator here so I don't have a confident suggestion for how to implement the latter case, but nsIScrollbarMediator::ScrollByUnit() with aUnit = DEVICE_PIXELS is worth trying.

Comment 18

[Razvan Cojocaru]

(In reply to Botond Ballo [:botond] from comment #17)

(In reply to Razvan Cojocaru from comment #16)

Unless I've misdiagnosed this, it would appear, then, that the solution might be something along the lines of: scroll by one page at each step only if current thumb position + page length < mDestinationPoint, otherwise scroll by abs(current thumb position - mDestinationPoint)?

Yup, that sounds reasonable.

I'm not deeply familiar with the various abstractions like nsIScrollbarMediator here so I don't have a confident suggestion for how to implement the latter case, but nsIScrollbarMediator::ScrollByUnit() with aUnit = DEVICE_PIXELS is worth trying.

I take it that mDestinationPoint is not a device pixels quantity (it's the untyped nsPoint)? Should that be converted to something clearer (CSSPoint, etc.)?

Comment 19

[Botond Ballo [:botond]]

(In reply to Razvan Cojocaru from comment #18)

I take it that mDestinationPoint is not a device pixels quantity (it's the untyped nsPoint)? Should that be converted to something clearer (CSSPoint, etc.)?

We use nsPoint to represent quantities in app units. The conversion ratio to device pixels is PresContext()->AppUnitsPerDevPixel().

I also realized that the ScrollByUnit() API is a bit unusual in that it doesn't actually take an aDelta parameter; however, given the implementation here, the aDirection parameter can be used as a delta representing an integer number of device pixels.

Comment 20

[Razvan Cojocaru]

Attachment: Bug 1331390 - Don't over scroll when mouse down on scrollbar if smooth scroll is enabled. r?botond

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/22f915e74381
Don't over scroll when mouse down on scrollbar if smooth scroll is enabled. r=botond

Comment 22

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/22f915e74381

Comment 23

[Botond Ballo [:botond]]

Thank you Razvan for fixing this long-standing issue!

Comment 24

[Razvan Cojocaru]

(In reply to Botond Ballo [:botond] from comment #23)

Thank you Razvan for fixing this long-standing issue!

Happy to help, thanks for the reviews!