Closed Bug 1328053 Opened 3 years ago Closed 3 years ago

Scrollbar thumb doesn't follow mouse during Shift+Click scrolling

Categories

(Core :: Layout, defect)

34 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla54
Tracking Status
firefox-esr45 --- wontfix
firefox51 --- wontfix
firefox52 --- verified
firefox53 --- verified
firefox54 --- verified

People

(Reporter: arni2033, Assigned: kip)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file)

>>>   My Info:   Win7_64, Nightly 49, 32bit, ID 20160526082509
STR_1:
0. Create 20 profiles (see http://mzl.la/1BAQGnB)
1. Launch Firefox with flag -P, hover mouse over empty area in scrollbar (not scrollbar thumb)
2. Hold Shift, then hold Left mouse button, then move mouse vertically

AR:  Scrollbar thumb isn't placed directly under mouse pointer. There's ~15px indent between them
ER:  The center of scrollbar thumb should precisely follow the mouse pointer


This is regression from bug 957445 (most likely). Regression range:
> https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=daa84204a11a&tochange=dc352a7bf234@ :kip (Kearwood Gilbert):
It seems that this is a regresion caused by your change. Please have a look.
No longer blocks: 1277113
Component: Untriaged → Layout
Product: Firefox → Core
Step 2 in the scrollbar background works fine for me on Linux (it keeps scrolling
to the start/end depending on which side of the thumb I click on, which is
the expected behavior on Linux.)

I'm guessing the suggested ER is Windows/OSX-specific behavior.

Kip, can you have a look please?
Flags: needinfo?(kgilbert)
>>>   My Info:   Win7_64, Nightly 53, 32bit, ID 20170107030205 (2017-01-07)
Reproducible.
Pay attention: the comment 0 do not mention "click".
Step 2 says "Hold left mouse button", and never says "Release left mouse button". So, no click.
I have managed to reproduce on Windows 10 Home / 64-bit.

The offset seems to be determined by the position of the cursor at mouse-down.
With the same build, I was only able to reproduce this on the profile selection scroll bar.  Chrome UI scroll bars (ie. browsing history) and content scroll bars did not appear to have this effect.

I am looking through the patchset to determine what could have caused the regression there.
This problem appears to be due to SetCurrentThumbPosition being asynchronous, so we can't use thumbFrame->GetPosition() immediately afterwards.

In the case of scrollToClick, we always wish to drag the thumb from the center, so we can simply use the position passed to SetCurrentThumbPosition for nsSliderFrame::mThumbStart.
Flags: needinfo?(kgilbert)
Please note that this may also be the root cause for Bug 1331390.  I've added a comment and dependency there to re-test it once the patch for this bug lands.
I am running my proposed fix through try as a sanity check, if it passes all tests, then will assign for a review.
Assignee: nobody → kgilbert
The tests passed successfully.

Although this patch fixed the original issue in the profile selection box, I notice that it would affect scrolling behavior slightly in the browser itself.

In particular:

- Shift-clicking on a scroll bar thumb would cause it to jump to be centered under the mouse position.
- Shift-clicking near the extents of the browser scroll will not cause overscroll; however, the cursor must move past the center position of the scroll bar thumb before the subsequent shift-click-drag will move the thumb.

If these changes are acceptable / desirable, then this patch will fix the issue.  Otherwise, I suggest finding a way to make SetCurrentThumbPosition()'s effect visible immediately with GetPosition().
I have found the source of the regression in the Bug 957445 patchset:

During the refactoring of nsSliderFrame::SetCurrentPositionInternal in Bug 957445, one of the two calls to nsSliderFrame::UpdateAttribute was missed.  This resulted in the position, identified with nsGkAtoms::curpos, not being updated immediately after the call to nsSliderFrame::SetCurrentThumbPosition

I am re-added this call to nsSliderFrame::UpdateAttribute() in the updated patch, and have pushed to try to ensure that it doesn't trigger any failures in the smooth scrolling tests.  This patch corrects the issue without the side effects described in Comment 9.
Attachment #8829703 - Flags: review?(mats)
The try run looks good.  I believe this 1-line fix should correct this issue, and potentially others that were regressed.
Comment on attachment 8829703 [details]
Bug 1328053 - Correct thumb position when shift-click scrolling

https://reviewboard.mozilla.org/r/106714/#review108406

::: layout/xul/nsSliderFrame.cpp:889
(Diff revision 2)
>        nscoord newPos = nsPresContext::CSSPixelsToAppUnits(aNewPos);
>        mediator->ThumbMoved(scrollbarFrame, oldPos, newPos);
>        if (!weakFrame.IsAlive()) {
>          return;
>        }
> +      UpdateAttribute(scrollbar, aNewPos, false, aIsSmooth);

Might be worth changing "false" to "/*aNotify*/ false" just as a reminder to the reader that this particular UpdateAttribute call is safe and doesn't need a weakFrame.IsAlive() check after it.
Attachment #8829703 - Flags: review?(mats) → review+
Pushed by kgilbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b30c5aed91fb
Correct thumb position when shift-click scrolling r=mats
https://hg.mozilla.org/mozilla-central/rev/b30c5aed91fb
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Looks like a good candidate for backport to 52 and 53. Please fill out the requests :)
Flags: needinfo?(kgilbert)
Version: Trunk → 34 Branch
Comment on attachment 8829703 [details]
Bug 1328053 - Correct thumb position when shift-click scrolling

