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?

Limi: This was requested in the jankshrank etherpad. Can you provide more details here? Are you asking that we deviate from platform-specific behavior?

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.

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

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?

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?

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

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

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

roc: feedback ping?

This looks a lot like issues discussed in bug 593466, bug 635645, bug 629507 and bug 675866.

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

Thanks for your help Robert. I've tested this out and it works fine now.

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?

I made the scroll multiplier a named constant. https://hg.mozilla.org/integration/fx-team/rev/1fd0846a865f

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.