976370 - don't let scrollbars interfere with the creation of a scrollable layer

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Timothy Nikkel (:tnikkel)]

Attachment: patch

If the only thing stopping scroll layer items from being merged is a scrollbar item for the same scroll frame we could (should?) just move the scrollbar up to be directly on top of the scroll layer item containing the scrolled content. This is a more desirably position for the scrollbar anyway, and it will let us create a scrollable layer. Note that the attached patch will also move a scrollbar that is directly below a scroll layer item that has merged fully successfully to be directly above it.

If the patch for bug 965593 is relanded we will need this so that it doesn't cause regressions like bug 975221.

Comment 1

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

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

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

::: layout/base/nsDisplayList.cpp
@@ +1003,5 @@
>    }
>    return opaqueClipped;
>  }
>  
> +/* Checks if aPotenialScrollItem is a scroll layer item and

s/aPotential/aPotential/, here and the actual variable name

Comment 2

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

Blocks a 1.3+ bug, so requesting 1.3+ on this too.

Comment 3

[Timothy Nikkel (:tnikkel)]

I'm not sure how to write a reftest for this since overlay scrollbars fadeaway, so any test would fail some of the time if enough time past for the scrollbars to start fading after we make them visible by scrolling.

Comment 4

[Timothy Nikkel (:tnikkel)]

Fixed the typos and pushed
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d1674345259

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4d1674345259

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Please request approval-mozilla-b2g28 for this patch for v1.3 uplift.

Comment 7

[Timothy Nikkel (:tnikkel)]

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

NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

Scrollbars are currently z-index: max 32bit int; on 1.3 so this will have very limited effects if any on 1.3 until bug 965593 is landed. But I think it's better to get this in sooner rather than later.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 965593
User impact if declined: slow scrolling in some cases, specifically in the sms app
Testing completed: just landed on m-c. fixes the bugs I was seeing.
Risk to taking this patch (and alternatives if risky): this should be pretty low risk, it moves overlay scrollbars up in the overall page stack by at most one slot if the item they are moving above contains the content the scrollbars are scrolling. it has a pretty limited effect, and should only improve things in those cases.
String or UUID changes made by this patch: none

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/3928aa00dd3f