691609 - Floating scrollbars draw under border/background

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Move the scrollbars from BorderBackground() to PositionedDescendants()

The floating scrollbars used in Fennec is draw between the BorderBackground() DisplayList and the rest of the scrollbox childs frames.
This mean the scrollbars will be draw under any other type of content.
The patch moves the scrollbars from the BorderBackground() list to the PositionnedDescendant() list to ensure they stay on top.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This came up in bug 657893. Can you extract the relevant part of https://bugzilla.mozilla.org/attachment.cgi?id=561485 ?

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

After have read bug 657893, I think we want to take the code from this patch for moving the scrollbars to another display list instead of the relevant part of https://bugzilla.mozilla.org/attachment.cgi?id=561485 .
There is probably a comment I can borrow since it seems like the PositionnedDescendant() list is required to not mess up with layers and we probably want to keep this information to not change the list by error later.

I could be wrong since I don't know that much the layout code but I don't think there is any reason for keeping 2 calls to ::AppendScrollPartsTo() since as far as I understand if the scrollbars are placed in the PositionedDescendant() list, they will be ordered by z-index, and will leave on top of content by default if nothing has a defined z-index.

Patches in bug 657893 will probably merge easily on top of this one. 
Adding Florian to the discussion in case I have misunderstood his patches.

Comment 3

[Markus Stange [:mstange]]

Attachment: alternative patch

I hit the same problem in bug 636564. This is the patch I was going to attach there. It gets rid of the scrollbar overlay system metric / pref and instead makes the decision based on whether the scrollbar is positioned.
Does the mobile overlay scrollbar CSS include position:relative on the scrollbar? If not, it should.

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Markus Stange from comment #3)
> Does the mobile overlay scrollbar CSS include position:relative on the
> scrollbar? If not, it should.

It does. I do like the  idea of removing the pref.

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 564404 [details] [diff] [review]
Move the scrollbars from BorderBackground() to PositionedDescendants()

Removing r? since the latest proposed approach is likely better.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 564462 [details] [diff] [review]
alternative patch

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

This patch looks good, but instead of having two functions, just pass a parameter to indicate whether to use the positioned scroll parts or not.

Comment 7

[Markus Stange [:mstange]]

Attachment: v2

merged the functions

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Can we land this bug or does it rely on something else?

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It can land AFAIK

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/7f8c69ad0895

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://hg.mozilla.org/mozilla-central/rev/7f8c69ad0895