Closed Bug 1249710 Opened 4 years ago Closed 3 years ago

async drag: Can't drag all the way vertically

Categories

(Core :: Panning and Zooming, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla52
Tracking Status
firefox52 --- verified

People

(Reporter: BenWa, Assigned: kevin.m.wern)

References

Details

(Keywords: correctness, Whiteboard: [gfx-noted])

Attachments

(1 file)

STR:
0) Enable async drag
1) https://github.com/mozilla/rr/issues/1652
2) Drag the horizontal scrollbar showing the GDB snippet
This is also made worse by increasing full-zoom.
Duplicate of this bug: 1292607
Assignee: nobody → kevin.m.wern
Assigning myself here because I'm closer to figuring out this issue from working on bug 1249162. Moreso than I am to solving that bug itself.
This patch just identifies the fix. We'll probably need testing for this, or this and other bugs combined.

The issue here is:
* There are two "max scroll positions"--one for the scrollbar (scrollMax, size of scrollbar - size of thumb) and one for the view of the content (maxScrollPosition, size of scrollable pane - size of the view).
* The second value is miscalculated because mFrameMetrics.GetCompositionBounds() is used for the size of the view. This value does not change according to zoom, and can also be larger than the view--even without zoom. So maxScrollPosition is really smaller than it should be.
* Because maxScrollPosition is used to scale mousePosition to the scrollableFrame's bounds, i.e. mousePosition/scrollMax * maxScrollPosition, the thumb and the mouse end up staggered in most cases, especially when zoom is not 1.
* This manifests itself in the above bug (because the scroll position is capped at maxScrollPosition), the aforementioned staggering, and large leaps when clicking on the scroll thumb as scrollPosition moves to where it "should" be based on this proportion.

Let me know if the replacement value makes sense.
Comment on attachment 8806796 [details]
Bug 1249710: fix unit mismatch calculating scroll range during async scrollbar drag

https://reviewboard.mozilla.org/r/90100/#review89808

Nice work\! 

You are right that we have a unit mismatch in the calculation of |maxScrollPosition|: we are subtracting a ParentLayer coordinate (coming from FrameMetrics::GetCompositionBounds()) from a CSS coordinate (coming from FrameMetrics::GetScrollableRect()).

You're also right that it's the ParentLayer coordinate that should be adjusted, since the result of the subtraction is interpreted as a CSS coordinate.

GetViewport() is a good guess as to where to get the correct CSS coordinate from, but isn't quite right. The terminology here is a bit confusing: we use "composition bounds" to refer to the size of the "window" through which we look at scrollable content, while "viewport" refers to an HTML document's CSS viewport. The CSS viewport is typically the same size as the composition bounds, especially on desktop, but it doesn't have to be: a web author can override it to be different using the <meta name="viewport"> tag \[1\]. (Note also that for scrollable frames that don't represent a document, but something else like a scrollable <div>, GetViewport() is meaningless, as documented here \[2\]).

So the quantity that we want to use really is the composition bounds, but converted to CSS coordinates. We have a handy function that gives us that: FrameMetrics::CalculateCompositedRectInCssPixels().

I would also like to take this opportunity to improve the use of strongly-typed units in HandleDragEvent(). Currently, several variables in that function have type |float| when they could instead have a strongly-typed unit such as |CSSCoord| or |ParentLayerCoord|.

To fix this, I suggest the following:

  \- Revise the GetAxisStart(), GetAxisEnd() etc. functions to return strongly-typed units.

    This will require changing their signatures from things like:

      template <typename T>
      static float GetAxisStart(AsyncDragMetrics::DragDirection aDir, T aValue);

    to things like:

      template <typename Units>
      static CoordTyped<Units> GetAxisStart(AsyncDragMetrics::DragDirection aDir, const PointTyped<Units>& aValue);

    (This is so that we can express the correct return type. If we leave the argument type as "T",
    we'd have a hard time expressing the right return type.)

    It will also require writing multiple overloads for some of these functions, as some
    of them are called with both PointTyped and RectTyped arguments - that's fine.

    (Observe that, just with this change, the unit mismatch in the calculation of |maxScrollPosition|
    becomes a compiler error\! Had we done this from the beginning, we would never have introduced
    this bug :)).

  \- Where possible, change the types of |float| variables in HandleDragEvent(), such as
    |maxScrollPosition|, to be typed units like |CSSCoord|.

