Closed Bug 415523 Opened 13 years ago Closed 13 years ago

Scrollbar thumb jumps on drag start

Categories

(Core :: XUL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9beta4

People

(Reporter: mstange, Assigned: mstange)

Details

(Keywords: regression)

Attachments

(2 files, 4 obsolete files)

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.
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.
Attachment #301247 - Flags: review?(bzbarsky)
Attached patch Patch v1.0, diff -w (obsolete) — Splinter Review
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.
Attachment #301247 - Flags: review?(bzbarsky) → review?(roc)
Component: General → XP Toolkit/Widgets
Product: Firefox → Core
Version: unspecified → Trunk
--> confirming. Patch also seems to fix the issue.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → mstange
This is a regression from Firefox 2.0.0.x.
Flags: blocking1.9?
Keywords: regression
QA Contact: general → xptoolkit.widgets
(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 ;)
Status: NEW → ASSIGNED
Attachment #301247 - Attachment description: Patch v1.0: Extract the thumb positioning logic → Fix v1.0: Extract the thumb positioning logic, fix bug
(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).
(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.
Is this a Classic thing? I don't see it in the Modern theme in SeaMonkey.
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.
(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.
Ah yes, that's classic. Well, I guess it's really any theme that uses native looking widgets.
(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.
Attachment #301247 - Attachment is obsolete: true
Attachment #301248 - Attachment is obsolete: true
Attachment #301410 - Flags: review?(roc)
Attachment #301247 - Flags: review?(roc)
Attached patch Fix v1.1, diff -w (obsolete) — Splinter Review
> 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.
(In reply to comment #15)
> in pixels and therefore should be
> PRInt32 not nscoord

Ah ok, I didn't know that.
Thanks for reviewing!
Attachment #301410 - Attachment is obsolete: true
Attachment #301411 - Attachment is obsolete: true
Attachment #301499 - Flags: review?(roc)
Attachment #301410 - Flags: review?(roc)
Flags: wanted1.9+
Flags: blocking1.9?
Flags: blocking1.9-
Comment on attachment 301499 [details] [diff] [review]
Fix v1.2: address review comments

Thanks for writing!
Attachment #301499 - Flags: superreview+
Attachment #301499 - Flags: review?(roc)
Attachment #301499 - Flags: review+
Attachment #301499 - Flags: approval1.9+
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
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
OS: Mac OS X → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta4
You need to log in before you can comment on or make changes to this bug.