Closed Bug 1164340 Opened 5 years ago Closed 5 years ago

subframe scrollbars not async transformed properly in some cases

Categories

(Core :: Panning and Zooming, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: tnikkel, Assigned: tnikkel)

References

()

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch patch (obsolete) — Splinter Review
Attachment #8605078 - Flags: review?(botond)
Attached patch patch (obsolete) — Splinter Review
With both horizontal and vertical scrollbar code now.
Attachment #8605078 - Attachment is obsolete: true
Attachment #8605078 - Flags: review?(botond)
Attachment #8605079 - Flags: review?(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).
Attachment #8605079 - Flags: review?(botond)
Attached patch patch v2Splinter Review
Attachment #8605079 - Attachment is obsolete: true
Attachment #8605533 - Flags: review?(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.
Attachment #8605533 - Flags: review?(botond) → review+
Okay, I put another patch in my queue to make those comment changes and will push with the main patch.
You need to log in before you can comment on or make changes to this bug.