The default bug view has changed. See this FAQ.

Pressing the vertical slider down arrow with smooth scrolling should move three lines instead of one

VERIFIED FIXED in mozilla12

Status

()

Core
Layout: View Rendering
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: jaws, Assigned: jaws)

Tracking

(Blocks: 1 bug)

Trunk
mozilla12
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Comment hidden (empty)
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?
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.
(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.
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?
Assignee: nobody → jwein
Status: NEW → ASSIGNED
Attachment #586797 - Flags: feedback?(roc)
(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?
Attachment #586797 - Flags: feedback?(roc)
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?
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.
(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!!!!!
Attachment #589349 - Flags: feedback?(roc)
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.
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+
I made the scroll multiplier a named constant.

https://hg.mozilla.org/integration/fx-team/rev/1fd0846a865f
Whiteboard: [fixed-in-fx-team]
Created attachment 593034 [details] [diff] [review]
Patch for checkin
https://hg.mozilla.org/mozilla-central/rev/1fd0846a865f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla12
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
You need to log in before you can comment on or make changes to this bug.