Approval Request Comment
[Feature/Bug causing the regression]: Bug 957445 (Fix the way scrollbars communicate with nsHTML/XULScrollFrame)
[User impact if declined]: If declined, shift-clicking between the scrollbar thumb and the arrows can result in a different scroll position than expected.
[Is this code covered by automated tests?]: Automated tests execute the code, but do not test for expected scrollbar position when shift-clicking.
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: See User Story in Bug 1328053 for steps to reproduce in Windows:

0. Create 20 profiles (see http://mzl.la/1BAQGnB)
1. Launch Firefox with flag -P, hover mouse over empty area in scrollbar (not scrollbar thumb)
2. Hold Shift, then hold Left mouse button, then move mouse vertically

AR:  Scrollbar thumb isn't placed directly under mouse pointer. There's ~15px indent between them
ER:  The center of scrollbar thumb should precisely follow the mouse pointer

[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Not risky as the change is a single line and potential regressions to non-shift-click scrolling and smooth scrolling are covered by tests.
[String changes made/needed]: None
Flags: needinfo?(kgilbert)
Attachment #8829703 - Flags: approval-mozilla-beta?
Attachment #8829703 - Flags: approval-mozilla-aurora?
Comment on attachment 8829703 [details]
Bug 1328053 - Correct thumb position when shift-click scrolling

simple fix for scrolling regression, aurora53+, beta52+
Attachment #8829703 - Flags: approval-mozilla-beta?
Attachment #8829703 - Flags: approval-mozilla-beta+
Attachment #8829703 - Flags: approval-mozilla-aurora?
Attachment #8829703 - Flags: approval-mozilla-aurora+
Checkin-needed for aurora and beta uplift
Keywords: checkin-needed
Comment on attachment 8829703 [details]
Bug 1328053 - Correct thumb position when shift-click scrolling

Fixes a regression, has baked on Nightly for a bit, Aurora53+
Flags: qe-verify+
AFAICT, this was already uplifted by the time the checkin-needed request was made. Feel free to ni? me if I'm misunderstanding something here.
Keywords: checkin-needed
(In reply to Ryan VanderMeulen [:RyanVM] from comment #24)
> AFAICT, this was already uplifted by the time the checkin-needed request was
> made. Feel free to ni? me if I'm misunderstanding something here.

Indeed, the checkin-needed request was redundant.

Thanks Ryan!
Reproduced the initial issue using Nightly 53.0a1 (Build ID: 20170117030218) on Windows 10 x64. 

Verified as fixed using the latest Nightly 54.0a1 (Build ID: 20170207030214) on Windows 10 x64.
Status: RESOLVED → VERIFIED
I have reproduced this issue using Firefox 53.0a1 (20170117030218) on Win 8.1 x64.
I can confirm this issue is fixed, I verified using Firefox 52.0b6 and 53.0a2 on Win 8.1 x64, Ubuntu 14.04 x64 and Mac OS X 10.12.3.
Depends on: 1344339
You need to log in before you can comment on or make changes to this bug.