Over scrolling when mouse down on scrollbar if smooth scroll is enabled
Categories
(Core :: Layout, defect, P3)
Tracking
()
People
(Reporter: alice0775, Assigned: rzvncj)
References
Details
(4 keywords)
Attachments
(1 file)
(deleted),
text/x-phabricator-request
|
Details |
Comment 1•8 years ago
|
||
Comment 2•8 years ago
|
||
Comment 3•8 years ago
|
||
Comment 4•8 years ago
|
||
Reporter | ||
Updated•8 years ago
|
Comment 5•8 years ago
|
||
Comment 6•8 years ago
|
||
Updated•7 years ago
|
Comment 10•7 years ago
|
||
Updated•7 years ago
|
Reporter | ||
Updated•4 years ago
|
Comment 11•3 years ago
|
||
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•3 years ago
|
||
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?)
Reporter | ||
Comment 13•3 years ago
|
||
This is of particular concern after the page has finished loading and the CPU/HDD has settled down.
Steps To Reproduce:
- Open long page (e.g., https://ftp.mozilla.org/pub/firefox/nightly/2022/01/2022-01-01-09-53-22-mozilla-central/ )
- Wait for page load
- Mouse down and keep on the empty area of vertical scrollbar and do not move mouse pointer
- wait to stop scroll
Actual results:
Scroll thumb goes too far past the mouse pointer position
Chrome works as expected.
Updated•2 years ago
|
Assignee | ||
Comment 14•2 years ago
|
||
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.
Assignee | ||
Comment 15•2 years ago
|
||
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.
Assignee | ||
Comment 16•2 years ago
|
||
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•2 years ago
|
||
(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 byabs(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.
Assignee | ||
Comment 18•2 years ago
|
||
(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 byabs(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, butnsIScrollbarMediator::ScrollByUnit()
withaUnit = 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•2 years ago
|
||
(In reply to Razvan Cojocaru from comment #18)
I take it that
mDestinationPoint
is not a device pixels quantity (it's the untypednsPoint
)? 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.
Assignee | ||
Comment 20•2 years ago
|
||
Updated•2 years ago
|
Updated•1 year ago
|
Comment 21•1 year ago
|
||
Comment 22•1 year ago
|
||
bugherder |
Comment 23•1 year ago
|
||
Thank you Razvan for fixing this long-standing issue!
Assignee | ||
Comment 24•1 year ago
|
||
(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!
Updated•1 year ago
|
Description
•