1249162 - [apz scrollbar] Clicking on scrollbar slider moves it to wrong position

Status: VERIFIED FIXED

(Core :: Panning and Zooming, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I'm using Ubuntu 15.10 in case that matters. I enabled apz.drag.enabled.

STR:
1. Click down over the actual slider part of the scrollbar.

Expected:
The slider stays put and waits for you to move the mouse.

Actual:
The slider moves, sometimes to the point that no part of it is under the mouse position anymore.

Comment 1

[Benoit Girard (:BenWa)]

Thanks, I'll get it fixed before we enable.

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Affects Windows too.

Comment 3

[Botond Ballo [:botond]]

Happens to me too. It's made much worse by increasing full-zoom.

Comment 5

[Kevin Wern]

Spoke with botond about this and other bugs--I'm taking this one on first. He sent me all the info I need to get started.

I've done everything on reproduce

Comment 6

[Kevin Wern]

(In reply to Kevin Wern from comment #5)
> Spoke with botond about this and other bugs--I'm taking this one on first.
> He sent me all the info I need to get started.
> 
> I've done everything on reproduce

Sorry, tapped send on mobile early.

Basically, I've started by reproducing the bug  and looking at the relevant code. Hoping to really dive in this weekend.

Thanks so much for your help, Botond!

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Awesome, thanks for picking this up! Scrollbar dragging is currently the most-used scrolling method that doesn't go through APZ, so improving the performance by having it go through APZ should help the user experience quite a bit.

Comment 8

[Kevin Wern]

Hey, all. It took me a bit but I think I found the solution. Patch to follow.

The thumb shifts are caused by 3 things:
- Very slight scaling mismatch between actual position and scroll position. mousePosition starts at 1px instead of 0; maxScroll ends up 2px greater than it should be (both before zooming). This covers any case where the scrollbar thumb shifts towards the middle, even with repeated clicks.
- Truncation of AsyncDragMetrics.mScrollbarDragOffset. This covers any case where the zoom factor is not 100% and the thumb doesn't shift with a certain click offset, but *does* shift a single time from the same position with a different offset.
- Clicking after using an alternate form of scrolling that is more granular. Moves the thumb to the nearest round pixel (again, before zooming). This covers any single shift after using something other than a drag (e.g. using the scroll wheel/trackpad)

These issues were solved by:
- Adjusting the calculations for scrollMax (subtracting 2px) and mousePosition (subtracting 1px).
- Making the CSSInt* items in AsyncDragMetrics CSS* and adjusting the assignments in nsSliderFrame::StartDrag to reflect that.
- Only performing AsyncDragMetrics::HandleDragEvent on MOUSE_MOVE.

This patch works, but I suspect there's a better way to represent the scale fixes--any insight there is appreciated. Additionally, the other fixes are harder to verify without commenting out the last one.

Definitely learned a ton from going in the wrong direction a few times :).

Comment 9

[Kevin Wern]

(In reply to Kevin Wern from comment #8)
> - Making the CSSInt* items in AsyncDragMetrics CSS* and adjusting the
> assignments in nsSliderFrame::StartDrag to reflect that.

StartAPZDrag*

Comment 10

[Kevin Wern]

(In reply to Kevin Wern from comment #8)
> - Only performing AsyncDragMetrics::HandleDragEvent on MOUSE_MOVE.

AsyncPanZoomController::HandleDragEvent*

Comment 11

[Kevin Wern]

Attachment: bug 1249162 - Fix unwanted thumb shifts when starting APZ drag

Fix thumb position determination in three places:
- account for scroll track range excluding first and last pixels when
  calculating scrollMax and mousePosition in
  AsyncPanZoomController::HandleDragEvent.
- make AsyncDragMetrics properties mScrollTrack and mScrollbarDragOffset use
  float instead of int coordinates, and update assignments in
  nsSliderFrame::StartAPZDrag
- only change position on MOUSE_MOVE.

Review commit: https://reviewboard.mozilla.org/r/95850/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/95850/

Comment 12

[Botond Ballo [:botond]]

Thanks, Kevin!

(In reply to Kevin Wern from comment #8)
> This patch works, but I suspect there's a better way to represent the scale
> fixes--any insight there is appreciated.

I'll give it some thought. I'll need to get a better understanding of this code myself :)

For now, one question to help me understand the patch better:

> The thumb shifts are caused by 3 things:
> - Very slight scaling mismatch between actual position and scroll position.
> mousePosition starts at 1px instead of 0; maxScroll ends up 2px greater than
> it should be (both before zooming). This covers any case where the scrollbar
> thumb shifts towards the middle, even with repeated clicks.
> - Truncation of AsyncDragMetrics.mScrollbarDragOffset. This covers any case
> where the zoom factor is not 100% and the thumb doesn't shift with a certain
> click offset, but *does* shift a single time from the same position with a
> different offset.
> - Clicking after using an alternate form of scrolling that is more granular.
> Moves the thumb to the nearest round pixel (again, before zooming). This
> covers any single shift after using something other than a drag (e.g. using
> the scroll wheel/trackpad)
> 
> These issues were solved by:
> [...]
> - Only performing AsyncDragMetrics::HandleDragEvent on MOUSE_MOVE.

Which of the above issues does this solve?

Comment 13

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #12)
> > These issues were solved by:
> > [...]
> > - Only performing AsyncDragMetrics::HandleDragEvent on MOUSE_MOVE.
> 
> Which of the above issues does this solve?

Ok, I think I understand: even though we changed the representation of AsyncDragMetrics::mScrollbarDragOffset to be fractional, the source value is still rounded to an integer number of CSS pixels, and so values derived from it may not be able to match a fractional scroll offset accurately enough.

I'd like to understand why the source value is being rounded to an integer number of CSS pixels; I investigated briefly, but I haven't been able to get to the bottom of it. Will follow up on this.

Comment 14

[Botond Ballo [:botond]]

I also have some ideas about the 1px and 2px offsets, I'll write them down when I get a chance.

Comment 15

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #13)
> Ok, I think I understand: even though we changed the representation of
> AsyncDragMetrics::mScrollbarDragOffset to be fractional, the source value is
> still rounded to an integer number of CSS pixels, and so values derived from
> it may not be able to match a fractional scroll offset accurately enough.
> 
> I'd like to understand why the source value is being rounded to an integer
> number of CSS pixels; I investigated briefly, but I haven't been able to get
> to the bottom of it. Will follow up on this.

Markus tells me the value is being rounded deliberately, to avoid invalidation of the scroll thumb while scrolling, and that the main-thread codepath deals with the resulting inconsistency between the thumb's position and the scroll offset by only updating the scroll offset based on the thumb position during a drag after the mouse actually moved.

This is the same thing the patch is doing for the APZ codepath, so that's fine.

Comment 16

[Kevin Wern]

Hey, thanks for taking the time to look at this. Sorry I haven't been able to engage the last day or so.

(In reply to Botond Ballo [:botond] from comment #15)
> Markus tells me the value is being rounded deliberately, to avoid
> invalidation of the scroll thumb while scrolling, and that the main-thread
> codepath deals with the resulting inconsistency between the thumb's position
> and the scroll offset by only updating the scroll offset based on the thumb
> position during a drag after the mouse actually moved.

Hmm, interesting.

> This is the same thing the patch is doing for the APZ codepath, so that's
> fine.

That was the basis for my decision to do that here, as well. I didn't want it to be a cure-all but I figured it at least mirrored the non-apz behavior.

(In reply to Botond Ballo [:botond] from comment #14)
> I also have some ideas about the 1px and 2px offsets, I'll write them down
> when I get a chance.

I'm sure doing the calculation once in nsSliderFrame would be the most effective method--restricting the slider track before it is even passed to dragMetrics. I'm just not sure where the best place to do this would be.

Comment 17

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #14)
> I also have some ideas about the 1px and 2px offsets, I'll write them down
> when I get a chance.

I think there we're making two calculation mistakes, one in AsyncPanZoomController::HandleDragEvent, and one in nsSliderFrame::StartAPZDrag.

Let's talk about HandleDragEvent() first.

To help explain the mistake, let me define some quantities to help us talk about things. For simplicity, let's project things into just the dimension in which we're scrolling (and for exposition, let's say that direction is vertical).

  - Let A be the top of the scroll frame's composition bounds
  - Let B be the top of the scrollbar's slider track.
    The only reason (that I'm aware of) why there can be a non-zero offset between A and B,
    in the direction of scrolling, is if there's a scrollbar button. In that case, the
    slider track starts below the scrollbar button.
  - Let C be the top of the scroll thumb.
  - Let D be the point where we're grabbed the scroll thumb. This is where the mouse was
    when the drag started.
  - Let E be the mouse's current position.

Now, in HandleDragEvent():

  - scrollbarPoint is E
  - mScrollbarDragOffset is (D - C)
  - the start of cssCompositionBound is A
  - the start of mScrollTrack is (B - A)

If we plug these values into the calculation for mousePosition [1], we get that the value of mousePosition is (E - D) + (C - B). This makes sense:

  - (C - B) is how far the top of the scroll thumb was from the top of the slider track
    at the beginning of the drag. It can be thought of as the scroll offset at the
    beginning of the drag, in "scroll track space" (a coordinate space where the scrollable
    range of the element is scaled down to the movement range of the scroll thumb).

  - (E - D) is how far the mouse moved since the start of the drag. It can be thought of
    as the scroll offset delta resulting from the drag (cumulative since the beginning of
    the drag), also in "scroll track space".

Now, further down we divide this quantity by "scrollMax" to derive a scroll percentage [2].

For this to be sensible, "scrollMax" must be the length of the scroll range, in this same "scroll track space".

That is, it should be <length of slider track> - <length of thumb>.

What is actually is, is GetAxisEnd(mScrollTrack) - node->GetScrollSize().

node->GetScrollSize() is indeed the length of the thumb - that part's good. [Except the function is badly named. Feel free to rename it. Also feel free to make it return typed units (LayerIntCoord in this case).]

GetAxisEnd(mScrollTrack) is not quite the length of the slider track - it's GetAxisLength(mScrollTrack), which is the length of the slider track, plus GetAxisStart(mScrollTrack), which is the offset of the slider track relative to the composition bounds ("B - A" in the above terminology).

This offset has no business being part of the calculation. 
 ==> I think we need to use GetAxisLength(mScrollTrack) instead of GetAxisEnd(mScrollTrack) there

[1] http://searchfox.org/mozilla-central/rev/ef3cede9a5b050fffade9055ca274654f2bc719e/gfx/layers/apz/src/AsyncPanZoomController.cpp#917
[2] http://searchfox.org/mozilla-central/rev/ef3cede9a5b050fffade9055ca274654f2bc719e/gfx/layers/apz/src/AsyncPanZoomController.cpp#927

Comment 18

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #17)
> I think there we're making two calculation mistakes, one in
> AsyncPanZoomController::HandleDragEvent, and one in
> nsSliderFrame::StartAPZDrag.

Now, for the issue in StartAPZDrag():

I believe there is a mistake in the computation of the slider track rect [1].

Here's how the frame tree looks for a scroll bar:

  HTMLScrollFrame
    ScrollbarFrame
      SliderFrame
        ButtonBoxFrame

  - The HTMLScrollFrame is the entire scroll frame (the frames for the 
    scrollable content would be children of it, and siblings of the 
    ScrollbarFrame).

  - The ScrollbarFrame is the frame for the entire scrollbar, including
    the scrollbar buttons and the slider track. (In my configuration,
    there are no scrollbar buttons, but if there were, they would be
    siblings of the SliderFrame.

  - The SliderFrame is the frame for the slider track.

  - The ButtonBoxFrame is the frame for the thumb.

Here's how we're comuting the slider track's rect, that's used to derive AsyncDragMetrics::mScrollTrack:

  nsRect sliderTrack = GetRect() - scrollbarBox->GetPosition();

Note that GetRect() and GetPosition() on a frame return a frame's rect or position relative to its parent.

Since we're inside nsSliderFrame, GetRect() returns the rect of the slider frame relative to the scrollbar frame.

scrollbarBox is the scrollbar frame, so its GetPosition() returns the position of the scrollbar frame relative to the scroll frame.

It does not make sense to subtract these quantities. It would make sense to add them, if we wanted the rect of the slider track relative to the scroll frame. However, we don't quite want that: we want the rect of the slider track relative to the scroll frame's composition bounds. (That's the interpretation I assumed in comment 17, and the one that makes it easy to work with the quantity on the APZ side.)

Note that the composition bounds can be offset relative to the scroll frame if the scroll frame has a border. (Also if the scrollbar is on the left or top side instead of the usual right or bottom side, although that offset is not in the direction we're interested in).

So, here's what I suggest doing:

  - Start with the nsSliderFrame's GetRect()
  - *Add* the scrollbar frame's GetPosition(), to get coordinates relative
    to the scroll frame.
  - *Subtract* the position of the composition bounds relative to the scroll
    frame to get coordinates relative to the composition bounds.

The composition bounds is called the "scroll port" in layout-land, and it can be obtained from the scroll frame via GetScrollPortRect() [2]. We already have the scroll frame in StartAPZDrag() - it's the variable "cf", which you should also feel free to rename - it just needs to be cast to the right frame type (nsIScrollableFrame) using do_QueryFrame.

[1] http://searchfox.org/mozilla-central/rev/ef3cede9a5b050fffade9055ca274654f2bc719e/layout/xul/nsSliderFrame.cpp#960
[2] http://searchfox.org/mozilla-central/rev/ef3cede9a5b050fffade9055ca274654f2bc719e/layout/generic/nsIScrollableFrame.h#128

Comment 19

[Botond Ballo [:botond]]

So, here's what I'm curious about: does fixing the calculation mistakes described in comment 17 and comment 18 allow us to remove the 1px and 2px adjustments? Could you try this out and see?

Comment 20

[Botond Ballo [:botond]]

Comment on attachment 8814759 [details]
bug 1249162 - Fix unwanted thumb shifts when starting APZ drag

https://reviewboard.mozilla.org/r/95850/#review97598

Clearing review until we figure out the answer to the question asked in comment 19.

Comment 21

[Kevin Wern]

Hey, Botond! Sorry, I got a bit caught up in investigating (which I still need to do more of), but I figured I owe you an update :).

The changes make sense to me, but they didn't work for the current issue--my configuration has no offset between SliderFrame, ScrollFrame, and composition bounds. I agree that they are necessary, though, and some of the original calculations REALLY didn't make sense.

I think I know what the issue is. It seems that the thumb position in gecko code starts with an offset based on GetXULClientRect(), which is 1px (in app units) all around. Applying this padding to scrollTrack before passing it to dragMetrics fixes the problem. I'm sure min is constrained by this value (in SetCurrentThumbPosition()), and I see it happen with the max value (by printing pos in nsSliderFrame and comparing to scrollMax), I'm just trying to confirm where this actually happens.

I'm just updating you on this to let you know the result, and once I figure this out for sure, I'll try send a patch with your changes and my proposed change.

Let me know if this makes sense.

Comment 22

[Kevin Wern]

Comment on attachment 8814759 [details]
bug 1249162 - Fix unwanted thumb shifts when starting APZ drag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/95850/diff/1-2/

Comment 23

[Kevin Wern]

Hey, Botond!

Sorry, I was sort of in a rush typing the last comment, so I never got to properly thank you for your write-up. It really saved me a lot of time, especially with things that I wouldn't have noticed from my configuration--I'm still figuring out a lot from trial and error stepping through running code. I feel I understand the code, especially in nsSliderFrame, conceptually a lot better.

Anyway, I found the two points where the constraints from GetXULClientRect() are applied to the min position and slider range (called availableRange), so I think this patch properly solves the issue with the offset.

Comment 24

[Botond Ballo [:botond]]

Comment on attachment 8814759 [details]
bug 1249162 - Fix unwanted thumb shifts when starting APZ drag

https://reviewboard.mozilla.org/r/95850/#review98220

Sorry for the slow response - there's a Mozilla All Hands meeting this week, and things are pretty hectic.

Anyways, this all looks great! Thanks for tracking down that last remaining issue with the 1px padding.

I just have two nits related to naming:

::: gfx/layers/apz/src/HitTestingTreeNode.h:99
(Diff revision 2)
>    void SetScrollbarData(FrameMetrics::ViewID aScrollViewId,
>                          Layer::ScrollDirection aDir,
> -                        int32_t aScrollSize,
> +                        int32_t aScrollbarSize,
>                          bool aIsScrollContainer);
>    bool MatchesScrollDragMetrics(const AsyncDragMetrics& aDragMetrics) const;
> -  int32_t GetScrollSize() const;
> +  LayerIntCoord GetScrollbarSize() const;

Can we call this GetScrollThumbSize()? "Scrollbar" is ambiguous, it could mean the thumb or the entire scrollbar.

Even better would be GetScrollThumbLength(), as we generally use "size" to mean both directions.

(I know LayerMetricsWrapper calls it GetScrollbarSize(). Feel free to rename that too.)

::: layout/xul/nsSliderFrame.cpp:948
(Diff revision 2)
> -  nsIContent* scrollableContent = cf->GetContent();
> +  nsIContent* scrollableContent = scrollFrame->GetContent();
>    if (!scrollableContent) {
>      return false;
>    }
>  
> +  nsIScrollableFrame* scrollableFrameBounds = do_QueryFrame(scrollFrame);

"bounds" suggests a rectangle, not a frame. "scrollFrame" would be a good name for this if it weren't used already.

How about "scrollFrameAsScrollable"? That would be consistent with [1].

[1] http://searchfox.org/mozilla-central/rev/3e157ac240611f80df409ae76221d46a31c20dfc/layout/base/nsIPresShell.h#448

Comment 25

[Kevin Wern]

Comment on attachment 8814759 [details]
bug 1249162 - Fix unwanted thumb shifts when starting APZ drag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/95850/diff/2-3/

Comment 26

[Botond Ballo [:botond]]

Comment on attachment 8814759 [details]
bug 1249162 - Fix unwanted thumb shifts when starting APZ drag

https://reviewboard.mozilla.org/r/95850/#review98452

Thanks! I submitted a Try push.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fc1a3d4fd03c
Fix unwanted thumb shifts when starting APZ drag r=botond

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fc1a3d4fd03c

Comment 29

[Botond Ballo [:botond]]

Kevin, thanks for your patience figuring out the subtle issues causing this bug. Nice work!

Comment 30

[Botond Ballo [:botond]]

Async scrollbar dragging is currently Nightly-only, so marking as firefox-53:disabled.

Comment 31

[Botond Ballo [:botond]]

To answer a question Ovidiu asked over IRC: this bug affected all websites with scrollbars, though the amount by which the scrollbar would move when you pressed the mouse button to start dragging it would vary; it could be as little as a pixel. IIRC it was more noticeable at non-default full-zoom levels (e.g. 120%).

Comment 32

[ovidiu boca[:Ovidiu]]

I verified this issue on Mac OS X 10.10, Windows 10 x32 and Ubuntu 14.04 (with touchpad) and I'm not able to reproduce this bug anymore. I will mark this as a verified fix.