415523 - Scrollbar thumb jumps on drag start

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Markus Stange [:mstange]]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2008020104 Minefield/3.0b3pre
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2008020104 Minefield/3.0b3pre

When dragging the scrollbar thumb, the first mouse move event moves the scrollbar thumb further than the mouse has moved.

Reproducible: Always

Steps to Reproduce:
AFAIK this bug is only visible on Mac OS X.
1. Make sure you've checked "Together" in the OS X General prefs panel for scroll arrows.
2. Visit a page that is long enough to require a scroll bar.
3. Notice that the scrollbar thumb is 6px away from the top border of the viewport when you're at the top of the document.
4. Click and hold the scrollbar thumb.
5. Move your mouse pointer 1px upwards.
Actual Results:  
The scrollbar thumb moves 5px downwards.

Expected Results:  
The scrollbar thumb should follow the mouse pointer accurately.

This bug can only be reproduced on Mac OS X because on other platforms (I think) the clientRect of the scrollbar always starts at 0px; that is, in vertical scrollbars, clientRect.y is always 0.
In those cases, the thumb-dragging logic in nsSliderFrame.cpp works.
However, on OS X, clientRect.y can be 6px (=360), so the logic fails.

Comment 1

[Markus Stange [:mstange]]

Attachment: Fix v1.0: Extract the thumb positioning logic, fix bug

Transforing the thumb position into the scroll position was duplicated several times in that file, so I decided to rework the logic into a new method (SetCurrentThumbPosition). This function also substracts the clientRect's offset, which was missing.

Comment 2

[Markus Stange [:mstange]]

Attachment: Patch v1.0, diff -w

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 301247 [details] [diff] [review]
Fix v1.0: Extract the thumb positioning logic, fix bug

I'm not going to get to this in any sort of reasonable timeframe (measured in months).  Passing off to roc.

Comment 4

[Stefan [:stefanh]]

--> confirming. Patch also seems to fix the issue.

Comment 5

[Samuel Sidler (old account; do not CC)]

This is a regression from Firefox 2.0.0.x.

Comment 6

[Markus Stange [:mstange]]

(In reply to comment #4)
> Patch also seems to fix the issue.

Err, yes. That's what I meant with
> This function also substracts the clientRect's offset, which was missing.

I probably should have stated that more clearly ;)

Comment 7

[Stefan [:stefanh]]

