arni2033 Reporter
	Description • 8 years ago

>>>   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.

	Mats Palmgren (inactive)
	Comment 1 • 8 years ago

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?

	castiel743
	Comment 2 • 8 years ago

>>>   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.

	:kip (Kearwood Gilbert) Assignee
	Comment 3 • 8 years ago

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.

	:kip (Kearwood Gilbert) Assignee
	Comment 4 • 8 years ago

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.

	:kip (Kearwood Gilbert) Assignee
	Comment 5 • 7 years ago

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.

	:kip (Kearwood Gilbert) Assignee
	Comment 6 • 7 years ago

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.

	:kip (Kearwood Gilbert) Assignee
	Comment 8 • 7 years ago

I am running my proposed fix through try as a sanity check, if it passes all tests, then will assign for a review.

	:kip (Kearwood Gilbert) Assignee
	Comment 9 • 7 years ago

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().

	:kip (Kearwood Gilbert) Assignee
	Comment 11 • 7 years ago

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.

	:kip (Kearwood Gilbert) Assignee
	Comment 12 • 7 years ago

The try run looks good.  I believe this 1-line fix should correct this issue, and potentially others that were regressed.

	Mats Palmgren (inactive)
	Comment 13 • 7 years ago
mozreview-review

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.

	Pulsebot
	Comment 15 • 7 years ago

Pushed by kgilbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b30c5aed91fb
Correct thumb position when shift-click scrolling r=mats

	Phil Ringnalda (:philor)
	Comment 16 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/b30c5aed91fb

	Ryan VanderMeulen [:RyanVM]
	Comment 17 • 7 years ago

Looks like a good candidate for backport to 52 and 53. Please fill out the requests :)

	:kip (Kearwood Gilbert) Assignee
	Comment 18 • 7 years ago

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

	Julien Cristau [:jcristau]
	Comment 19 • 7 years ago

Comment on attachment 8829703 [details]
Bug 1328053 - Correct thumb position when shift-click scrolling

simple fix for scrolling regression, aurora53+, beta52+

	Carsten Book [:Tomcat]
	Comment 20 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/c76aa324c7c2

	Carsten Book [:Tomcat]
	Comment 21 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/04c3a6ba0e1c

	:kip (Kearwood Gilbert) Assignee
	Comment 22 • 7 years ago

Checkin-needed for aurora and beta uplift

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 23 • 7 years ago

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+

	Ryan VanderMeulen [:RyanVM]
	Comment 24 • 7 years ago

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.

	:kip (Kearwood Gilbert) Assignee
	Comment 25 • 7 years ago

(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!

	Simona Badau, Desktop QA
	Comment 26 • 7 years ago

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.

	Timea Zsoldos [:zstimi/tzsoldos], Desktop QA
	Comment 27 • 7 years ago

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.