If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Scrollbars cause content to be shifted in e10s

RESOLVED FIXED in mozilla26

Status

()

Core
Layout
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: billm, Assigned: billm)

Tracking

unspecified
mozilla26
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
Created attachment 793220 [details] [diff] [review]
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.
Attachment #793220 - Flags: review?(matt.woodrow)
Attachment #793220 - Flags: review?(bugmail.mozilla)
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 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.
Attachment #793220 - Flags: review?(bugmail.mozilla) → review-
(Assignee)

Updated

4 years ago
Depends on: 907906
(Assignee)

Comment 3

4 years ago
Created attachment 793665 [details] [diff] [review]
scrollbar-visibility v2

I couldn't use Deflate because of bug 907906. This does an equivalent thing I think.
Attachment #793220 - Attachment is obsolete: true
Attachment #793220 - Flags: review?(matt.woodrow)
Attachment #793665 - Flags: review?(matt.woodrow)
Attachment #793665 - Flags: review?(bugmail.mozilla)
Attachment #793665 - Flags: review?(matt.woodrow) → review+
Attachment #793665 - Flags: review?(bugmail.mozilla) → review+
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.

Updated

4 years ago
Blocks: 904533
(Assignee)

Comment 5

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

I made the first change, but not the second.

Comment 6

4 years ago
Will this work ok on osx and metro where we have overlay scrollbars that don't take up any content space?
https://hg.mozilla.org/mozilla-central/rev/3c869e0d0028
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26

Comment 8

4 years ago
(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.

Updated

4 years ago
Depends on: 908748
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Flags: needinfo?(bugmail.mozilla)
Filed bug 909281.
Flags: needinfo?(bugmail.mozilla)
You need to log in before you can comment on or make changes to this bug.