Closed Bug 1152469 Opened 5 years ago Closed 5 years ago

Scroll thumb overlaps scrollbar arrow during async scroll

Categories

(Core :: Panning and Zooming, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: botond, Assigned: dvander)

References

Details

Attachments

(1 file)

STR:
  1. Enable APZ on a platform that has non-overlay scrollbars,
     like Linux or Windows.
  2. Load a page that scrolls vertically.
  3. Do a quick wheel scroll ending at the top or bottom of
     the page.

Expected results:
  The scroll thumb is correctly animated during the async scroll.

Actual results:
  The scroll thumb can overlap the arrow at the end of the scrollbar
  during the async scroll.
Issue was diagnosed and a fix suggested on IRC. Posting relevant subset of conversation for reference:

[14:11] <botond> dvander: yeah, i do see the issue where after scrolling fast, the scroll thumb overlaps the arrow
[14:12] <botond> dvander: and, i'm pretty sure i know why, too
[14:14] <botond> dvander: AsyncCompositionManager::ApplyAsyncTransformToScrollbarForContent does a back-of-the-envelope calculation to approximate how Layout would position the scroll thumb in response to a scroll: http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/AsyncCompositionManager.cpp#746
[14:15] <botond> dvander: that calculation was written with overlay scrollbars on b2g in mind, and thus doesn't take into account the space taken up by the arrows in non-overlay scrollbars
[14:16] <botond> dvander: accordingly, the position it calculates for the thumb for, say, a scroll offset of 0, is "at the top of the scrollbar", which on desktop means overlapping the arrow
[14:21] <botond> dvander: presumably fixing this will involve layout telling the compositor about scrollbar arrow sizes somehow
[14:48] <mstange> all we'd need is one float, that expresses the ratio between scroll position changes and thumb position changes
[14:52] <botond> mstange: yep, that'd work
[14:53] <mstange> botond: yay
[14:53] <mstange> botond: I think there's an mRatio field in nsSliderFrame that expresses exactly that
Assignee: nobody → dvander
Blocks: 1086162
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86_64 → All
Attached patch fixSplinter Review
Patch implementing the proposed solution.
Attachment #8590512 - Flags: review?(mstange)
Comment on attachment 8590512 [details] [diff] [review]
fix

@botond, The ratio from layout is in AppUnits, which this patch converts to CSSPixels. But I think I actually have to convert this to ParentLayerCoords too?
Attachment #8590512 - Flags: review?(botond)
Comment on attachment 8590512 [details] [diff] [review]
fix

Looks good, but I'd do the conversion from app units to CSS pixels in nsSliderFrame::GetThumbRatio(). (I don't like saying that a ratio is "in app units", when it's really in "app units per css pixel", or more specifically in "thumb offset app units per scrolled css pixels". Doing the conversion earlier makes it a true unitless value ("css pixels per css pixels") and gets around that problem.)

At this point it might almost make sense to group the three scrollbar-related layer properties into a ScrollbarProperties struct. But let's not do that in thi bug, if at all.
Attachment #8590512 - Flags: review?(mstange) → review+
Comment on attachment 8590512 [details] [diff] [review]
fix

Review of attachment 8590512 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to David Anderson [:dvander] from comment #3)
> @botond, The ratio from layout is in AppUnits, which this patch converts to
> CSSPixels. But I think I actually have to convert this to ParentLayerCoords
> too?

As Markus pointed out, the ratio is not "in app units", it's in "app units per CSS pixel", which you then convert to "CSS pixels per CSS pixel", which is unitless, so there's no need for any additional conversion between CSS and ParentLayer units.

(I agree that it would be nicer to store the ratio in "CSS pixels per CSS pixel" to begin with.)

::: gfx/layers/Layers.h
@@ +1717,5 @@
>    };
>    nsAutoPtr<StickyPositionData> mStickyPositionData;
>    FrameMetrics::ViewID mScrollbarTargetId;
>    ScrollDirection mScrollbarDirection;
> +  float mScrollbarThumbRatio; // Ratio of the thumb position to the scroll

Please initialize this to something (probably zero) in the constructor.
Attachment #8590512 - Flags: review?(botond) → review+
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8c2cb069b031

w/ comments addressed. Thanks for clarifying the mRatio units.
https://hg.mozilla.org/mozilla-central/rev/8c2cb069b031
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.