\[1\] https://developer.mozilla.org/en/docs/Mozilla/Mobile/Viewport_meta_tag
\[2\] http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/gfx/layers/FrameMetrics.h#623
Attachment #8806796 - Flags: review?(botond)
Funny, I was just about to ask about using cssCompositionBounds (which stores the return from the same function). Your second annotation was where I originally started to think maybe I made wrong choice.

Anyway, I'll work on those changes. Maybe it'll reveal some more about bug 1249162, as well :).
Comment on attachment 8806796 [details]
Bug 1249710: fix unit mismatch calculating scroll range during async scrollbar drag

https://reviewboard.mozilla.org/r/90100/#review89864

Looks great, r+ with just one small change:

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:864
(Diff revision 2)
> +    return aValue.y;
> +  }
> +}
> +
> +template <typename Units>
> +static CoordTyped<Units> GetAxisEnd(AsyncDragMetrics::DragDirection aDir, const IntRectTyped<Units>& aValue) {

If we're taking IntRectTyped as argument, let's return IntCoordTyped.
Attachment #8806796 - Flags: review?(botond) → review+
Comment on attachment 8806796 [details]
Bug 1249710: fix unit mismatch calculating scroll range during async scrollbar drag

https://reviewboard.mozilla.org/r/90100/#review90420

Last thing: could you mention async scrollbar dragging in the commit message please?

Then I can land the patch directly from MozReview :)
Thanks! Autolanded.
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/720fa40353a7
fix unit mismatch calculating scroll range during async scrollbar drag r=botond
https://hg.mozilla.org/mozilla-central/rev/720fa40353a7
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
I couldn't reproduce this on a Win 10 64-bit or an iMac OS X 10.11 systems.
I did reproduced it on a MacBook Pro retina with 10.11.6 where I can confirm it is fixed on Firefox 52 beta 4 using the url from this bug and it's duplicate, bug 1249749.

I noticed that on https://github.com/mozilla/rr/issues/1652 with apz drag enabled, I can't scroll vertically the first time I try after I open (or refresh) the page, only the second mouse drag performs the scrolling. Is anyone aware of an open bug for this matter? Thanks!
Status: RESOLVED → VERIFIED
(In reply to Petruta Rasa [QA] [:petruta] from comment #17)
> I noticed that on https://github.com/mozilla/rr/issues/1652 with apz drag
> enabled, I can't scroll vertically the first time I try after I open (or
> refresh) the page, only the second mouse drag performs the scrolling. Is
> anyone aware of an open bug for this matter? Thanks!

I don't think there's an open bug for this - please file, thanks!
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #18)
> (In reply to Petruta Rasa [QA] [:petruta] from comment #17)
> > I noticed that on https://github.com/mozilla/rr/issues/1652 with apz drag
> > enabled, I can't scroll vertically the first time I try after I open (or
> > refresh) the page, only the second mouse drag performs the scrolling. Is
> > anyone aware of an open bug for this matter? Thanks!
> 
> I don't think there's an open bug for this - please file, thanks!

I was about to file the bug when I noticed that it only reproduces on Firefox 52 beta with apz.drag.enabled set to true. Considering that it is already fixed on Nightly and Aurora, and the pref is disabled by default on beta, I don't think it should be filled at this moment. Please let me know if that's not the case.
That's correct, thanks for checking!
You need to log in before you can comment on or make changes to this bug.