Scrollbars jump around when scrolled while zoomed in in the browser

RESOLVED FIXED in Firefox 40

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: kats, Assigned: kats)

Tracking

unspecified
mozilla40
All
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed, b2g-master fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

I tracked this down as a regrssion from bug 1152469. STR:

1) On a master B2G build, load http://people.mozilla.com/~kgupta/grid.html
2) Zoom in
3) Pan around

Note that at step 2 once the zoom is done the scrollbar jumps significantly. Also note at step 3 how the scrollbar janks on repaints; the translation applied during async scrolling is too much and so the repaint causes it to move backwards.

With the patch from bug 1152469 backed out, these issues don't happen. (There is still a small jump once the zoom finishes, but it's very minor compared to what happens with bug 1152469 applied).
Posted patch Patch (obsolete) — Splinter Review
Looks like in bug 1152469 the term "compositedHeight / scrollableHeight" was replaced with "aScrollbar->GetScrollbarThumbRatio()". The former term was equivalent to "(metrics.mCompositionBounds / effectiveZoom).height / metrics.GetScrollableRect().height" which includes the resolution and async zoom bits. The ScrollbarThumbRatio however does not. This patch seems to fix the behaviour for me locally.

However, I'm also wondering about the code at [1] for example. It too contains a "compositedWidth/scrollableWidth" term (inverted) - should that have been replaced by the thumb ratio as well?

[1] https://hg.mozilla.org/mozilla-central/rev/8c2cb069b031#l3.43
Assignee: nobody → bugmail.mozilla
Attachment #8598211 - Flags: review?(botond)
Comment on attachment 8598211 [details] [diff] [review]
Patch

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

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +728,5 @@
>  
>      // The scrollbar thumb ratio is in AppUnits.
> +    const LayoutDeviceToParentLayerScale nonLayoutScale = effectiveZoom /
> +        metrics.GetDevPixelsPerCSSPixel();
> +    const float ratio = aScrollbar->GetScrollbarThumbRatio() / nonLayoutScale.scale;

It took a lot of staring at this and fiddling around with numbers to figure out why this is necessary, but I think I finally figured it out:

The units of Layer::GetScrollbarThumbRatio() (and nsSliderFrame::GetThumbRatio(), which is where it comes from) is "CSS pixels of the scroll frame's parent's space per CSS pixels of the scroll frame's space" (or equivalently, app units of ... per app units of ...), in spite of the comment in nsSliderFrame::GetThumbRatio() which claims it returns a "true unitless ratio".

Before this ratio can be treated as a true unitless ratio, therefore, it must be divided by the conversion factor from the parent space into the scroll frame's space, which is precisely what you're calling nonLayoutScale here.

r+ with the following comments added:

  - Comments for Layer::GetScrolbaerThumbRatio() and nsSliderFrame::GetThumbRatio()
    which state the correct units these are in.

  - A comment here, replacing "The scrollbar thumb ratio is in AppUnits", stating
    what we're doing with the ratio here.

Thanks!
Attachment #8598211 - Flags: review?(botond) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #1)
> However, I'm also wondering about the code at [1] for example. It too
> contains a "compositedWidth/scrollableWidth" term (inverted) - should that
> have been replaced by the thumb ratio as well?
> 
> [1] https://hg.mozilla.org/mozilla-central/rev/8c2cb069b031#l3.43

Oh and yes, good catch! Let's change these as well to use the (adjusted) ratio.
Updated with comments, carrying r+
Attachment #8598211 - Attachment is obsolete: true
Attachment #8598672 - Flags: review+
Fairly trivial patch and it seems to work as intended.
Attachment #8598674 - Flags: review?(botond)
Comment on attachment 8598674 [details] [diff] [review]
Part 2 - Use ratio more widely

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

Thanks!
Attachment #8598674 - Flags: review?(botond) → review+
https://hg.mozilla.org/mozilla-central/rev/bf1ed8e07f86
https://hg.mozilla.org/mozilla-central/rev/f27a83ace0ce
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Depends on: 1164340
You need to log in before you can comment on or make changes to this bug.