Last Comment Bug 710373 - Pressing the vertical slider down arrow with smooth scrolling should move three lines instead of one
: Pressing the vertical slider down arrow with smooth scrolling should move thr...
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: Layout: View Rendering (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: Jared Wein [:jaws] (please needinfo? me)
:
Mentors:
Depends on:
Blocks: 710372
  Show dependency treegraph
 
Reported: 2011-12-13 13:25 PST by Jared Wein [:jaws] (please needinfo? me)
Modified: 2012-05-28 08:25 PDT (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
an attempt to fix the bug (doesn't work) (2.16 KB, patch)
2012-01-08 05:33 PST, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
WIP of patch for bug 710373 (doesn't work) (1.30 KB, patch)
2012-01-17 16:43 PST, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
Patch for bug 710373 (1.94 KB, patch)
2012-01-31 02:31 PST, Jared Wein [:jaws] (please needinfo? me)
roc: review+
Details | Diff | Splinter Review
Patch for checkin (2.00 KB, patch)
2012-01-31 03:08 PST, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review

Description Jared Wein [:jaws] (please needinfo? me) 2011-12-13 13:25:54 PST

    
Comment 1 Jared Wein [:jaws] (please needinfo? me) 2012-01-07 22:35:39 PST
roc: Do you know who might be able to take this?
Comment 2 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-08 00:49:38 PST
No, but I don't really understand this bug. Isn't this behavior platform-specific?
Comment 3 Jared Wein [:jaws] (please needinfo? me) 2012-01-08 02:13:48 PST
Limi: This was requested in the jankshrank etherpad. Can you provide more details here? Are you asking that we deviate from platform-specific behavior?
Comment 4 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-08 02:26:11 PST
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.
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-08 02:28:03 PST
The best place to do this would be nsGfxScrollFrameInner::CreateAnonymousContent.
Comment 6 Jared Wein [:jaws] (please needinfo? me) 2012-01-08 03:29:29 PST
(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
Comment 7 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-08 03:43:10 PST
(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.
Comment 8 Jared Wein [:jaws] (please needinfo? me) 2012-01-08 05:33:53 PST
Created attachment 586797 [details] [diff] [review]
an attempt to fix the bug (doesn't work)

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?
Comment 9 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-08 14:00:50 PST
(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?
Comment 10 Jared Wein [:jaws] (please needinfo? me) 2012-01-17 16:43:35 PST
Created attachment 589349 [details] [diff] [review]
WIP of patch for bug 710373 (doesn't work)

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?
Comment 11 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-18 15:26:16 PST
(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.
Comment 12 Jared Wein [:jaws] (please needinfo? me) 2012-01-19 16:33:00 PST
(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?
Comment 13 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-19 17:47:57 PST
(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?
Comment 14 Jared Wein [:jaws] (please needinfo? me) 2012-01-19 22:05:39 PST
(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?
Comment 15 Jared Wein [:jaws] (please needinfo? me) 2012-01-24 22:37:56 PST
roc: feedback ping?
Comment 16 [not reading bugmail] 2012-01-28 19:39:26 PST
This looks a lot like issues discussed in bug 593466, bug 635645, bug 629507 and bug 675866.
Comment 17 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-30 13:16:02 PST
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!!!!!
Comment 18 Jared Wein [:jaws] (please needinfo? me) 2012-01-31 02:31:16 PST
Created attachment 593030 [details] [diff] [review]
Patch for bug 710373

Thanks for your help Robert. I've tested this out and it works fine now.
Comment 19 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-31 02:33:09 PST
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?
Comment 20 Jared Wein [:jaws] (please needinfo? me) 2012-01-31 03:06:49 PST
I made the scroll multiplier a named constant.

https://hg.mozilla.org/integration/fx-team/rev/1fd0846a865f
Comment 21 Jared Wein [:jaws] (please needinfo? me) 2012-01-31 03:08:05 PST
Created attachment 593034 [details] [diff] [review]
Patch for checkin
Comment 22 Tim Taubert [:ttaubert] 2012-01-31 05:48:05 PST
https://hg.mozilla.org/mozilla-central/rev/1fd0846a865f
Comment 23 Mihaela Velimiroviciu (:mihaelav) 2012-05-28 08:25:00 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.