907495 - Scrollbars cause content to be shifted in e10s

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: scrollbar-visibility

I'm running normal Firefox (not metro) with multiple processes and OMTC. When there are no scrollbars, everything works fine. When only one scrollbar is displayed, the content and the scrollbar both get shifted up or to the left by 13 pixels, which is the width of a scrollbar. Whether the shift is up or to the left is determined by which kind of scrollbar is shown. Strangely, when both scrollbars are displayed, everything works fine.

The problem seems to be that mCompositionBounds for the content layer includes the size of the scrollbars while mScrollableRect does not. As a consequence, this code here is triggered:

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/AsyncCompositionManager.cpp#597

The shift is caused by overscrollTranslation being non-zero.

:kats suggested that we should just remove the scrollbar size from mCompositionBounds. This patch appears to do that.

I should point out that clicking on the scrollbar still doesn't work--it just treats it as a click to the content under the scrollbar. I still need to figure out why that is.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 793220 [details] [diff] [review]
scrollbar-visibility

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

::: layout/base/nsDisplayList.cpp
@@ +689,5 @@
> +    nsMargin sizes = scrollableFrame->GetActualScrollbarSizes();
> +    int32_t w = nsPresContext::AppUnitsToIntCSSPixels(sizes.LeftRight());
> +    int32_t h = nsPresContext::AppUnitsToIntCSSPixels(sizes.TopBottom());
> +    metrics.mCompositionBounds.width -= w;
> +    metrics.mCompositionBounds.height -= h;

I think you want to convert sizes to an nsIntMargin, and then do metrics.mCompositionBounds.Deflate(sizes) here.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 793220 [details] [diff] [review]
scrollbar-visibility

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

I agree with Matt. In case of RTL pages the scrollbar might be on the left in which case the mCompositionBounds.x will need to be adjusted as well. Deflate will do that.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: scrollbar-visibility v2

I couldn't use Deflate because of bug 907906. This does an equivalent thing I think.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

FYI I landed bug 907906 which should allow you to do something like this:

ScreenIntMargin boundMargins(nsPresContext::AppUnitsToIntCSSPixels(sizes.top),
                             nsPresContext::AppUnitsToIntCSSPixels(sizes.right),
                             nsPresContext::AppUnitsToIntCSSPixels(sizes.bottom),
                             nsPresContext::AppUnitsToIntCSSPixels(sizes.left));
metrics.mCompositionBounds.Deflate(boundMargins);

We can even add a helper to the CSSPixel struct in layout/base/Units.h that will simplify this to:

ScreenIntMargin boundMargins = CSSIntMargin::FromAppUnitsRounded(sizes) * CSSToScreenScale(1.0); // 1.0 scale because the scrollbars are not subject to scaling
metrics.mCompositionBounds.Deflate(boundMargins);

If you don't want to do this right away I can do it as a follow-up, just let me know.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c869e0d0028

I made the first change, but not the second.

Comment 6

[Jim Mathies [:jimm]]

Will this work ok on osx and metro where we have overlay scrollbars that don't take up any content space?

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3c869e0d0028

Comment 8

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #6)
> Will this work ok on osx and metro where we have overlay scrollbars that
> don't take up any content space?

nope. will file a follow up.

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #4)
> FYI I landed bug 907906 which should allow you to do something like this:
> 
> ScreenIntMargin
> boundMargins(nsPresContext::AppUnitsToIntCSSPixels(sizes.top),
>                             
> nsPresContext::AppUnitsToIntCSSPixels(sizes.right),
>                             
> nsPresContext::AppUnitsToIntCSSPixels(sizes.bottom),
>                             
> nsPresContext::AppUnitsToIntCSSPixels(sizes.left));
> metrics.mCompositionBounds.Deflate(boundMargins);
> 
> We can even add a helper to the CSSPixel struct in layout/base/Units.h that
> will simplify this to:
> 
> ScreenIntMargin boundMargins = CSSIntMargin::FromAppUnitsRounded(sizes) *
> CSSToScreenScale(1.0); // 1.0 scale because the scrollbars are not subject
> to scaling
> metrics.mCompositionBounds.Deflate(boundMargins);
> 
> If you don't want to do this right away I can do it as a follow-up, just let
> me know.

Please file that; the code looks rather fishy right now.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Filed bug 909281.