(In reply to comment #6)
> (In reply to comment #4)
> > Patch also seems to fix the issue.
> 
> Err, yes. That's what I meant with
> > This function also substracts the clientRect's offset, which was missing.
> 
> I probably should have stated that more clearly ;)
> 

I don't really know why I added that comment, I guess for some reason I just wanted to point out that I've tried the patch and it seemed to work (but since I have no knowledge of the code I didn't want to say anything more).

Comment 8

[Markus Stange [:mstange]]

(In reply to comment #7)
> I don't really know why I added that comment, I guess for some reason I just
> wanted to point out that I've tried the patch and it seemed to work

Ah, I see. Thank you for testing it.

Comment 9

[jag (Peter Annema)]

Is this a Classic thing? I don't see it in the Modern theme in SeaMonkey.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This is great!

-         SetCurrentPosition(scrollbar, (int) (mThumbStart / onePixel / mRatio),
-                            PR_FALSE, PR_TRUE);
+        SetCurrentThumbPosition(scrollbar, mThumbStart, PR_FALSE, PR_TRUE, PR_FALSE);

Fix indent

+  void SetCurrentThumbPosition(nsIContent* scrollbar, nscoord pos, PRBool aIsSmooth,
+                               PRBool aImmediateRedraw, PRBool maySnap);
   void SetCurrentPosition(nsIContent* scrollbar, nscoord pos, PRBool aIsSmooth,
                           PRBool aImmediateRedraw);

Would you mind adding comments documenting at least these two functions so it's clear why we need both, and which one to use in any situation?

Also if you could fix 'scrollbar', 'pos' and 'maySnap' to follow the aParam naming convention ... I don't much like that convention but consistency is important.

+  nscoord realpos = newpos - offset;
+  realpos /= nsPresContext::CSSPixelsToAppUnits(1);

make realpos a float so you don't lose precision and use nsPresContext::AppUnitsToFloatCSSPixels.

+  PRBool snap = PR_FALSE;
+  if (maySnap) {
+    snap = mContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::snap,
+                                 nsGkAtoms::_true, eCaseMatters);
+  if (snap) {

Simpler as "if (maySnap && ..."

> nscoord(realpos / mRatio)

Use NSToIntRound. I know you're just moving this code but we may as well fix it.

Comment 11

[Markus Stange [:mstange]]

Attachment: Illustration to show the problem

(In reply to comment #9)
> Is this a Classic thing? I don't see it in the Modern theme in SeaMonkey.

You need a scrollbar that looks like this. I don't know the Modern theme in SeaMonkey.

Comment 12

[jag (Peter Annema)]

Ah yes, that's classic. Well, I guess it's really any theme that uses native looking widgets.

Comment 13

[Markus Stange [:mstange]]

Attachment: Fix v1.1: address review comments

(In reply to comment #10)
> -         SetCurrentPosition(scrollbar, (int) (mThumbStart / onePixel /
> mRatio),
> -                            PR_FALSE, PR_TRUE);
> +        SetCurrentThumbPosition(scrollbar, mThumbStart, PR_FALSE, PR_TRUE,
> PR_FALSE);
> 
> Fix indent

I believe that's what I did here. The old indentation was wrong.

> +  void SetCurrentThumbPosition(nsIContent* scrollbar, nscoord pos, PRBool
> aIsSmooth,
> +                               PRBool aImmediateRedraw, PRBool maySnap);
>    void SetCurrentPosition(nsIContent* scrollbar, nscoord pos, PRBool
> aIsSmooth,
>                            PRBool aImmediateRedraw);
> 
> Would you mind adding comments documenting at least these two functions so it's
> clear why we need both, and which one to use in any situation?

I tried too... is it ok this way?
However, I'm not sure if we really need both; SetCurrentThumbPosition is now the only caller of SetCurrentPosition. Should I merge them?

> Also if you could fix 'scrollbar', 'pos' and 'maySnap' to follow the aParam
> naming convention ... I don't much like that convention but consistency is
> important.

Done.

> +  nscoord realpos = newpos - offset;
> +  realpos /= nsPresContext::CSSPixelsToAppUnits(1);
> 
> make realpos a float so you don't lose precision and use
> nsPresContext::AppUnitsToFloatCSSPixels.

Done.

> +  PRBool snap = PR_FALSE;
> +  if (maySnap) {
> +    snap = mContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::snap,
> +                                 nsGkAtoms::_true, eCaseMatters);
> +  if (snap) {
> 
> Simpler as "if (maySnap && ..."

Done.

> > nscoord(realpos / mRatio)
> 
> Use NSToIntRound. I know you're just moving this code but we may as well fix
> it.

I realized there's NSToCoordRound as well and used that instead.

Do you know what to do with the snapping behaviour? Currently aMaySnap is only set to true in the thumb-dragging code. Should we set it to true in the scroll-to-click code as well? Maybe even on thumb-mouse-down? I haven't seen a snapping scrollbar yet, so I don't know how it's supposed to feel.

Could anyone test if this patch fixes bug 81586 as well? On OS X, there is no snapping back when the mouse leaves the thumb, so I can't test it myself.

Comment 14

[Markus Stange [:mstange]]

Attachment: Fix v1.1, diff -w

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> However, I'm not sure if we really need both; SetCurrentThumbPosition is now
> the only caller of SetCurrentPosition. Should I merge them?

No, this is fine.

> I realized there's NSToCoordRound as well and used that instead.

The unfortunate truth is that the newpos parameters to SetCurrentPosition(Internal) are actually in pixels and therefore should be PRInt32 not nscoord, and we should use NSToIntRound instead of NSToCoordRound to convert to them.

Not sure about snapping. Let's not change that in this bug, there is no need here.

Comment 16

[Markus Stange [:mstange]]

Attachment: Fix v1.2: address review comments

(In reply to comment #15)
> in pixels and therefore should be
> PRInt32 not nscoord

Ah ok, I didn't know that.
Thanks for reviewing!

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 301499 [details] [diff] [review]
Fix v1.2: address review comments

Thanks for writing!

Comment 18

[Reed Loden [:reed] (use needinfo?)]

Checking in layout/xul/base/src/nsSliderFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsSliderFrame.cpp,v  <--  nsSliderFrame.cpp
new revision: 1.172; previous revision: 1.171
done
Checking in layout/xul/base/src/nsSliderFrame.h;
/cvsroot/mozilla/layout/xul/base/src/nsSliderFrame.h,v  <--  nsSliderFrame.h
new revision: 1.72; previous revision: 1.71
done