Closed
Bug 710373
Opened 13 years ago
Closed 13 years ago
Pressing the vertical slider down arrow with smooth scrolling should move three lines instead of one
Categories
(Core :: Web Painting, defect)
Core
Web Painting
Tracking
()
VERIFIED
FIXED
mozilla12
People
(Reporter: jaws, Assigned: jaws)
References
Details
Attachments
(2 files, 2 obsolete files)
1.94 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
2.00 KB,
patch
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•13 years ago
|
||
roc: Do you know who might be able to take this?
No, but I don't really understand this bug. Isn't this behavior platform-specific?
Assignee | ||
Comment 3•13 years ago
|
||
Limi: This was requested in the jankshrank etherpad. Can you provide more details here? Are you asking that we deviate from platform-specific behavior?
Summary: Pressing the veritcal slider down arrow with smooth scrolling should move three lines instead of one → Pressing the vertical slider down arrow with smooth scrolling should move three lines instead of one
Actually I think I'm wrong, this code isn't platform specific.
Anyway, this is a trivial change. You just need to set the 'increment' attribute of the appropriate scrollbar element.
The best place to do this would be nsGfxScrollFrameInner::CreateAnonymousContent.
Assignee | ||
Comment 6•13 years ago
|
||
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5)
> The best place to do this would be
> nsGfxScrollFrameInner::CreateAnonymousContent.
I couldn't find any references to an 'increment' within nsGfxScrollFrameInner::CreateAnonymousContent. Shouldn't the change be made in nsGfxScrollFrameInner::GetLineScrollAmount?
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp#2210
(In reply to Jared Wein [:jwein and :jaws] from comment #6)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5)
> > The best place to do this would be
> > nsGfxScrollFrameInner::CreateAnonymousContent.
>
> I couldn't find any references to an 'increment' within
> nsGfxScrollFrameInner::CreateAnonymousContent. Shouldn't the change be made
> in nsGfxScrollFrameInner::GetLineScrollAmount?
No, that will affect all kinds of things including mousewheel scrolling.
You just want to add an 'increment' attribute like we add the other attributes.
Assignee | ||
Comment 8•13 years ago
|
||
roc: I've tried adding the attribute to the vertical scrollbar, as well as the resizer (I expect that is wrong/unnecessary), but I can't see any difference.
I did a full rebuild, and used the keyboard down arrow as well as pressing on the down arrow of the scrollbar.
Am I on the right path?
(In reply to Jared Wein [:jwein and :jaws] from comment #8)
> I did a full rebuild, and used the keyboard down arrow as well as pressing
> on the down arrow of the scrollbar.
Keyboard down arrow is a different beast.
I expect pressing the down button to work with that patch. Sad that it doesn't. Maybe stepping through nsScrollbarButtonFrame::HandleButtonPress would help understand why it doesn't?
Assignee | ||
Updated•13 years ago
|
Attachment #586797 -
Flags: feedback?(roc)
Assignee | ||
Comment 10•13 years ago
|
||
I've stepped through the code and got down to nsSliderFrame::GetIntegerAttribute where it always return "19" for the increment attribute, regardless of what increment is set to on the mVScrollBarContent.
roc: Do you have any other tips?
Attachment #586797 -
Attachment is obsolete: true
Attachment #589349 -
Flags: feedback?(roc)
(In reply to Jared Wein [:jaws] from comment #10)
> I've stepped through the code and got down to
> nsSliderFrame::GetIntegerAttribute where it always return "19" for the
> increment attribute, regardless of what increment is set to on the
> mVScrollBarContent.
You mean "1"? Odd... Can you step through GetIntegerAttribute to figure out why?
The slider element's parent element should be mVScrollbarContent, can you verify that? The xbl:inherits on the <xul:slider> element in scrollbar.xml includes "increment" so it should have been inherited.
Assignee | ||
Comment 12•13 years ago
|
||
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #11)
> (In reply to Jared Wein [:jaws] from comment #10)
> > I've stepped through the code and got down to
> > nsSliderFrame::GetIntegerAttribute where it always return "19" for the
> > increment attribute, regardless of what increment is set to on the
> > mVScrollBarContent.
>
> You mean "1"? Odd... Can you step through GetIntegerAttribute to figure out
> why?
Down in nsXULelement::GetAttr, the result that comes back is:
aResult
{mData=0x0d7801d0 L"19" mLength=2 mFlags=65541 }
mData: 0x0d7801d0 L"19"
mLength: 2
mFlags: 65541
Could the 19 here be in CSS pixels instead of lines?
> The slider element's parent element should be mVScrollbarContent, can you
> verify that?
I'm not sure how to verify this. I got the memory address of mVScrollbarContent and the memory address of |this| within nsScrollbarButtonFrame::HandleButtonPress and they are different. This makes sense to me though, since the button frame isn't the scrollbar, but I'm not sure how to find the parent scrollbar from the button frame (or the button from the mVScrollbarContent instance).
> The xbl:inherits on the <xul:slider> element in scrollbar.xml
> includes "increment" so it should have been inherited.
The <xul:slider> has the xbl:inherits attribute that includes increment, but the various xul:scrollbarbuttons don't have the increment attribute inherited. Should they?
(In reply to Jared Wein [:jaws] from comment #12)
> Could the 19 here be in CSS pixels instead of lines?
Yes, of course! I'm dumb.
> The <xul:slider> has the xbl:inherits attribute that includes increment, but the
> various xul:scrollbarbuttons don't have the increment attribute inherited. Should
> they?
I don't think so. We're getting the increment from the scrollbar element. In fact, the slider increment shouldn't even be relevant.
So the question is, is the scrollbar element found in HandleButtonPress the same as the mVScrollbarContent where you set the increment?
Assignee | ||
Comment 14•13 years ago
|
||
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #13)
> So the question is, is the scrollbar element found in HandleButtonPress the
> same as the mVScrollbarContent where you set the increment?
Ok, I think I've figured it out!
I set a breakpoint in nsGfxScrollFrameInner::CreateAnonymousContent and nsScrollbarButtonFrame::HandleButtonPress.
I created a new window, and nsGfxScrollFrameInner::CreateAnonymousContent was called three times with the following mVScrollbarContent instances, in order:
0x13d82178
0x0dd3a490
0x0dd3dc58
Then I pressed the down button on the scrollbar of the window, and the scrollbar->mParent->mInner->mVScrollbarContent has an mRawPtr with address of 0x0dd3dc58.
I'm not sure why three vertical scrollbars are created (unless there is one mVScrollbarContent each for the up button, slider, and down button).
roc: Does this make sense?
Assignee | ||
Comment 15•13 years ago
|
||
roc: feedback ping?
Comment 16•13 years ago
|
||
I am an idiot.
nsGfxScrollFrameInner::FinishReflowForScrollbar resets the 'increment' attribute to match the line height.
So you want to modify nsGfxScrollFrameInner::ReflowFinished to change what we pass to FinishReflowForScrollbar.
Sorry!!!!!
Assignee | ||
Updated•13 years ago
|
Attachment #589349 -
Flags: feedback?(roc)
Assignee | ||
Comment 18•13 years ago
|
||
Thanks for your help Robert. I've tested this out and it works fine now.
Attachment #589349 -
Attachment is obsolete: true
Attachment #593030 -
Flags: review?(roc)
Comment on attachment 593030 [details] [diff] [review]
Patch for bug 710373
Review of attachment 593030 [details] [diff] [review]:
-----------------------------------------------------------------
::: layout/generic/nsGfxScrollFrame.cpp
@@ +3218,5 @@
> nsWeakFrame weakFrame(mOuter);
> nsPoint scrollPos = GetScrollPosition();
> // XXX shouldn't we use GetPageScrollAmount/GetLineScrollAmount here?
> if (vScroll) {
> + nscoord fontHeight = GetLineScrollAmount().height * 3;
Make this a named constant? or even a pref?
Attachment #593030 -
Flags: review?(roc) → review+
Assignee | ||
Comment 20•13 years ago
|
||
I made the scroll multiplier a named constant.
https://hg.mozilla.org/integration/fx-team/rev/1fd0846a865f
Whiteboard: [fixed-in-fx-team]
Assignee | ||
Comment 21•13 years ago
|
||
Comment 22•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla12
Comment 23•13 years ago
|
||
Mozilla/5.0 (Windows NT 6.1; rv:13.0) Gecko/20100101 Firefox/13.0
Mozilla/5.0 (X11; Linux i686; rv:13.0) Gecko/20100101 Firefox/13.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:13.0) Gecko/20100101 Firefox/13.0
Verified the fix on the above builds: pressing the vertical slider up/down arrow with moves three lines.
Marking as VERIFIED.
Status: RESOLVED → VERIFIED
Updated•6 years ago
|
Component: Layout: View Rendering → Layout: Web Painting
You need to log in
before you can comment on or make changes to this bug.
Description
•