1164340 - subframe scrollbars not async transformed properly in some cases

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Timothy Nikkel (:tnikkel)]

Load https://bugzilla.mozilla.org/show_bug.cgi?id=1133905 and try to scroll the first comment, watch the scrollbar. It looks like the async transform is moving it too far, and then it gets corrected back to where it should be.

Before bug 1163259 we were finding the wrong metrics (from the parent process) which didn't have any zoom so the scrollbars just got updated when layout updated mostly.

The problem is that the fix for bug 1158933 wasn't quite sufficient for subframes.

bug 1158933, comment 2
(In reply to Botond Ballo [:botond] from comment #2)
> 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.

This is true for scrollframes that are zoomable. But non-zoomable ones always have this conversion factor as 1.

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Comment 2

[Timothy Nikkel (:tnikkel)]

Attachment: patch

With both horizontal and vertical scrollbar code now.

Comment 3

[Botond Ballo [:botond]]

Comment on attachment 8605079 [details] [diff] [review]
patch

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

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +732,5 @@
> +    // to the scrollframe's space. Only the root is zoomable, so only for the
> +    // root does this differ from 1.
> +    if (metrics.IsRootScrollable()) {
> +      ratio = ratio / nonLayoutScale.scale;
> +    }

Discussed on IRC. Summary: 

  - nonLayoutScale should exclude the parent resolution and the 
    css-driven resolution, and then it's correct to use it for 
    both root frames and subframes (it will be 1 for subframes 
    in the absence of subframe zooming).

  - The fact that nonLayoutScale currently includes the parent 
    resolution and the css-driven resolution hasn't been problematic
    for the root frame, because those values are typically 1
    for the root frame (but we shouldn't necessarily be assuming
    that).

Comment 4

[Timothy Nikkel (:tnikkel)]

Attachment: patch v2

Comment 5

[Botond Ballo [:botond]]

Comment on attachment 8605533 [details] [diff] [review]
patch v2

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

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +720,5 @@
>      const float yScale = 1.f / asyncZoomY;
>  
>      // Note: |metrics.GetZoom()| doesn't yet include the async zoom, so
>      // |metrics.CalculateCompositedSizeInCssPixels()| would not give a correct
>      // result.

Since you're touching this code anyways, could you truncate this comment to just "Note: |metrics.GetZoom()| doesn't yet include the async zoom."? (The rest of the comment was referring to code which has since been removed.)

@@ +727,3 @@
>      // Here we convert the scrollbar thumb ratio into a true unitless ratio by
>      // dividing out the conversion factor from the scrollframe's parent's space
>      // to the scrollframe's space.

Can we add back the following comment, which was accidentally removed in bug 1152469:

// The scroll thumb needs to be translated in opposite direction of the
// async scroll. This is because scrolling down, which translates the layer
// content up, should result in moving the scroll thumb down.

Comment 6

[Timothy Nikkel (:tnikkel)]

Okay, I put another patch in my queue to make those comment changes and will push with the main patch.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/da51150e5d76
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ada89ffc2de

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/da51150e5d76
https://hg.mozilla.org/mozilla-central/rev/9ada89ffc2de