Closed
Bug 415523
Opened 15 years ago
Closed 15 years ago
Scrollbar thumb jumps on drag start
Categories
(Core :: XUL, defect)
Core
XUL
Tracking
()
RESOLVED
FIXED
mozilla1.9beta4
People
(Reporter: mstange, Assigned: mstange)
Details
(Keywords: regression)
Attachments
(2 files, 4 obsolete files)
9.15 KB,
image/png
|
Details | |
15.77 KB,
patch
|
roc
:
review+
roc
:
superreview+
roc
:
approval1.9+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•15 years ago
|
||
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)
Assignee | ||
Comment 2•15 years ago
|
||
![]() |
||
Comment 3•15 years ago
|
||
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)
Assignee | ||
Updated•15 years ago
|
Component: General → XP Toolkit/Widgets
Product: Firefox → Core
Version: unspecified → Trunk
Comment 4•15 years ago
|
||
--> confirming. Patch also seems to fix the issue.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Updated•15 years ago
|
Assignee: nobody → mstange
Comment 5•15 years ago
|
||
This is a regression from Firefox 2.0.0.x.
Flags: blocking1.9?
Keywords: regression
Updated•15 years ago
|
QA Contact: general → xptoolkit.widgets
Assignee | ||
Comment 6•15 years ago
|
||
(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
Assignee | ||
Updated•15 years ago
|
Attachment #301247 -
Attachment description: Patch v1.0: Extract the thumb positioning logic → Fix v1.0: Extract the thumb positioning logic, fix bug
Comment 7•15 years ago
|
||
(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).
Assignee | ||
Comment 8•15 years ago
|
||
(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•15 years ago
|
||
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.
Assignee | ||
Comment 11•15 years ago
|
||
(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•15 years ago
|
||
Ah yes, that's classic. Well, I guess it's really any theme that uses native looking widgets.
Assignee | ||
Comment 13•15 years ago
|
||
(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)
Assignee | ||
Comment 14•15 years ago
|
||
> 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.
Assignee | ||
Comment 16•15 years ago
|
||
(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+
Keywords: checkin-needed
Comment 18•15 years ago
|
||
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: 15 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.
Description
•